docs: flush subscribers before deregister in package README examples#201
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (17)**/*.{md,rst,html,txt}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Files:
**/*.{md,rst,html}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-brand-terminology.md)
Files:
**/*.md📄 CodeRabbit inference engine (.agents/skills/contribute-integration/SKILL.md)
Files:
**/{docs,examples,**/*.md,*.patch,*.diff,.github,*.sh,*.yaml,*.yml}📄 CodeRabbit inference engine (.agents/skills/rename-surfaces/SKILL.md)
Files:
**/*.{md,rst,txt}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Files:
**/*.{md,rst}📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
Files:
{docs/**,README.md,CONTRIBUTING.md,**/*.md}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
{docs/**,README.md,**/Cargo.toml,**/package.json,**/*.md}📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
Files:
**/*.{md,mdx,py,sh,yaml,yml,toml,json}📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md)
Files:
**/*.{html,md,mdx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/README.md📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{rs,py,js,ts,tsx,jsx,go,sh,toml,yaml,yml,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
python/nemo_relay/**/*⚙️ CodeRabbit configuration file
Files:
{crates/adaptive/**,python/nemo_relay/plugin.py,go/nemo_relay/**,**/node/**,**/wasm/**}📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Files:
go/nemo_relay/**/*⚙️ CodeRabbit configuration file
Files:
{crates/adaptive/**,python/nemo_relay/adaptive.py,python/nemo_relay/plugin.py,go/nemo_relay/adaptive/**,go/nemo_relay/!(adaptive)/**,**/node/**,**/wasm/**}📄 CodeRabbit inference engine (.agents/skills/maintain-optimizer/SKILL.md)
Files:
crates/{python,ffi,node,wasm}/**/*⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (7)
WalkthroughREADME examples for Python, Node, WASM, and Go were updated to call the appropriate subscriber flush helper before deregistration; Python’s first example also adds explanatory text about asynchronous native subscriber delivery. ChangesGetting Started example updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XS
|
I'm sorry about putting this PR ready and then immediately removing this PR's ready status... |
The package READMEs' "Getting Started" examples register a subscriber, emit events, then deregister without calling flush. Native subscriber delivery is non-blocking (a background dispatcher), so examples that exit right after deregistering can lose queued events: - Python (`python/nemo_relay/README.md`): printed nothing, or intermittently a single partial line; the `to_dict()` / `to_json()` callback never ran. - Go (`go/nemo_relay/README.md`): flaky -- dropped the trailing tool-end and scope-end events on exit (verified: 2 of 3 runs were partial). Node and WASM are not currently broken (Node keeps the event loop alive via the ref'd ThreadsafeFunction until delivery; WASM's flush is a single-threaded no-op), but their READMEs are updated for parity so every package README models the same flush-before-deregister pattern the quickstarts already use. - Python / Node / WASM: call `flushSubscribers()` (`subscribers.flush()` in Python) before `deregister(...)`. - Go: `defer nemo.FlushSubscribers()`, ordered so it runs after the deferred `scope.Pop` emits the scope-end event and before the deferred deregister. Verified by running each example: with the flush, all events are delivered deterministically (Go 5/5, Python and Node 3/3 across repeated runs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
zhongxuanwang-nv
force-pushed
the
docs/readme-subscriber-flush
branch
from
8ce4a66 to
5458193
Compare
zhongxuanwang-nv
changed the title
|
/merge |
…VIDIA#201) #### Overview The package READMEs' "Getting Started" examples register a subscriber, emit events, then deregister without calling flush. Native subscriber delivery is non-blocking, so an example that exits right after deregistering can lose queued events. This updates all four package READMEs to flush before deregister, matching the quickstarts (which already flush). - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details **Functional fixes** (verified by running — the example dropped events without flush): - `python/nemo_relay/README.md`: printed nothing, or intermittently a single partial line; the `to_dict()` / `to_json()` callback never ran. Adds `nemo_relay.subscribers.flush()` before each `deregister(...)`. - `go/nemo_relay/README.md`: flaky — dropped the trailing tool-end and scope-end events on exit (2 of 3 runs partial). Adds `defer nemo.FlushSubscribers()`, ordered to run after the deferred `scope.Pop` emits the scope-end event and before the deferred deregister. **Parity** (not currently broken; updated so every README models the same pattern): - `crates/node/README.md`: Node keeps the event loop alive via the ref'd ThreadsafeFunction, so it already delivers; adds `flushSubscribers()` for consistency. - `crates/wasm/README.md`: WASM's `flushSubscribers()` is a single-threaded no-op (tested as such); adds it for consistency. #### Where should the reviewer start? The four README diffs — each adds a flush before `deregister(...)` (Node/WASM also import `flushSubscribers`). Verified by running each example with the flush added: events deliver deterministically (Go 5/5, Python and Node 3/3 across repeated runs). WASM's `flushSubscribers` is an exported no-op (`crates/wasm/src/api/mod.rs`; covered by `crates/wasm/tests-js/scope_tests.mjs`). Note: the `crates/node/README.md` example also depends on the Node binding change in NVIDIA#196 (pass a real `ScopeHandle` to `withScope` callbacks). That is orthogonal to this flush change — NVIDIA#196 is code-only and does not touch the README. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to: NVIDIA#196 (Node `withScope` ScopeHandle; orthogonal, touches the same Node README example) ## Summary by CodeRabbit * **Documentation** * Updated Getting Started examples across Python, Node.js, WebAssembly, and Go language bindings to reflect the proper sequence for flushing subscribers before deregistration. Authors: - Zhongxuan (Daniel) Wang (https://github.com/zhongxuanwang-nv) Approvers: - Will Killian (https://github.com/willkill07) URL: NVIDIA#201 Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
2 participants
Overview
The package READMEs' "Getting Started" examples register a subscriber, emit events, then deregister without calling flush. Native subscriber delivery is non-blocking, so an example that exits right after deregistering can lose queued events. This updates all four package READMEs to flush before deregister, matching the quickstarts (which already flush).
Details
Functional fixes (verified by running — the example dropped events without flush):
python/nemo_relay/README.md: printed nothing, or intermittently a single partial line; theto_dict()/to_json()callback never ran. Addsnemo_relay.subscribers.flush()before eachderegister(...).go/nemo_relay/README.md: flaky — dropped the trailing tool-end and scope-end events on exit (2 of 3 runs partial). Addsdefer nemo.FlushSubscribers(), ordered to run after the deferredscope.Popemits the scope-end event and before the deferred deregister.Parity (not currently broken; updated so every README models the same pattern):
crates/node/README.md: Node keeps the event loop alive via the ref'd ThreadsafeFunction, so it already delivers; addsflushSubscribers()for consistency.crates/wasm/README.md: WASM'sflushSubscribers()is a single-threaded no-op (tested as such); adds it for consistency.Where should the reviewer start?
The four README diffs — each adds a flush before
deregister(...)(Node/WASM also importflushSubscribers). Verified by running each example with the flush added: events deliver deterministically (Go 5/5, Python and Node 3/3 across repeated runs). WASM'sflushSubscribersis an exported no-op (crates/wasm/src/api/mod.rs; covered bycrates/wasm/tests-js/scope_tests.mjs).Note: the
crates/node/README.mdexample also depends on the Node binding change in #196 (pass a realScopeHandletowithScopecallbacks). That is orthogonal to this flush change — #196 is code-only and does not touch the README.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
withScopeScopeHandle; orthogonal, touches the same Node README example)Summary by CodeRabbit