Skip to content
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

Enhanced Type Hints for Improved Readability and Maintainability: Addressing Type Mismatch in query_quadrant Return Statement #2452

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Hk669
Copy link
Collaborator

@Hk669 Hk669 commented Apr 19, 2024

Why are these changes needed?

  1. I've updated the type hints of the *_utils to improve code readability, maintainability, and to facilitate static type checking. This enhancement will make it easier for developers to understand the expected types of parameters and return values, reducing the likelihood of type-related errors and improving overall code quality.

  2. updated the query_quadrant in qdrant_retrieve_user_proxy_agent.py, the return statement doesn't match with the type mentioned. otherwise returns a dict, could be a problem for the developer to use query_quadrant.

Related issue number

No issue is related.

Checks

@Hk669 Hk669 requested a review from ekzhu April 20, 2024 16:10
@Hk669
Copy link
Collaborator Author

Hk669 commented May 3, 2024

@sonichi i've no idea why the compression test is failing, which isn't relevant to this PR. maybe because of the previous commits?
i made some changes in the test_compressible_agent, any suggestions would be greatly appreciated.

@sonichi sonichi requested a review from yiranwu0 May 5, 2024 14:52
@sonichi
Copy link
Collaborator

sonichi commented May 5, 2024

@yiranwu0 @WaelKarkoub @gagb is it time to retire compressible agent? Have we reached feature parity with the TransformMessages capability?

@WaelKarkoub
Copy link
Collaborator

@yiranwu0 @WaelKarkoub @gagb is it time to retire compressible agent? Have we reached feature parity with the TransformMessages capability?

@sonichi I'm working on LLMLingua support, once that goes in, we should retire compressible agent. #2225

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the code with a source url, shall we keep as original?
Another issue is that this file is not covered by test right now because the test that covered this file in v0.1 hasn't been migrated to v0.2.

Copy link
Collaborator Author

@Hk669 Hk669 May 5, 2024

Choose a reason for hiding this comment

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

i think, these changes are required if, these utils are being used in any of our modules or the developer. there are some return types to be updated, which is mentioned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sonichi shall i cover the tests for the utils in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible that we'll deprecate this util file. It's better to avoid changes to this file in this PR, especially the code which contains a source url.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure @sonichi , lets not make changes anymore to this file. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants