Added python gRPC API bindings#618
Conversation
gaius-qi
left a comment
There was a problem hiding this comment.
Please remove v2 path => api/*.py
There was a problem hiding this comment.
Pull request overview
This PR adds Python gRPC API bindings for Dragonfly by generating Python code from existing protobuf definitions. This serves as the foundation (step 1) for a future Python SDK.
Changes:
- Added generated Python gRPC stub files from protobuf definitions for scheduler, manager, dfdaemon, common, and errordetails APIs
- Added pyproject.toml for Python package configuration with setuptools backend
- Added basic README documenting the package
Reviewed changes
Copilot reviewed 12 out of 20 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| python/pyproject.toml | Package configuration defining dependencies (grpcio, protobuf) and build system |
| python/api/v2/*.py | Generated Python gRPC stubs for v2 API (scheduler, manager, dfdaemon, common, errordetails) |
| python/api/v2/pycache/*.pyc | Compiled Python bytecode files (should NOT be committed) |
| python/README.md | Basic documentation for the package |
| .gitignore | Added .venv/ pattern for Python virtual environments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
python/api/v2/scheduler_pb2_grpc.py
Outdated
| raise NotImplementedError('Method not implemented!') | ||
|
|
||
| def DeleteCachePeer(self, request, context): | ||
| """DeletCacheePeer releases cache peer in scheduler. |
There was a problem hiding this comment.
Typo in docstring: "DeletCacheePeer" should be "DeleteCachePeer" (remove the extra 'e'). This is auto-generated code from the protobuf definition, so the typo exists in the source .proto file and should be fixed there before regenerating.
| """DeletCacheePeer releases cache peer in scheduler. | |
| """DeleteCachePeer releases cache peer in scheduler. |
| raise NotImplementedError('Method not implemented!') | ||
|
|
||
| def ListSeedPeers(self, request, context): | ||
| """List acitve schedulers configuration. |
There was a problem hiding this comment.
Typo in docstring: "acitve" should be "active". This appears twice (lines 98 and 133). This is auto-generated code from the protobuf definition, so the typo exists in the source .proto file and should be fixed there before regenerating.
| raise NotImplementedError('Method not implemented!') | ||
|
|
||
| def ListSchedulers(self, request, context): | ||
| """List acitve schedulers configuration. |
There was a problem hiding this comment.
Typo in docstring: "acitve" should be "active". This is the second occurrence of the same typo. This is auto-generated code from the protobuf definition, so the typo exists in the source .proto file and should be fixed there before regenerating.
python/README.md
Outdated
| ## Python Dragonfly API | ||
|
|
||
| Generated Python gRPC bindings for Dragonfly. | ||
|
|
||
| Source protos in https://github.com/dragonflyoss/api | ||
|
|
There was a problem hiding this comment.
The Python package is missing essential metadata and configuration files. Consider adding: 1) A MANIFEST.in file to ensure non-Python files (like README.md) are included in distributions, 2) Package metadata fields in pyproject.toml (authors, license, repository URL, etc.), 3) A way to specify which packages to include (either explicitly listing them or using [tool.setuptools.packages.find]). Without these, the package may not install or distribute correctly.
| ] | ||
|
|
||
| [build-system] | ||
| requires = ["setuptools"] | ||
| build-backend = "setuptools.build_meta" |
There was a problem hiding this comment.
The pyproject.toml is missing several important metadata fields for a proper Python package distribution: 'authors', 'readme', 'license', 'homepage', and 'repository'. Additionally, the package should specify 'packages' or use a proper package discovery mechanism to ensure the API modules are included in the distribution. Consider adding: authors = [{name = "...", email = "..."}], readme = "README.md", license = {text = "..."}, and [tool.setuptools.packages.find] to automatically discover packages.
| ] | |
| [build-system] | |
| requires = ["setuptools"] | |
| build-backend = "setuptools.build_meta" | |
| ] | |
| authors = [ | |
| { name = "Your Name", email = "you@example.com" }, | |
| ] | |
| readme = "README.md" | |
| license = { text = "Proprietary" } | |
| [project.urls] | |
| homepage = "https://example.com/dragonfly-api" | |
| repository = "https://example.com/dragonfly-api/repository" | |
| [build-system] | |
| requires = ["setuptools"] | |
| build-backend = "setuptools.build_meta" | |
| [tool.setuptools.packages.find] | |
| include = ["dragonfly_api*"] |
python/api/v2/scheduler_pb2.py
Outdated
|
|
||
|
|
||
| from . import common_pb2 as common__pb2 | ||
| from . import errordetails_pb2 as errordetails__pb2 |
There was a problem hiding this comment.
Import of 'errordetails__pb2' is not used.
| from . import errordetails_pb2 as errordetails__pb2 |
python/api/v2/scheduler_pb2.py
Outdated
|
|
||
| from . import common_pb2 as common__pb2 | ||
| from . import errordetails_pb2 as errordetails__pb2 | ||
| from google.protobuf import duration_pb2 as google_dot_protobuf_dot_duration__pb2 |
There was a problem hiding this comment.
Import of 'google_dot_protobuf_dot_duration__pb2' is not used.
python/api/v2/scheduler_pb2.py
Outdated
| from . import common_pb2 as common__pb2 | ||
| from . import errordetails_pb2 as errordetails__pb2 | ||
| from google.protobuf import duration_pb2 as google_dot_protobuf_dot_duration__pb2 | ||
| from google.protobuf import empty_pb2 as google_dot_protobuf_dot_empty__pb2 |
There was a problem hiding this comment.
Import of 'google_dot_protobuf_dot_empty__pb2' is not used.
python/api/v2/scheduler_pb2.py
Outdated
| from . import errordetails_pb2 as errordetails__pb2 | ||
| from google.protobuf import duration_pb2 as google_dot_protobuf_dot_duration__pb2 | ||
| from google.protobuf import empty_pb2 as google_dot_protobuf_dot_empty__pb2 | ||
| from google.protobuf import timestamp_pb2 as google_dot_protobuf_dot_timestamp__pb2 |
There was a problem hiding this comment.
Import of 'google_dot_protobuf_dot_timestamp__pb2' is not used.
| from google.protobuf import timestamp_pb2 as google_dot_protobuf_dot_timestamp__pb2 |
python/api/v2/scheduler_pb2_grpc.py
Outdated
| # Generated by the gRPC Python protocol compiler plugin. DO NOT EDIT! | ||
| """Client and server classes corresponding to protobuf-defined services.""" | ||
| import grpc | ||
| import warnings |
There was a problem hiding this comment.
Import of 'warnings' is not used.
| import warnings |
gaius-qi
left a comment
There was a problem hiding this comment.
Please generate code by namely/protoc-all, refer to https://github.com/dragonflyoss/api/blob/main/hack/protoc.sh.
|
Sure, on it @gaius-qi |
|
Should I address all Copilot comments as well or https://github.com/dragonflyoss/api/blob/main/hack/protoc.sh would be enough? |
|
Hey @gaius-qi I regenerated Python bindings using namely/protoc-all and flattened api/v2/ to api/ per your review. Currently only v2 protos are generated. Happy to add v1 bindings if needed. |
Signed-off-by: chinmaychahar <chinmay.cc.06@gmail.com>
Signed-off-by: chinmaychahar <chinmay.cc.06@gmail.com>
a3ef700 to
0bc3fc1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| import grpc | ||
|
|
||
| from google.protobuf import empty_pb2 as google_dot_protobuf_dot_empty__pb2 | ||
| from pkg.apis.manager.v2 import manager_pb2 as pkg_dot_apis_dot_manager_dot_v2_dot_manager__pb2 |
There was a problem hiding this comment.
The generated stub imports pkg.apis.manager.v2.manager_pb2, but this PR adds python/api/manager_pb2.py instead of a pkg/apis/manager/v2/manager_pb2.py module. As a result, importing manager_pb2_grpc will raise ModuleNotFoundError unless the generated files are placed in the expected pkg/apis/... package hierarchy (or the generation options/imports are adjusted).
| from pkg.apis.manager.v2 import manager_pb2 as pkg_dot_apis_dot_manager_dot_v2_dot_manager__pb2 | |
| from . import manager_pb2 as pkg_dot_apis_dot_manager_dot_v2_dot_manager__pb2 |
|
|
||
|
|
||
| from google.protobuf import empty_pb2 as google_dot_protobuf_dot_empty__pb2 | ||
| from pkg.apis.common.v2 import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 |
There was a problem hiding this comment.
This generated module imports pkg.apis.common.v2.common_pb2, but the PR places common_pb2.py under python/api/ rather than pkg/apis/common/v2/. As-is, import ...manager_pb2 will fail; the generated Python files need to live in a directory hierarchy matching the proto paths (or be regenerated with matching import paths).
| from pkg.apis.common.v2 import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | |
| from . import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 |
| """List acitve schedulers configuration. | ||
| """ |
There was a problem hiding this comment.
Typo in docstring: "acitve" should be "active".
| ] | ||
|
|
||
| [build-system] | ||
| requires = ["setuptools"] |
There was a problem hiding this comment.
setuptools is unpinned in build-system.requires. Since this project uses PEP 621 [project] metadata, older setuptools versions can fail to build; consider requiring setuptools>=61 (and typically include wheel as well) to make builds reproducible.
| requires = ["setuptools"] | |
| requires = ["setuptools>=61", "wheel"] |
| from pkg.apis.common.v2 import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | ||
| from pkg.apis.scheduler.v2 import scheduler_pb2 as pkg_dot_apis_dot_scheduler_dot_v2_dot_scheduler__pb2 |
There was a problem hiding this comment.
These imports reference modules under pkg.apis.* (e.g. pkg.apis.common.v2.common_pb2), but this PR places the generated files under python/api/*.py. With the current layout, importing this module will fail because the pkg/apis/... Python package hierarchy doesn’t exist. Either generate the Python outputs into a directory structure that matches the proto paths (so pkg/apis/.../*_pb2.py exists under the package root) or regenerate with import paths that match python/api.
| from pkg.apis.common.v2 import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | |
| from pkg.apis.scheduler.v2 import scheduler_pb2 as pkg_dot_apis_dot_scheduler_dot_v2_dot_scheduler__pb2 | |
| from . import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | |
| from . import scheduler_pb2 as pkg_dot_apis_dot_scheduler_dot_v2_dot_scheduler__pb2 |
| from pkg.apis.common.v2 import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | ||
| from pkg.apis.dfdaemon.v2 import dfdaemon_pb2 as pkg_dot_apis_dot_dfdaemon_dot_v2_dot_dfdaemon__pb2 |
There was a problem hiding this comment.
Like the other generated modules, this file imports pkg.apis.* modules that are not present under python/ in this PR (the generated code is in python/api). Unless the package layout is changed to include pkg/apis/... under the Python package root (or the code is regenerated with matching import paths), importing these stubs will fail at runtime.
| from pkg.apis.common.v2 import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | |
| from pkg.apis.dfdaemon.v2 import dfdaemon_pb2 as pkg_dot_apis_dot_dfdaemon_dot_v2_dot_dfdaemon__pb2 | |
| from . import common_pb2 as pkg_dot_apis_dot_common_dot_v2_dot_common__pb2 | |
| from . import dfdaemon_pb2 as pkg_dot_apis_dot_dfdaemon_dot_v2_dot_dfdaemon__pb2 |
| def ListSeedPeers(self, request, context): | ||
| """List acitve schedulers configuration. | ||
| """ | ||
| context.set_code(grpc.StatusCode.UNIMPLEMENTED) |
There was a problem hiding this comment.
The ListSeedPeers method docstring says "List acitve schedulers configuration", which is both misspelled and describes the wrong resource (seed peers vs schedulers). Since this is generated code, the fix likely needs to be applied in the source .proto comments and then the stubs regenerated.
| raise NotImplementedError('Method not implemented!') | ||
|
|
||
| def UpdatePersistentTask(self, request, context): | ||
| """UpdatePersistentTask updates metadate of thr persistent task in p2p network. |
There was a problem hiding this comment.
Typos in docstring: "metadate of thr" should be "metadata of the".
| """UpdatePersistentTask updates metadate of thr persistent task in p2p network. | |
| """UpdatePersistentTask updates metadata of the persistent task in p2p network. |
| def UpdatePersistentCacheTask(self, request, context): | ||
| """UpdatePersistentCacheTask updates metadate of thr persistent cache task in p2p network. | ||
| """ |
There was a problem hiding this comment.
Typos in docstring: "metadate of thr" should be "metadata of the".
|
The files were autogenerated @gaius-qi , should I address all of Copilot's comments? |
This PR adds codegen Python gRPC bindings for Dragonfly APIs
Description
Related Issue
#4421
Motivation and Context
This API dependency package with generated Python gRPC code based on the existing .protoc files serves as step 1 for the Python SDK design