-
Notifications
You must be signed in to change notification settings - Fork 139
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
[gen grpc] Include pyi grpc files #917
base: dev
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
📝 WalkthroughWalkthroughThe pull request adds a new dependency, ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
qdrant_client/proto/points.proto (2)
599-602
: Addition ofGeoDistance
MessageThe new
GeoDistance
message correctly encapsulates geographic distance information with an origin and a target. For clarity, consider renaming the fieldto
to something likedestination
if it represents a target location.
612-616
: Addition ofDivExpression
MessageThe
DivExpression
message introduces fields forleft
andright
operands, plus an optionalby_zero_default
to address division by zero. Verify that the generated client and the server implementation use this default value consistently to prevent potential runtime exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lock
is excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.toml
(1 hunks)qdrant_client/grpc/collections_service_pb2.pyi
(1 hunks)qdrant_client/grpc/json_with_int_pb2.pyi
(1 hunks)qdrant_client/grpc/points_service_pb2.pyi
(1 hunks)qdrant_client/grpc/qdrant_pb2.pyi
(1 hunks)qdrant_client/grpc/snapshots_service_pb2.pyi
(1 hunks)qdrant_client/proto/points.proto
(3 hunks)tools/generate_grpc_client.sh
(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- qdrant_client/grpc/points_service_pb2.pyi
- qdrant_client/grpc/collections_service_pb2.pyi
- qdrant_client/grpc/qdrant_pb2.pyi
- qdrant_client/grpc/snapshots_service_pb2.pyi
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
- GitHub Check: Python 3.13 test
🔇 Additional comments (13)
qdrant_client/proto/points.proto (6)
575-578
: New Message Added:Formula
The new
Formula
message encapsulates a scoring formula by holding anExpression
along with a defaults map. This design is clear and aligns with expanding query expressiveness.
580-597
: ComprehensiveExpression
DefinitionThe
Expression
message now supports a wide range of operations (constant, variable, condition, various arithmetic operations, and logarithmic/exponential functions) via its oneof variant. This provides excellent flexibility for constructing complex formulas. Please ensure that the documentation or usage examples clarify each variant's intended usage.
604-606
: NewMultExpression
MessageThe introduction of
MultExpression
employing a repeatedExpression
field to represent multiplication is straightforward and meets the design needs.
608-610
: NewSumExpression
MessageThe
SumExpression
message is well-defined with a repeated field for addends to perform summation. This addition is clear and consistent with similar arithmetic operations.
618-621
: Introduction ofPowExpression
MessageThe new
PowExpression
neatly captures power operations withbase
andexponent
fields. The design is clear and should integrate well with the other mathematical expression messages.
1048-1050
: Enhancement ofFieldCondition
The extension of
FieldCondition
withoptional bool is_empty
andoptional bool is_null
provides additional granularity for filtering based on field content. Ensure that the backend logic correctly interprets the distinction between "empty" and "null" values.pyproject.toml (1)
55-55
: Added necessary dependency for enhanced type checking.The addition of
mypy-protobuf
at version^3.3.0
aligns perfectly with the PR objectives to improve the type stubs for gRPC client. This dependency is used to generate TypeScript-like interface files (.pyi
) for Protocol Buffers, which significantly enhances type checking capabilities when working with gRPC services.tools/generate_grpc_client.sh (2)
7-7
: Good flexibility improvement in grpcio dependency.Changing from a fixed version to a minimum version requirement (
grpcio>=1.48.2
) provides better flexibility for the build environment while maintaining backward compatibility. This approach allows users to benefit from bug fixes and performance improvements in newer versions of grpcio without being stuck on a specific version.
43-48
: Improved code generation with type stub support.The reformatted command with the addition of
--mypy_out=./qdrant_client/grpc
is a significant enhancement that enables the generation of.pyi
type stub files. These files provide detailed type annotations for the gRPC client, improving compatibility with static type checkers like pyright and tools like ruff, which is the core objective of this PR.The multi-line format also improves readability and maintainability of the script.
qdrant_client/grpc/json_with_int_pb2.pyi (4)
1-4
: Auto-generated type stub file as expected.This is an auto-generated file by mypy-protobuf, as correctly indicated in the header comments. These type stubs are essential for improving the developer experience when working with the gRPC client.
14-17
: Python version compatibility handling is appropriate.The conditional import based on Python version ensures compatibility with both older and newer Python versions, which is a good practice for a library that supports multiple Python versions.
95-131
: Complete type definitions for the Value class.The
Value
class contains comprehensive type definitions for various value types, including explicit support for integer values (line 106), which is important for strong typing in Python. TheWhichOneof
method properly defines the possible field names, and all fields are properly documented.
135-154
:✅ Verification successful
Verify ListValue implementation consistency.
The ListValue class looks well-defined with proper type annotations. To ensure consistency, you might want to verify that the implementation aligns with how lists of values are handled in the rest of your codebase.
🏁 Script executed:
#!/bin/bash # Check how ListValue is used across the codebase rg -A 5 -B 5 "ListValue" --glob "*.py" | grep -v "json_with_int_pb2.pyi"Length of output: 5472
ListValue Implementation Consistency Verified
The usage of
ListValue
in the conversion logic (as seen inqdrant_client/conversions/conversion.py
) aligns with the type definitions provided inqdrant_client/grpc/json_with_int_pb2.pyi
. The class is correctly defined with proper type annotations and behaves as expected when wrapping list values. No further modifications are necessary.
This PR picks up from #506 to improve DX when working with conversions.
Regenerates the gRPC client but with better type stubs that actually work with pyright and ruff.