Skip to content

Upgrade redisvl and improve resource management #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 25, 2025

Conversation

tylerhutcherson
Copy link
Collaborator

@tylerhutcherson tylerhutcherson commented Feb 24, 2025

  • Updates RedisVL and other libraries to best ranges.
  • Implements lifespan hooks for app startup and cleanup.
  • Cleans up resources on shutdown.
  • Fixes test fixtures for ideal loading and usage.

@tylerhutcherson tylerhutcherson added the enhancement New feature or request label Feb 24, 2025
@tylerhutcherson tylerhutcherson changed the title Upgrade redisvl Upgrade redisvl and improve resource management Feb 25, 2025
@tylerhutcherson
Copy link
Collaborator Author

Wierd -- when I implement the same updates to arxiv search I am seeing the warning/errors in the logs":

https://github.com/redis-developer/redis-arXiv-search/actions/runs/13512854236/job/37756259025?pr=32

I must be doing something different/wrong there. My eyes are tired! Will check back in the morning.

@abrookins
Copy link

Ah, yes. FastAPI's lifespan context manager doesn't run during tests if you use AsyncClient, only TestClient (and write your tests with sync Python). The logging I/O error is related to Pytest, which creates the ugly traceback, but the message means we have an async client instance that isn't getting cleaned up. That'll be true if the lifespan function never runs during tests (it doesn't, I checked).

There are some elaborate ways you can make the lifespan events fire during tests, but in our case, we can get away with an autouse Pytest fixture. Just pushed to your branch.

As for the Pytest logging issue, there isn't a great way to prevent that, but we can use another fixture to help. I pushed one up. But because we're logging within other people's projects as a library, it may be worth suppressing the logging message in our weakref finalizer.

@tylerhutcherson
Copy link
Collaborator Author

tylerhutcherson commented Feb 25, 2025

Ah, yes. FastAPI's lifespan context manager doesn't run during tests if you use AsyncClient, only TestClient (and write your tests with sync Python). The logging I/O error is related to Pytest, which creates the ugly traceback, but the message means we have an async client instance that isn't getting cleaned up. That'll be true if the lifespan function never runs during tests (it doesn't, I checked).

There are some elaborate ways you can make the lifespan events fire during tests, but in our case, we can get away with an autouse Pytest fixture. Just pushed to your branch.

As for the Pytest logging issue, there isn't a great way to prevent that, but we can use another fixture to help. I pushed one up. But because we're logging within other people's projects as a library, it may be worth suppressing the logging message in our weakref finalizer.

To get around the lifespan issue, I had used the asgi-lifespan pkg LifespanManager as a context manager around the httpx client and that manually would fire those lifespan events and cleanup on exit. It adds yet another dependency, but it is "clean".

Only thing tripping me up is that I see the logging error/warning for the arxiv demo but not this one.

Copy link
Contributor

@rbs333 rbs333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me maybe we can rubber duck on the warning you're getting in arxiv later.

@tylerhutcherson
Copy link
Collaborator Author

Landed on using the LifespanManager class from asgi-lifespan because the hooks don't fire for pytest

Copy link

@abrookins abrookins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@tylerhutcherson tylerhutcherson merged commit 081e241 into master Feb 25, 2025
1 check passed
@tylerhutcherson tylerhutcherson deleted the feat/RAAE-671-upgrade-redisvl branch February 25, 2025 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants