Skip to content

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

Merged
merged 5 commits into from
Feb 28, 2025

Conversation

tylerhutcherson
Copy link
Contributor

@tylerhutcherson tylerhutcherson commented Feb 22, 2025

  • PR implements a few dependency upgrades (redisvl) and fixes a dependabot security warning
  • Updates CI/CD flow to mirror RedisVL, removing hiredis (as this is tested in RedisVL), but adding support for Python 3.13 and Redis 8
  • Improves test running modularity (supporting any optional args on a pytest command)
  • Adds integration tests to the CI flow with the optional flag --run-api-tests
  • Updates async resource handling where appropriate
  • Removes anthropic from tests in favor of OpenAI for package simplicity and dependency management

So this was actually a bit tricky based on a few findings:

  1. The RedisConnectionFactory deprecates get_async_redis_connection in favor of _get_aredis_connection, with the latter being an async 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.
  2. This package as a whole doesn't do module validation or CLIENT SETINFO stuff if the user provides their own Redis client. The RedisConnectionFactory handles that, and should probably be done here somehow?
  3. The failing tests seem to be linked to the order of client initialization steps, and it no longer throwing an error?

@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!

@tylerhutcherson tylerhutcherson added the enhancement New feature or request label Feb 22, 2025
@tylerhutcherson tylerhutcherson changed the title refactor to use redisvl 0.4.1+ WIP -- refactor to use redisvl 0.4.1+ Feb 22, 2025
@abrookins
Copy link
Contributor

abrookins commented Feb 24, 2025

  1. For get_async_redis_connection, what we're going to deprecate is calling it as a sync function. So for now, users should keep using the function but will need to await it in 1.0.0.
  2. In the search index classes, we validate an existing connection if you use from_existing, so I think we should do the same in RedisVL if you supply a client in init. I can release that in a point release.
  3. I'll check those failing tests! Since the connection is now lazy, if you want to test a connection error, you have to do something that uses the connection. I'll update the tests.

@tylerhutcherson tylerhutcherson changed the title WIP -- refactor to use redisvl 0.4.1+ Upgrade redisvl and improve async resource handling Feb 27, 2025
@tylerhutcherson tylerhutcherson changed the title Upgrade redisvl and improve async resource handling Upgrade dependencies and improve resource handling Feb 27, 2025
@tylerhutcherson tylerhutcherson marked this pull request as ready for review February 27, 2025 13:50
@tylerhutcherson tylerhutcherson requested review from abrookins and bsbodden and removed request for abrookins February 27, 2025 13:51
@tylerhutcherson tylerhutcherson force-pushed the feat/RAAE-681-upgrade-redisvl branch 2 times, most recently from 38f5a43 to f98149b Compare February 27, 2025 14:38
Copy link
Contributor

@bsbodden bsbodden 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
Copy link
Contributor Author

These three tests keep getting stuck. Curious if you all can replicate? You need an Openai key in your env. And then just run make test-all locally.

@tylerhutcherson
Copy link
Contributor Author

These three tests keep getting stuck. Curious if you all can replicate? You need an Openai key in your env. And then just run make test-all locally.

Fixed.

Copy link
Contributor

@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.

Looks great! Just had one question about the CI setup.

@tylerhutcherson tylerhutcherson merged commit cada331 into main Feb 28, 2025
19 checks passed
@tylerhutcherson tylerhutcherson deleted the feat/RAAE-681-upgrade-redisvl branch February 28, 2025 14:48
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