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

Bump version to 1.7.0 #36

Merged
merged 26 commits into from
Dec 8, 2023
Merged

Bump version to 1.7.0 #36

merged 26 commits into from
Dec 8, 2023

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Dec 7, 2023

Depends on qdrant/qdrant#3175.

WIP

@timvisee timvisee mentioned this pull request Dec 7, 2023
19 tasks
build/Main.cs Outdated Show resolved Hide resolved
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Dec 7, 2023

Also, just to clarify, I already figured why the build fails and how to fix it and currently working on it. (Thanks for the help, @IvanPleshkov! 🙏)

@russcam
Copy link
Collaborator

russcam commented Dec 7, 2023

@ffuugoo my apologies, this needs to be removed in qdrant/qdrant:

https://github.com/qdrant/qdrant/blob/cafab322d08b1dd38204063e756a9c1b59bd9a72/lib/api/src/grpc/proto/health_check.proto#L5

The namespace should only have been added to qdrant package protos 😵‍💫

Integrations tests are configured to also run against docker image version specified by <QdrantVersion>. Change this version to run against another docker image version:

public const string QdrantImage = "qdrant/qdrant:" + QdrantGrpcClient.QdrantVersion;

TODO:
- Remove "internal" imports from `qdrant.proto`
...but I suck at C# and my code doesn't work 😂
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Dec 7, 2023

@IvanPleshkov suggested to take over and help with C# bits (cause I can't do C#).

@timvisee
Copy link
Member

timvisee commented Dec 7, 2023

@russcam

my apologies, this needs to be removed in qdrant/qdrant:

https://github.com/qdrant/qdrant/blob/cafab322d08b1dd38204063e756a9c1b59bd9a72/lib/api/src/grpc/proto/health_check.proto#L5

The namespace should only have been added to qdrant package protos 😵‍💫

Remove just that single line from the protocol definitions? I can quickly arrange that.

Edit: qdrant/qdrant#3180

@ffuugoo
Copy link
Contributor Author

ffuugoo commented Dec 7, 2023

Remove just that single line from the protocol definitions? I can quickly arrange that.

I've added a more "generic" fix for this into this PR (and also created a similar-ish qdrant/qdrant#3181 PR, sorry).

build/Main.cs Outdated Show resolved Hide resolved
@russcam
Copy link
Collaborator

russcam commented Dec 7, 2023

@timvisee yes, removing that one line will fix compilation.

The other issue that will arise is that the integration tests are configured to run against the same docker image label as specified by <QdrantVersion>, so if that image is not yet available in docker repo, they will fail. The simple fix for this PR is as per #36 (comment). I can open another PR to control this docker version separately too if it would be helpful.

@IvanPleshkov
Copy link
Contributor

After QDrant v1.7.0 publishing we will revert hardcoded pathes and versions.

@russcam could you please review C# changes meanwhile? We will public client asap after qdrant release

Copy link
Collaborator

@russcam russcam left a comment

Choose a reason for hiding this comment

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

left some minor nits and suggestions about keeping timeout and cancellationToken parameters as the last parameters.

src/Qdrant.Client/Grpc/ShardKey.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/Grpc/SparseVectorConfig.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/Grpc/Vectors.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/Grpc/Vector.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/LoggingExtensions.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/QdrantClient.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/QdrantClient.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/QdrantClient.cs Outdated Show resolved Hide resolved
src/Qdrant.Client/QdrantClient.cs Outdated Show resolved Hide resolved
build/Main.cs Outdated Show resolved Hide resolved
@ffuugoo
Copy link
Contributor Author

ffuugoo commented Dec 8, 2023

@IvanPleshkov is doing God's work here. Thanks for picking this up! 😭🙏

@timvisee timvisee marked this pull request as ready for review December 8, 2023 10:25
@timvisee timvisee merged commit 37c1201 into main Dec 8, 2023
1 check passed
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