Add: docstrings for the remote-buffer API and Worker lifecycle - #1542
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:
📝 WalkthroughWalkthroughDocumentation-only updates expand public API docstrings for orchestration scopes, remote task interfaces, worker memory operations, worker registration, and lifecycle methods. No logic or public signatures changed. ChangesPublic API documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/task_interface.py`:
- Around line 187-194: Update the RemoteTensorRef documentation to describe
HOST_INLINE handles created by RemoteTensorRef.host_inline(): they contain
inline payload bytes, have no remote allocation, are not released through either
Worker release API, and use nbytes for payload size. Add this distinction
wherever the owner/imported handle behavior is described, including the
referenced additional documentation.
In `@python/simpler/worker.py`:
- Around line 2407-2413: Update the remote worker registration docstring to
state that endpoint hosts must be numeric IPv4 addresses or localhost, matching
the validation performed by _parse_remote_endpoint(). Do not advertise general
numeric IP or IPv6 support unless that parser is explicitly extended.
- Around line 6272-6288: Update the close() docstring to state that any
incomplete accepted-run drain, including interruption, leaves teardown
unattempted and permits retry; do not limit the retryable case to drain
timeouts. Preserve the existing documentation for completed teardown and other
close() behaviors.
🪄 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: CHILL
Plan: Pro Plus
Run ID: abec49ea-d5da-48fb-9a7b-cfa50b0da3a7
📒 Files selected for processing (3)
python/simpler/orchestrator.pypython/simpler/task_interface.pypython/simpler/worker.py
3907057 to
39b9e3f
Compare
The user-facing Python reference added in hw-native-sys#1526 is hand-maintained and says the source is authoritative when the two disagree. That only holds if the source actually documents itself; docstring coverage on the public API was 54% in worker.py and 57% in task_interface.py, concentrated in exactly the surface a reader is least able to infer — the L4 remote-buffer calls and the terminal close() contract. Public non-nested symbols in worker.py, task_interface.py and orchestrator.py now all carry docstrings. Each states behaviour read out of the implementation rather than restating the signature: - Worker.remote_free and remote_release_import are idempotent, reject the opposite handle kind, and defer while a live task slot or an outstanding import still references the buffer, instead of issuing the release now. - Worker.remote_export can only narrow access, never widen it, and nbytes=None means "to the end of the buffer". - Worker.remote_import rejects an export minted by a different Worker and one whose owner buffer is already freed. - Worker.add_remote_worker must precede init(), requires level >= 4, and validates the endpoint before any process is forked. Its host must be a numeric IPv4 address or localhost: hostname resolution is unbounded and so unsupported, and an IPv6 literal is rejected earlier still, because the endpoint is parsed as one host:port pair and fails on its colon count. - RemoteBufferHandle distinguishes owner from imported handles, which are not interchangeable, and the HOST_INLINE form that RemoteTensorRef.host_inline mints — that one names no allocation, so neither release call applies to it and its nbytes describes the payload rather than an allocation. - RemoteAddressSpace records that REMOTE_WINDOW and UB_LDST are protocol placeholders today. Worker.close() had its contract in a comment block rather than a docstring, so the reentrancy, init-guard, single-shot-teardown and owner-thread rules were invisible to help() and to any generated reference. The docstring distils that contract; the linearization comments explaining the code below it stay where they are. Those comments also under-described the contract, and are corrected so the file does not contradict its own docstring: they called a drain timeout "the one retry path", but an asynchronous interruption that leaves an accepted run fence undrained equally leaves teardown un-attempted and the tree intact. Both paths are now named, in the docstring and in the two comments. Eight symbols in worker.py remain undocumented on purpose: they are handle_task / handle_control / dispatch closures nested inside private loop functions, not API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai All three inline findings addressed and resolved. Each was a real accuracy defect in my docstrings, not a style nit:
One disclosure: the third fix also required correcting two pre-existing comments in Docstring coverage is unchanged at 36/36, 49/49 and 17/17 for public non-nested symbols. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
worker_file=$(fd -a '^worker\.py$' | grep -E '(^|/)python/simpler/worker\.py$' | head -n 1)
task_file=$(fd -a '^task_interface\.py$' | grep -E '(^|/)python/simpler/task_interface\.py$' | head -n 1)
printf '%s\n' "== Files =="
printf 'worker: %s\ntask interface: %s\n' "$worker_file" "$task_file"
printf '%s\n' "== Symbol outline =="
ast-grep outline "$worker_file" --items all --match 'Worker|_parse_remote_endpoint|_validate_numeric_endpoint_host' --view expanded
ast-grep outline "$task_file" --items all --match 'RemoteBufferHandle|RemoteTensorRef' --view expanded
printf '%s\n' "== Relevant definitions and documentation =="
rg -n -A22 -B5 \
'class RemoteBufferHandle\b|class RemoteTensorRef\b|def host_inline\b|def _validate_numeric_endpoint_host\b|def _parse_remote_endpoint\b|def close\b|retry path|retryable|lease budget|drain timeout|HOST_INLINE|IPv4|localhost' \
"$worker_file" "$task_file"
printf '%s\n' "== Worker.close async interruption range =="
sed -n '6280,6360p' "$worker_file"Length of output: 77973
🐇✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
docs/user/reference/python-api.md(from #1526) is hand-maintained and states that the source is authoritative when the two disagree. That promise only holds if the source documents itself — and coverage on the public API was 54% inworker.py, 57% intask_interface.py, concentrated in exactly the surface a reader is least able to infer: the L4 remote-buffer calls and the terminalclose()contract.Public non-nested symbols in
worker.py,task_interface.pyandorchestrator.pyare now 100% documented.worker.pytask_interface.pyorchestrator.pyEach docstring states read-from-the-implementation behaviour
Not a restatement of the signature. The ones a caller cannot guess:
remote_free/remote_release_importare idempotent, reject the opposite handle kind, and defer while a live task slot or an outstanding import still references the buffer — they do not issue the release immediately (_pending_remote_buffer_frees).remote_exportcan only ever narrow access, never widen it (if flags & ~handle.access_flags: raise), andnbytes=Nonemeans "to the end of the buffer".remote_importrejects an export minted by a differentWorker, and one whose owner buffer is already freed.add_remote_workermust precedeinit(), requireslevel >= 4, and validates the endpoint before any process is forked — its host must be a numeric IP because hostname resolution is unbounded and uncancellable.RemoteBufferHandledistinguishes owner from imported handles (not interchangeable) and is constructible only throughWorker(token-guarded).RemoteAddressSpacerecords thatREMOTE_WINDOW/UB_LDSTare protocol placeholders today.Worker.close()is the notable one: its contract already existed as a 26-line comment block, so the reentrancy, init-guard, single-shot-teardown and owner-thread rules were invisible tohelp()and to any generated reference. The docstring distils that contract; the linearization comments that explain the code below it stay where they are. I did not invent new prose for it.Eight symbols left undocumented on purpose
worker.pyreports 36/44 if nested functions are counted. The eight arehandle_task/handle_control/dispatchclosures inside private loop functions (_sub_worker_loop,_run_chip_main_loop,_child_worker_loop,_dispatch_control_domain) — internal, not API. The 54% figure was partly a measurement artifact for the same reason.Why this matters beyond coverage
It is the prerequisite for replacing the hand-written reference with a generated one. With coverage at 54% a generator would have emitted a page full of holes; that was the stated reason for hand-writing it in #1526.
Verification
ruff check python/simpler/andruff format --checkcleantest_package_surface.pystill 5 passed / 1 skippedtask_interface.py:235), the narrow-only access check (worker.py:2901), and the before-init()guard (worker.py:2419)RemoteBufferExport's wording ontoRemoteBufferHandle's same-named properties; the final insertion is AST-position driven, and same-named properties on the two classes were re-checked individually for correct attribution and no duplicates