feat: expose committed_device_memory on DistributedWorker - #2209
feat: expose committed_device_memory on DistributedWorker#2209sunghajung6688 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthrough
ChangesDistributed memory API
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![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: c6d4b8991d
ℹ️ 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".
| self._require_open("committed_device_memory") | ||
| if self._w is None: | ||
| return 0 | ||
| return int(self._w.committed_device_memory(worker_id)) |
There was a problem hiding this comment.
When this new API is called with the repository-managed/runtime-pinned simpler checkout, self._w is still the current simpler.worker.Worker from the unchanged runtime gitlink, which does not expose committed_device_memory yet; callers therefore get an AttributeError instead of the memory counter. Please update the pinned/runtime dependency to a simpler revision containing that method, or guard this path with a clear version/runtime error so the PyPTO API is not advertised while its backend is absent.
Useful? React with 👍 / 👎.
46f3ce4 to
8b94e80
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/pypto/runtime/distributed_runner.py (1)
1627-1643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the new public API.
Cover normal per-worker forwarding, integer conversion, closed-worker behavior, absent workers, and the missing-runtime-method error. Place tests under
tests/, since this API affects memory accounting used for KV-cache sizing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/pypto/runtime/distributed_runner.py` around lines 1627 - 1643, Add focused tests under tests/ for DistributedRunner.committed_device_memory covering forwarding the requested worker_id and converting the result to int, returning the closed-worker behavior, handling absent workers, and raising the expected RuntimeError when the underlying runtime lacks committed_device_memory. Use the existing runner and mock/fake runtime test utilities and preserve the documented error message expectations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/pypto/runtime/distributed_runner.py`:
- Around line 1627-1643: Add focused tests under tests/ for
DistributedRunner.committed_device_memory covering forwarding the requested
worker_id and converting the result to int, returning the closed-worker
behavior, handling absent workers, and raising the expected RuntimeError when
the underlying runtime lacks committed_device_memory. Use the existing runner
and mock/fake runtime test utilities and preserve the documented error message
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 14f974a0-3afe-4493-83ea-4f9399d6871e
📒 Files selected for processing (2)
python/pypto/runtime/distributed_runner.pyruntime
Delegates to the underlying simpler Worker(level=3) facade (self._w.committed_device_memory), which forwards a CTRL_COMMITTED_DEVICE_MEMORY query to the forked chip child and sums across chips.
1 participant
Delegates to the underlying simpler Worker(level=3) facade (self._w.committed_device_memory), which forwards a CTRL_COMMITTED_DEVICE_MEMORY query to the forked chip child and sums across chips.
Closes hw-native-sys/simpler#1457