-
Notifications
You must be signed in to change notification settings - Fork 13
Upgrade dependencies and improve resource handling #9
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
Conversation
|
38f5a43
to
f98149b
Compare
This reverts commit 7d48f33.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
These three tests keep getting stuck. Curious if you all can replicate? You need an Openai key in your env. And then just run |
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just had one question about the CI setup.
hiredis
(as this is tested in RedisVL), but adding support forPython 3.13
andRedis 8
pytest
command)--run-api-tests
So this was actually a bit tricky based on a few findings:
RedisConnectionFactory
deprecatesget_async_redis_connection
in favor of_get_aredis_connection
, with the latter being anasync
method. It also performs module validation which is good. However, if we cutover, we'd need to implement some lazy connection strategy, but then I started to get lost so I left that out for now.RedisConnectionFactory
handles that, and should probably be done here somehow?@abrookins might ask if you have a moment early next week to take a crack at this one since you helped set up the async logic in the package and also contributed that work on redisvl. This is a start though! thanks guys!