feat(nvtx): add quent-nvtx-events — verbatim NVTX event vocabulary - #386
Conversation
📝 WalkthroughWalkthroughA new ChangesNVTX events crate
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integrations/nvtx/events/src/payload.rs (1)
138-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend
PayloadExtensionEventround-trip test to cover all three variants.Only
BinaryPayloadis exercised;SchemaRegisterandEnumRegister(both carryingVec<u8>descriptors) are untested, unlike the scalar payload test above which covers everyNvtxPayloadValuevariant individually.♻️ Suggested extension
#[test] fn deferred_payload_extension_vocabulary_round_trips() { - let event = PayloadExtensionEvent::BinaryPayload { - schema_id: 9, - bytes: vec![1, 2, 3, 4], - }; - let json = serde_json::to_string(&event).expect("serialize"); - let back: PayloadExtensionEvent = serde_json::from_str(&json).expect("deserialize"); - assert_eq!(back, event); + let events = [ + PayloadExtensionEvent::SchemaRegister { + domain: 1, + schema_id: 2, + descriptor: vec![9, 9], + }, + PayloadExtensionEvent::EnumRegister { + domain: 1, + enum_id: 3, + descriptor: vec![8, 8], + }, + PayloadExtensionEvent::BinaryPayload { + schema_id: 9, + bytes: vec![1, 2, 3, 4], + }, + ]; + for event in &events { + let json = serde_json::to_string(event).expect("serialize"); + let back: PayloadExtensionEvent = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(&back, event); + } }🤖 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 `@integrations/nvtx/events/src/payload.rs` around lines 138 - 147, Extend deferred_payload_extension_vocabulary_round_trips to serialize, deserialize, and assert equality for SchemaRegister and EnumRegister in addition to BinaryPayload, covering all three PayloadExtensionEvent variants and their Vec<u8> descriptors.
🤖 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 `@integrations/nvtx/events/src/payload.rs`:
- Around line 138-147: Extend deferred_payload_extension_vocabulary_round_trips
to serialize, deserialize, and assert equality for SchemaRegister and
EnumRegister in addition to BinaryPayload, covering all three
PayloadExtensionEvent variants and their Vec<u8> descriptors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: fab4c554-75ed-48b4-862b-108fd2bedb63
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (5)
Cargo.tomlintegrations/nvtx/events/Cargo.tomlintegrations/nvtx/events/src/attributes.rsintegrations/nvtx/events/src/lib.rsintegrations/nvtx/events/src/payload.rs
johanpel
left a comment
johanpel
left a comment
There was a problem hiding this comment.
Thanks for the PR, some comments:
| //! analyzer resolves registered strings, domains, and categories from the event | ||
| //! stream in a later phase. | ||
|
|
||
| use serde::{Deserialize, Serialize}; |
There was a problem hiding this comment.
We might want to feature gate serde support since not all exporters require it since #250 was solved
A Quent-agnostic, verbatim vocabulary of NVTX events: the `NvtxEvent` enum covering the core NVTX surface (push/pop + start/end ranges, marks, domain lifecycle, registered strings, category/thread naming, resource create/destroy), `NvtxMessage`/`NvtxColor`/`NvtxEventAttributes`, and the core `nvtxEventAttributes` payload union captured undecoded (`NvtxPayload`). Pure vocabulary: no Quent-internal dependencies, and `serde` is optional (behind a default-on `serde` feature), so the crate stays cleanly separable and can be offered upstream to the NVTX Rust crates later. Adapting these events into Quent's pipeline (entity naming, the event wrapper) is the bridge crate's concern, not this crate's. First of three stacked crate PRs for NVTX capture (events -> injection -> bridge). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
|
From a code perspective this looks fine to me but I think we need to reconsider adding it as a Quent crate and follow the suggestion here: #385 (comment) |
I think we should keep this and look into that in parallel. There is no need to block the follow tasks and pursue this which can be delayed with fork creation and setting up other things. Keeping this as a separate crate lets us remove this at later point quickly and switch over to the nvtx-sys upstream. |
I'm curious, with a fork we are not blocked, so why not do that now? |
If and when I do any changes to these crates, one would have to jump through couple of extra steps to achieve that. Instead if move/upstream them later, I can play with them quickly and upstream them once they are upstream ready. Potentially less work for me with the trade off being some extra bunch of lines in the repository temporarily. I am confident it won't stay in the same shape/form as it is today given how things change as we progress through next 4 phases listed as sub-issues in #76 |
johanpel
left a comment
johanpel
left a comment
There was a problem hiding this comment.
If and when I do any changes to these crates, one would have to jump through couple of extra steps to achieve that. Instead if move/upstream them later, I can play with them quickly and upstream them once they are upstream ready. Potentially less work for me with the trade off being some extra bunch of lines in the repository temporarily.
To make sure we are on the same page (perhaps this is clear already), the suggestion of @mbrobbel is to create a personal fork NVTX, add the Rust injection wrappers there, then use a Cargo git dependency.
You can iterate quickly on the fork without upstreaming anything to NVTX until you feel it's done. Only takes a bump of the git rev to synchronize changes. Less work in the end; when you're ready to upstream to NVTX, just open a PR from the fork, get it merged and update the git dep, no code migration needed. And you can leverage NVTX' existing build script to generate sys bindings.
Ultimately your choice, approving to unblock merging using the current approach if you prefer that.
That's true, it shifts the task to host this code temporally. However, I have to shift to different repository (locally of course) whenever I want edit the code from those crates. Yes, it is few simple steps, nevertheless some extra hops to achieve it compared to just editing the file in the sibling folder. Hence my hesitation to do it right away. Lets do it in place and handle the shifting part later. |
|
/merge |
|
Created ticket for upstreaming task #388 |
## `quent-nvtx-injection` — the NVTX injection cdylib PR 2 of 3, following #386 (`quent-nvtx-events`, merged). The bridge + capture e2e follows this one. ### What it is - The Quent-agnostic cdylib NVTX loads via `NVTX_INJECTION64_PATH`. Exports `InitializeInjectionNvtx2`, fills the CORE/CORE2 callback tables, and converts each callback into the `quent-nvtx-events` vocabulary **verbatim** (raw handles, no resolution/decoding). - Depends on nothing Quent-internal except `quent-nvtx-events`. Linux 64-bit only (`compile_error!` elsewhere). ### Safety & correctness - **Panic containment** — every `extern "C"` callback runs inside `catch_unwind`; a panic never crosses the C ABI. Synthesized handles/ids and push/pop levels are returned correctly even if conversion panics. - **Per-image init** — NVTX calls `InitializeInjectionNvtx2` once per NVTX-using image (exe + each instrumented `.so` has its own `nvtxGlobals`), so callbacks are installed on **every** call, not just the first — otherwise later images are silently no-op'd. - **Bounded reads** — attribute structs are read only up to their declared `size`; the payload union is read at the tagged member's exact width (no uninitialized upper bytes). - **String copy-in** — immediate `const char*` messages are copied into owned `String`s before returning; registered strings keep only their raw handle. - **Faithful push/pop** — range push/pop return NVTX's 0-based per-thread, per-domain nesting level. - **Bounds-checked table writes** — callbacks are written only into in-range, non-null NVTX slots. ### Generated bindings - `build.rs` runs `bindgen` over the NVTX ABI into `$OUT_DIR` on every build — nothing checked in, no `bindings.rs` to keep in sync (resource/range-id types folded into the allowlist; no hand-written `convert::abi`). - Headers + `libclang` come from the pixi-pinned `nvtx-c` / `libclang` packages (`[target.linux-64.dependencies]`), so `pixi run cargo …` is hermetic (`build.rs` reads `CONDA_PREFIX`). Bumping the captured NVTX version is a one-line `nvtx-c` bump — no regen feature, NVTX git dep, or `deny.toml` allow-git. ### Captured surface - **CORE2 (domain-scoped ASCII):** mark, range start/end/push/pop, domain / register-string / name-category / resource. - **CORE (classic default-domain ASCII):** `nvtxMarkA`/`Ex`, `nvtxRangePushA`/`Ex`, `nvtxRangePop`, `nvtxRangeStartA`/`Ex`/`nvtxRangeEnd`, `nvtxNameCategoryA`, `nvtxNameOsThreadA` — captured on the default domain (`0`). No vocabulary change: `quent-nvtx-events` already models the default domain as `domain 0`. - **Wide-char (`*W`/Unicode):** subscribed with warn-once stubs — Unicode capture deferred, but such a call emits a one-time diagnostic and keeps range nesting/ids valid instead of silently no-op'ing. ### Tests - 17 unit tests covering conversion and nesting logic (verbatim capture per kind, size-bounded reads, string copy-in, member-width payloads, per-thread/per-domain nesting, default-domain `*A` conversions). No NVTX headers or GPU required beyond the pixi-pinned `nvtx-c`. ### Upstreaming - The injection ABI allowlist / consumer surface is a candidate to contribute to `nvtx-sys` behind a `tools` feature (currently producer-only) — parallel track, not a blocker. Authors: - Pradeep Garigipati (https://github.com/9prady9) Approvers: - Johan Peltenburg (https://github.com/johanpel) URL: #391
PR 3 of 3, completing NVTX capture on top of rapidsai#386 (quent-nvtx-events) and rapidsai#391 (quent-nvtx-injection). `quent-nvtx` is the bridge that adapts each captured NvtxEvent into Quent's event pipeline and builds the self-configuring capture cdylib NVTX loads via NVTX_INJECTION64_PATH. - rlib: fronts Quent's unbounded EventSender with a bounded, lock-free ring (drop-and-count on overflow, surfaced at teardown); a drain thread forwards the ring to an exporter. The injection hook stamps a capture timestamp and enqueues without blocking the app thread. - cdylib: an ELF .init_array constructor reads QUENT_NVTX_OUTPUT_DIR (+ an optional QUENT_NVTX_SESSION), builds an ndjson pipeline, and installs the hook in the same image whose callbacks NVTX invokes; a .fini_array destructor flushes at exit. NvtxEventEntity is a transparent newtype over NvtxEvent that implements the pipeline's quent_events::EntityEvent (NAME = "NvtxEvent"); the vocabulary crate stays Quent-agnostic, so the newtype adapts it here. The e2e feature builds a deterministic, self-contained NVTX emitter (nvtx_test_app, no GPU) and a subprocess harness that attaches the cdylib and asserts every core NVTX kind, the CORE payload union, cross-thread range pairing, and per-thread naming round-trip through ndjson. NVTX client headers come from the pixi-pinned nvtx-c package ($CONDA_PREFIX/include), matching the injection crate; no NVTX git dep. Registered in workspace members only (not default-members), Linux 64-bit only. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
… e2e) PR 3 of 3, on rapidsai#386 (events) + rapidsai#391 (injection). Adapts each captured NvtxEvent into Quent's pipeline; the self-configuring cdylib NVTX loads via NVTX_INJECTION64_PATH. - Hook stamps each event and pushes to a bounded, lock-free ring (drop-and-count on overflow); a drain thread forwards to an ndjson exporter. - cdylib self-configures from QUENT_NVTX_OUTPUT_DIR at load, flushes at exit. - NvtxEventEntity: transparent newtype over NvtxEvent implementing EntityEvent. - e2e: one multi-threaded C++ producer (domain-scoped, mimics libcudf) run under the cdylib; asserts every core kind round-trips through ndjson. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
… e2e) PR 3 of 3, on rapidsai#386 (events) + rapidsai#391 (injection). Adapts each captured NvtxEvent into Quent's pipeline; the self-configuring cdylib NVTX loads via NVTX_INJECTION64_PATH. - Hook stamps each event and pushes to a bounded, lock-free ring (drop-and-count on overflow); a drain thread forwards to an ndjson exporter. - cdylib self-configures from QUENT_NVTX_OUTPUT_DIR at load, flushes at exit. - NvtxEventEntity: transparent newtype over NvtxEvent implementing EntityEvent. - e2e: one multi-threaded C++ producer (domain-scoped, mimics libcudf) run under the cdylib; asserts every core kind round-trips through ndjson. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
Wires captured NVTX events into a Quent pipeline, completing the foundational NVTX stack (#386 `quent-nvtx-events`, #391 `quent-nvtx-injection`). ### What it is The application drives capture: it owns its Quent `Context` and exporter, annotates its code with the NVTX Rust API, and links `quent-nvtx-injection` with its `static-injection` feature so NVTX initializes injection in-process at the first NVTX call — no cdylib, no `NVTX_INJECTION64_PATH`. - **`quent-nvtx-bridge`** — `NvtxEventEntity`, a `#[serde(transparent)]` newtype over `NvtxEvent` implementing Quent's `EntityEvent` (the orphan-rule adapter; the only crate depending on Quent internals). - **`quent-instrumentation`** — adds `Observer::sender()`, a cloned `EventSender` so the `'static` injection hook can emit into an app-owned observer that still flushes on drop. - **`quent-nvtx-injection`** — `static-injection` links the strong-symbol shim with `+whole-archive`, so the strong `InitializeInjectionNvtx2` overrides NVTX's weak no-op. - **`quent-nvtx-example`** — runnable wiring plus its test. ### Using it ```rust let observer = ctx.block_on(async { ctx.observer::<NvtxEventEntity>(opts).await })?; let sender = observer.sender(); quent_nvtx_injection::install_hook(move |e| { sender.send(Event::new_now(session, NvtxEventEntity::from(e))); })?; // ... nvtx::mark!(...), nvtx::range!(...) ... drop(observer); // flush ``` ### Tests `cargo test -p quent-nvtx-example` runs the example against a temp dir and asserts every core NVTX kind round-trips through ndjson. No GPU. Linux 64-bit only. Authors: - Pradeep Garigipati (https://github.com/9prady9) Approvers: - Johan Peltenburg (https://github.com/johanpel) URL: #402
3 participants
quent-nvtx-events— verbatim NVTX event vocabularyFirst of three PRs splitting the NVTX capture foundation out of #385 (per the request to split it up). Stack:
quent-nvtx-events— this PR (vocabulary types)quent-nvtx-injection— cdylib NVTX loads viaNVTX_INJECTION64_PATH(follows once this lands)quent-nvtx— bridge + deterministic test app + capture e2e (follows injection)What this adds
A Quent-agnostic, verbatim vocabulary of NVTX events — the raw shapes NVTX hands us, with no interpretation:
NvtxEventcovering the core surface: push/pop + start/end ranges, marks, domain lifecycle, registered strings, category/thread naming, resource create/destroy.NvtxMessage/NvtxColor/NvtxEventAttributes, and the corenvtxEventAttributespayload union captured undecoded (NvtxPayload).Pure vocabulary, separable by design
serdeis the only dependency and it's optional (default-onserdefeature), so a consumer that only needs the in-memory types doesn't pull it — compiles with and without the feature.Tests
None — the crate is pure serde-derived data types, so there's nothing crate-specific to assert beyond what serde itself guarantees (round-trip tests removed per review).