Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors protobuf stub generation into a new smg_grpc_proto/_proto_build.py, updates setup.py to delegate stub building (with optional PEP 660 editable-wheel hook), makes version tolerant of missing package metadata, and adds tests and pytest hooks validating proto discovery and generated stubs. Changes
Sequence DiagramsequenceDiagram
participant User as Installer
participant Setup as setup.py
participant Editable as EditableWheelWithProto
participant ProtoBuilder as smg_grpc_proto/_proto_build.py
participant FS as FileSystem
participant Protoc as grpc_tools.protoc
User->>Setup: install / editable install
Setup->>ProtoBuilder: _load_proto_build_helper() -> ensure_generated_stubs(..., force=True)
alt editable-wheel available
Setup->>Editable: register hook
Editable->>ProtoBuilder: ensure_generated_stubs(..., force=True)
end
ProtoBuilder->>FS: resolve proto sources (repo or packaged proto)
FS-->>ProtoBuilder: proto file paths
ProtoBuilder->>FS: acquire .proto-build.lock
ProtoBuilder->>FS: sync proto files into package proto/
ProtoBuilder->>Protoc: invoke protoc.main(args)
Protoc->>FS: write *_pb2*.py / *_pb2*.pyi
ProtoBuilder->>FS: post-process imports/mypy headers, swap generated tree atomically
ProtoBuilder-->>Setup: stubs ensured
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the gRPC proto compilation logic into a dedicated helper module and integrates it into the Python setup process, including support for PEP 660 editable installs. It also introduces a test suite to verify the integrity of the generated proto stubs and a bootstrap mechanism in conftest.py to ensure stubs are available during test execution. Feedback suggests addressing potential race conditions in the test bootstrap logic when running parallel tests and implementing a mechanism to detect and recompile out-of-date stubs based on source file timestamps.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea82487267
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ea82487 to
942b299
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 942b299ba8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grpc_client/python/smg_grpc_proto/_proto_build.py`:
- Around line 55-81: The current code calls _clear_generated_stubs(output_dir)
before importing grpc_tools and running protoc, which can leave the package
without stubs on import errors or failed compilation; instead generate into a
temporary directory and only replace the existing output_dir on success: change
the flow in _proto_build.py to (1) do NOT call
_clear_generated_stubs(output_dir) up-front, (2) create a temp dir (e.g.,
temp_output or tempfile.mkdtemp) and set the protoc args'
--python_out/--grpc_python_out/--pyi_out to that temp dir, (3) run
protoc.main(args) and check result, (4) on success atomically swap/replace the
old output_dir with the temp dir (or clear old with
_clear_generated_stubs(output_dir) then move temp into place), and (5) ensure
cleanup of the temp dir on failure; reference symbols: _clear_generated_stubs,
output_dir, proto_files, protoc.main, grpc_tools import.
In `@grpc_servicer/tests/conftest.py`:
- Around line 60-67: The collection hook currently silences bootstrap errors by
adding a skip when _PROTO_SYMBOL_SKIP_REASON is set; instead, detect collected
"test_proto_symbols.py" entries in pytest_collection_modifyitems and call
pytest.fail(...) with the _PROTO_SYMBOL_SKIP_REASON (or otherwise raise a
collection failure) so the test suite reports a failure rather than skipping;
update the logic in pytest_collection_modifyitems (referencing the function name
pytest_collection_modifyitems, the variable _PROTO_SYMBOL_SKIP_REASON, and the
test file name "test_proto_symbols.py") to surface the bootstrap error during
collection.
- Around line 15-19: The current guards using find_spec("smg_grpc_proto") and
find_spec("smg_grpc_servicer") allow an installed distribution to shadow the
local checkout; change the logic in conftest.py so the local source directories
are always prepended to sys.path (use sys.path.insert(0, str(_PROTO_SRC)) and
sys.path.insert(0, str(_SERVICER_SRC)) unconditionally instead of conditional on
find_spec) so imports prefer the checkout; update the same pattern referenced
around the other block (lines 44-49) to match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2b500fd-e396-4ad6-92c9-8fb31f0b9afa
📒 Files selected for processing (5)
crates/grpc_client/python/setup.pycrates/grpc_client/python/smg_grpc_proto/__init__.pycrates/grpc_client/python/smg_grpc_proto/_proto_build.pygrpc_servicer/tests/conftest.pygrpc_servicer/tests/test_proto_symbols.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/grpc_client/python/smg_grpc_proto/_proto_build.py (1)
139-149:⚠️ Potential issue | 🟠 MajorDon't destroy existing stubs before verifying
grpc_toolsavailability.The current flow clears
smg_grpc_proto/generatedbefore attempting to importgrpc_tools. If the import fails (missing dependency), the package is left without importable stubs. Move the cleanup after the import succeeds.🔧 Suggested fix
- _clear_generated_stubs(output_dir) - output_dir.mkdir(parents=True, exist_ok=True) - (output_dir / "__init__.py").write_text('"""Auto-generated protobuf stubs. Do not edit."""\n') - try: import grpc_tools from grpc_tools import protoc except ImportError as exc: raise RuntimeError( "grpcio-tools not installed. Install with: pip install grpcio-tools" ) from exc + + _clear_generated_stubs(output_dir) + output_dir.mkdir(parents=True, exist_ok=True) + (output_dir / "__init__.py").write_text('"""Auto-generated protobuf stubs. Do not edit."""\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grpc_client/python/smg_grpc_proto/_proto_build.py` around lines 139 - 149, The cleanup currently runs before verifying grpc_tools; move the call to _clear_generated_stubs so it happens only after the import succeeds: first attempt the try/except import of grpc_tools and protoc (the block referencing grpc_tools, protoc, and ImportError exc), and only after successful import call _clear_generated_stubs(output_dir), then create output_dir and write __init__.py as currently done; ensure you keep the same error message raised in the except branch when grpcio-tools is missing.grpc_servicer/tests/conftest.py (2)
15-19:⚠️ Potential issue | 🟠 MajorAlways prefer the local checkout over any installed distribution.
The
find_spec()guards allow a preinstalledsmg_grpc_protoorsmg_grpc_servicerto shadow the source-tree version. For source-tree testing, the local checkout should always take precedence to ensure tests run against the current code.🔧 Suggested fix
-if find_spec("smg_grpc_proto") is None and str(_PROTO_SRC) not in sys.path: +if str(_PROTO_SRC) not in sys.path: sys.path.insert(0, str(_PROTO_SRC)) -if find_spec("smg_grpc_servicer") is None and str(_SERVICER_SRC) not in sys.path: +if str(_SERVICER_SRC) not in sys.path: sys.path.insert(0, str(_SERVICER_SRC))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_servicer/tests/conftest.py` around lines 15 - 19, Remove the find_spec(...) checks so the local checkout always takes precedence: in conftest.py ensure you unconditionally insert str(_PROTO_SRC) and str(_SERVICER_SRC) at the front of sys.path (keeping the existing check that avoids duplicate insertion if the exact path string is already present), i.e. replace the blocks that currently check find_spec("smg_grpc_proto") and find_spec("smg_grpc_servicer") with direct sys.path.insert(0, str(_PROTO_SRC)) / sys.path.insert(0, str(_SERVICER_SRC)) (or the existing duplicate-avoidance variant) so the source-tree versions are preferred over any installed distributions.
54-61:⚠️ Potential issue | 🟠 MajorFail
test_proto_symbols.pywhen bootstrap breaks instead of silently skipping.Marking this file as skipped turns a broken proto bootstrap path into a green test run. Since this is the only coverage for generated symbols and bindings, a bootstrap failure should be surfaced as a test failure.
🔧 Suggested fix
def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None: if _PROTO_SYMBOL_SKIP_REASON is None: return - skip = pytest.mark.skip(reason=_PROTO_SYMBOL_SKIP_REASON) - for item in items: - if "test_proto_symbols.py" in item.nodeid: - item.add_marker(skip) + if any("test_proto_symbols.py" in item.nodeid for item in items): + raise pytest.UsageError(_PROTO_SYMBOL_SKIP_REASON)Based on learnings, the team prefers explicit failures over silent skips to catch misconfigurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_servicer/tests/conftest.py` around lines 54 - 61, Currently pytest_collection_modifyitems uses _PROTO_SYMBOL_SKIP_REASON to skip test_proto_symbols.py; instead, when _PROTO_SYMBOL_SKIP_REASON is set we should fail the run so bootstrap problems are surfaced. Modify pytest_collection_modifyitems to, if _PROTO_SYMBOL_SKIP_REASON is not None and any item.nodeid contains "test_proto_symbols.py", call pytest.fail (or otherwise raise a collection-time failure) with a clear message including _PROTO_SYMBOL_SKIP_REASON rather than adding a skip marker; keep the check for "test_proto_symbols.py" and use the same items loop to detect affected tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grpc_servicer/tests/test_proto_symbols.py`:
- Around line 54-104: The test test_kv_event_batch_serialization_round_trip
omits verifying the timestamp field after round-trip; update the test (inside
test_kv_event_batch_serialization_round_trip) to assert that decoded.timestamp
equals the original value (1_234_567_890.5) — e.g., add an assertion like assert
decoded.timestamp == 1_234_567_890.5 (or use an approximate comparison if you
prefer float tolerance) near the other decoded field assertions after parsing
the wire into decoded.
---
Duplicate comments:
In `@crates/grpc_client/python/smg_grpc_proto/_proto_build.py`:
- Around line 139-149: The cleanup currently runs before verifying grpc_tools;
move the call to _clear_generated_stubs so it happens only after the import
succeeds: first attempt the try/except import of grpc_tools and protoc (the
block referencing grpc_tools, protoc, and ImportError exc), and only after
successful import call _clear_generated_stubs(output_dir), then create
output_dir and write __init__.py as currently done; ensure you keep the same
error message raised in the except branch when grpcio-tools is missing.
In `@grpc_servicer/tests/conftest.py`:
- Around line 15-19: Remove the find_spec(...) checks so the local checkout
always takes precedence: in conftest.py ensure you unconditionally insert
str(_PROTO_SRC) and str(_SERVICER_SRC) at the front of sys.path (keeping the
existing check that avoids duplicate insertion if the exact path string is
already present), i.e. replace the blocks that currently check
find_spec("smg_grpc_proto") and find_spec("smg_grpc_servicer") with direct
sys.path.insert(0, str(_PROTO_SRC)) / sys.path.insert(0, str(_SERVICER_SRC)) (or
the existing duplicate-avoidance variant) so the source-tree versions are
preferred over any installed distributions.
- Around line 54-61: Currently pytest_collection_modifyitems uses
_PROTO_SYMBOL_SKIP_REASON to skip test_proto_symbols.py; instead, when
_PROTO_SYMBOL_SKIP_REASON is set we should fail the run so bootstrap problems
are surfaced. Modify pytest_collection_modifyitems to, if
_PROTO_SYMBOL_SKIP_REASON is not None and any item.nodeid contains
"test_proto_symbols.py", call pytest.fail (or otherwise raise a collection-time
failure) with a clear message including _PROTO_SYMBOL_SKIP_REASON rather than
adding a skip marker; keep the check for "test_proto_symbols.py" and use the
same items loop to detect affected tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49182d3b-e027-4849-8eae-f9b96644a792
📒 Files selected for processing (6)
crates/grpc_client/python/setup.pycrates/grpc_client/python/smg_grpc_proto/__init__.pycrates/grpc_client/python/smg_grpc_proto/_proto_build.pycrates/grpc_client/python/tests/test_proto_build.pygrpc_servicer/tests/conftest.pygrpc_servicer/tests/test_proto_symbols.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 396637c3cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
396637c to
6af18b0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6af18b0f35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
6af18b0 to
28a8f3a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28a8f3a9e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grpc_client/python/smg_grpc_proto/_proto_build.py`:
- Around line 149-161: The _proto_compile_lock context manager uses Unix-only
fcntl.flock (see lock_path and fcntl.flock calls) which contradicts the
"Operating System :: OS Independent" classifier; either replace the locking with
a cross-platform solution (e.g., use the filelock package and acquire/release a
FileLock around lock_path in _proto_compile_lock) or update pyproject.toml to a
POSIX/Unix OS classifier; make the change so the implementation and package
metadata remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a1ce65d-417c-4c3e-83e3-5ca28a4d8739
📒 Files selected for processing (4)
crates/grpc_client/python/smg_grpc_proto/_proto_build.pycrates/grpc_client/python/tests/test_proto_build.pygrpc_servicer/tests/conftest.pygrpc_servicer/tests/test_proto_symbols.py
28a8f3a to
7f4e258
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grpc_client/python/smg_grpc_proto/_proto_build.py`:
- Around line 33-35: The current logic discards partial overrides when either
source_proto_dir or source_proto_files is None; instead normalize them so a
single provided value is honored: if source_proto_dir is provided but
source_proto_files is None, enumerate *.proto files under source_proto_dir
(e.g., via pathlib.glob) and set source_proto_files to that list; if
source_proto_files is provided but source_proto_dir is None, compute the common
parent directory (e.g., pathlib.Path(...).parent or commonpath) and set
source_proto_dir to that directory; update the same logic at both the initial
check around source_proto_dir/source_proto_files and the later block at lines
171-173 so resolve_proto_sources(package_dir) is only used when both are truly
unset.
- Around line 67-82: The generated_stubs_are_current function currently only
ensures expected outputs exist and are newer than protos but ignores leftover
generated files; update generated_stubs_are_current to also list actual files in
the output_dir (package_dir / "smg_grpc_proto" / "generated") and compare that
set against expected_generated_stub_paths(output_dir, source_proto_files),
returning False if any extra files are present (so remove/rename of .proto
triggers regeneration). Keep the existing checks (existence and mtimes) but add
this extras check after computing expected_paths and before returning True; use
resolve_proto_sources when source_proto_files is None to derive expected_paths
as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a45fb76a-ec94-4460-a76d-accf8f76baa9
📒 Files selected for processing (5)
crates/grpc_client/python/pyproject.tomlcrates/grpc_client/python/smg_grpc_proto/_proto_build.pycrates/grpc_client/python/tests/test_proto_build.pygrpc_servicer/tests/conftest.pygrpc_servicer/tests/test_proto_symbols.py
7f4e258 to
7ad0fd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grpc_client/python/smg_grpc_proto/_proto_build.py`:
- Around line 51-61: The cleanup loop currently unlinks every .proto under
proto_dir which can delete the repo's checked-in sources when proto_dir is a
symlink; change the logic so you never call existing.unlink() when
proto_dir.is_symlink() is true: add a guard before the for existing in
proto_dir.glob("*.proto") loop (or inside it) to skip deletions if
proto_dir.is_symlink(), and ensure you still copy/overwrite files from
source_proto_files using shutil.copy2(target) so overrides are applied without
removing the real source files; reference proto_dir, source_proto_files,
existing.unlink(), and shutil.copy2 in your changes.
- Around line 142-145: The .pyi stub handling loop currently only prepends
mypy_header and skips the import normalization applied to .py files; update the
pyi_file processing (the for pyi_file in output_dir.glob("*_pb2*.pyi") block) to
perform the same import normalization as the .py handling: read content into the
content variable, apply the same rewrite that converts unqualified imports like
"import common_pb2" to relative imports (the same regex/logic used for the .py
files), then prepend mypy_header (or ensure the normalized content includes the
header) and write back with pyi_file.write_text; reuse the identical
transformation code so .pyi stubs resolve the package-relative imports the same
way as for .py files.
In `@crates/grpc_client/python/tests/test_proto_build.py`:
- Around line 8-10: Tests may import an installed smg_grpc_proto instead of the
checkout because the code only inserts _PACKAGE_ROOT into sys.path if it's
absent; change this to force the checkout path to index 0: unconditionally
remove any existing occurrences of str(_PACKAGE_ROOT) from sys.path (if present)
and then sys.path.insert(0, str(_PACKAGE_ROOT)) so the local package takes
precedence (update the snippet that uses _PACKAGE_ROOT and sys.path.insert to
always reinsert at index 0, mirroring the approach in
grpc_servicer/tests/conftest.py).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87d8af6d-2281-4687-8dd5-f98364acc651
📒 Files selected for processing (5)
crates/grpc_client/python/pyproject.tomlcrates/grpc_client/python/smg_grpc_proto/_proto_build.pycrates/grpc_client/python/tests/test_proto_build.pygrpc_servicer/tests/conftest.pygrpc_servicer/tests/test_proto_symbols.py
7ad0fd5 to
a10cee7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffb9aa5383
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ffb9aa5 to
01694f4
Compare
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
github-actions
Bot
added
stale
Signed-off-by: Sydney Firmin <sydney.firmin@oracle.com>
607e454 to
8d70679
Compare
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0af534e9f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| synced_proto_files = [] | ||
| for proto_file in source_proto_files: | ||
| target = proto_dir / proto_file.name | ||
| shutil.copy2(proto_file, target) |
There was a problem hiding this comment.
When source_proto_dir is an override and smg_grpc_proto/proto is the normal source-tree symlink to crates/grpc_client/proto, this copy2 writes through the symlink into the checked-in proto directory. If the override contains a same-named file such as common.proto, it overwrites the repository proto before generation; if it contains new names, it leaves extra protos behind for later builds. The override path should be staged somewhere that cannot mutate the symlink target, or the symlink should be handled explicitly before copying.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
1 participant
Description
This is PR 1 of the stacked
vllm-kv-eventsseries.It is intentionally limited to Python gRPC proto packaging and source-tree test bootstrap. It does not include the vLLM servicer changes, model-gateway KV event consumption, smoke tooling, or benchmark coverage.
Problem
smg-grpc-protowas brittle when used from a source checkout or editable install:setup.pysmg_grpc_protofrom a local checkout could fail before package metadata existedThose issues make it harder to iterate on the gRPC/proto surface and to add reviewable proto-level tests ahead of the larger KV event rollout.
Solution
This PR extracts proto build logic into a package-local helper, reuses that helper from
setup.py, and adds a lightweight source-tree bootstrap path for tests. It also adds focused proto symbol tests that validate the generated KV event message and RPC surface without requiring vLLM, torch, or a GPU.Changes
smg_grpc_proto/_proto_build.pysetup.pyto invoke the shared helper forbuild_py,develop, and PEP 660editable_wheelsmg_grpc_proto.__version__tolerate source-tree imports by falling back to0+localwhen package metadata is unavailablegrpc_servicer/tests/conftest.pybootstrap logic so source-tree pytest runs can generate local proto stubs when neededgrpc_servicer/tests/test_proto_symbols.pyto validate:KvEventBatchSubscribeKvEventsRequest.start_sequence_numberSubscribeKvEventsTest Plan
pytest grpc_servicer/tests/test_proto_symbols.py -qpython3 -m py_compile grpc_servicer/tests/conftest.py crates/grpc_client/python/smg_grpc_proto/_proto_build.py crates/grpc_client/python/setup.pygit diff --checkChecklist
Stack Context
Review order:
sfirmin/vllm-kv-events-01-grpc-protosfirmin/vllm-kv-events-02-grpc-servicersfirmin/vllm-kv-events-03-model-gatewaysfirmin/vllm-kv-events-04-docs-smokesfirmin/vllm-kv-events-05-benchmarksSummary by CodeRabbit
Refactoring
New Features
Bug Fixes
Tests
Chores