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

fix type boolean restriction on include vector for _QueryReference #1399

Merged
merged 6 commits into from
Nov 11, 2024

Conversation

paul7Junior
Copy link
Contributor

@paul7Junior paul7Junior commented Nov 10, 2024

This pull request is a proposed solution for the issue opened #1400

This change the type validation on include_vector parameter for _QueryReference that prevents from requesting named vectors using array of string on references as it would force you to pull all vectors with True, which is not convenient if you have a lots of vectors.

But using True seems like would not work either and is related to another issue I opened here: weaviate/weaviate#6279

This is my first pull request on here, happy to get feedbacks and add more tests.

@weaviate-git-bot
Copy link

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

@paul7Junior
Copy link
Contributor Author

I agree with the Contributor License Agreement.

@paul7Junior paul7Junior marked this pull request as ready for review November 11, 2024 14:49
@paul7Junior
Copy link
Contributor Author

hey Tommy,

i fixed the error related to lower versions not supporting namedVectors.

but not quite sure whats triggering this one: https://github.com/weaviate/weaviate-python-client/actions/runs/11780176232/job/32810722328?pr=1399

@tsmith023
Copy link
Contributor

Hi @paul7Junior, that fail looks like a known flake, which sadly there are a few in the CI! I'll rerun the tests again until they are all green and then I'll give it a review. Thanks so much for this contribution ❤️

Copy link
Contributor

@tsmith023 tsmith023 left a comment

Choose a reason for hiding this comment

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

Thanks for the added test! Only one comment about it, feel free to say no and we can merge it as it is 😛

integration/test_named_vectors.py Outdated Show resolved Hide resolved
@dirkkul dirkkul merged commit 6af9d69 into weaviate:main Nov 11, 2024
43 of 44 checks passed
@dirkkul
Copy link
Collaborator

dirkkul commented Nov 11, 2024

Thank you!

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.

4 participants