Repository navigation
feat: save and inspect opt-in invocation records - #423
Open
mnajafian-nv wants to merge 74 commits into
Open
mnajafian-nv wants to merge 74 commits into
mnajafian-nv wants to merge 74 commits into
Conversation
Core already writes adapter-invocation.json before each request, so a killed run shows which invocation started. Nothing on disk says how long it ran, how it ended, or why it stopped, and RunResult exists only in memory. Add an opt-in, append-only run-record.jsonl per invocation under <artifact root>/.fabric/<runtime_id>/<invocation_id>/. runtime.record.enabled turns it on; an artifact root alone does not. Planning rejects an enabled record without an artifact root, with a non-local environment provider, or with an adapter that is not a process or python adapter. Core writes a start line right before it sends the request to the host and an end line after the invocation returns or raises, then syncs the file once after the host lock is released. Both lines are built from typed, allowlisted fields: other context keys appear by name only, override_keys lists override names without values, and the end line keeps the error code, stage and retryable flag but never the message or adapter metadata. A failed write adds a record_write_failed event and never fails the run. The record artifact gets a unique name, as adapter and Relay artifacts do. RecordConfig derives Default (recording off), so a caller can turn recording on without spelling out every record field. The record module is crate-private; the line format is published as the v1alpha1 run-record-line schema. The RFC 3339 formatter lives in a crate-private time module so other core features can share it. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The recorder promises that a failed record write never replaces the invocation result, but only a failed create was tested. Add per-thread test hooks to the record writer that delay record creation or fail the start line, the end line or the sync, and use them to check that each failure keeps the result's status, output, error, usage and identity, adds one record_write_failed event, and leaves a raised invocation error unchanged. The hooks compile only in tests. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Core set the adapter's deadline and then created the record and wrote its start line, while the host timeout starts only when the request is sent. Slow record storage therefore shortened the adapter's budget without shortening the host timeout, so recording could turn a successful invocation into a timeout. Create the record and write the start line before core sets the deadline and writes adapter-invocation.json. The start line records the timeout, not the deadline, so the deadline the adapter receives and the one in adapter-invocation.json stay the same value. A failed adapter-invocation.json write now happens after the record starts, so its end line reads artifact_write_failed. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The start line cut model role names and request context key names to 256 characters, and both are map keys. Two names that shared their first 255 characters became the same key, so one entry silently replaced the other. Config validation does not bound either name, so keep both names whole and cut only values. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The record docs said recording never changes the run and that the file is never created through a symbolic link. Both overstated the contract: record I/O runs on the invocation path, so slow storage delays the request and the return, and create_new refuses only a file or symbolic link at the record path itself. State that record I/O errors do not replace the invocation result, that recording is synchronous and best-effort with no time limit, that the record is created before the adapter deadline is set, and that creation refuses an existing file or symbolic link at run-record.jsonl. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
|
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:
WalkthroughThe PR adds shared usage accounting, model-usage normalization, and catalog-based cost estimates. It also adds opt-in local invocation recording and Python tools to summarize usage and inspect saved results. Adapter instrumentation, runtime configuration, schemas, tests, and references are updated. ChangesUsage accounting and pricing
Invocation recording and reporting
Supporting contracts and references
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~120 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant DeepAgentsRuntime
participant AdapterContractReducer
participant FabricCore
participant PricingCatalog
participant RunRecordWriter
DeepAgentsRuntime->>AdapterContractReducer: Reduce model-call observations
AdapterContractReducer->>FabricCore: Return accounting extensions
FabricCore->>PricingCatalog: Resolve eligible catalog rates
PricingCatalog-->>FabricCore: Return rate and catalog metadata
FabricCore->>RunRecordWriter: Persist invocation lines and finalized result
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 440 functions across 53 files. (144 skipp… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title follows Conventional Commits format with the allowed lowercase type feat, uses a concise imperative summary, is 48 characters long, and has no trailing period. It accurately describes the …Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description is complete and follows the repository template. It includes an overview, reviewer starting points, related-issue information, and both required confirmation checkboxes. I… Full details: Title check
-
Explanation
The title follows Conventional Commits format with the allowed lowercase type
feat, uses a concise imperative summary, is 48 characters long, and has no trailing period. It accurately describes the opt-in invocation-record changes.
Full details: Description check
-
Explanation
The pull request description is complete and follows the repository template. It includes an overview, reviewer starting points, related-issue information, and both required confirmation checkboxes. It also provides useful implementation details and validation coverage.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 40.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 440 functions across 53 files. (144 skipped: 136 unsupported, 8 over the file limit.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Fern docs preview: https://nvidia-preview-pull-request-423.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This was referenced Oct 16, 2026
When the invocation raises, the run record cannot add an event to a result, and the raised error passes through unchanged. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A returned result can still gain the run_record artifact, so list the fields that stay the same instead of saying nothing else changes. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
…sult The result extension check raises InvalidAdapterExtension for a value that breaks the declared schema; the end line recorded it as fabric_error, so one fault had two codes. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
1 task done
Contributor
Author
|
/nvskills-ci |
Bring in the explicit environment lifecycle SDK, the OpenShell environment provider, and multi-runtime sandboxes. The config and runtime test modules keep both the run record tests and main's environment tests, and SchemaName::ALL grows to 23 entries for EnvironmentReference and RunRecordLine. The run record contract is unchanged. Main resolves a local environment's artifact root as before, falling back to runtime.artifacts, and a non-local provider, OpenShell included, still fails planning when the record is on. start_runtime_in() starts a local runtime through the same local adapter host, so its invocations write the record the same way. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Main now ships OpenShell as a real environment provider whose task paths pass config validation. Check that an OpenShell environment with valid task paths still fails planning when the run record is on, next to the existing docker case. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Regenerate the Rust library reference with Rust 1.94.0 so it lists main's environment lifecycle items next to the run record items, with updated page positions. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Bring in Relay OpenTelemetry file sinks, OpenShell SDK runtimes in task sandboxes, the turnkey OpenShell echo example, and the OpenShell sandbox name limit fix. The crate root declares both main's task_command module and this branch's time module. An enabled record still requires the local provider: config validation rejects it on the OpenShell provider, and start_runtime_in() checks that the environment handle's provider matches the plan, so main's OpenShell runtime path never runs with a record enabled. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Bring in the Claude adapter's per-invocation cost and cache token usage. No conflicts. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The Deep Agents adapter summed usage_metadata from the AI messages that reached graph state through the updates stream. Three cases came out wrong: - Deep Agents adds SummarizationMiddleware to every agent and subagent. Its summary call returns only text, so the summary model's message and usage never enter graph state, and an invocation that summarized under-reported input, output and total tokens. - When the invocation raised, including GraphRecursionError, the adapter returned no messages, so usage was None even though model calls had finished. - A count was summed over the calls that reported it and returned as known. ChatOpenAI also turns an omitted Chat Completions count into 0 and derives a missing total, so a partial provider report came out as measured zeros. The adapter now records usage from LangChain's chat-model callbacks, which fire for every chat-model call that runs in the adapter process: the main agent, declarative subagents and summarization. Agent Protocol subagents run remotely and are not included. A new recorder is created for each invocation, so earlier turns are never counted again. It outlives an exception, so a failed invocation still reports the fields that every counted call reported. Each token count and the cost are known only when every counted call in the invocation reported them, and an explicit zero stays a known zero. A request the provider rejects with a 4xx status (other than 408) contributes no usage. A 5xx response, timeout, cancellation or unfinished call leaves the invocation's counts and cost unknown, because the provider may have done work without reporting usage. OpenAI SDK retries happen inside one LangChain call, so a 5xx the SDK retries to success is not visible to the recorder, and only the final response's reported usage counts. When a response carries OpenAI-shaped raw usage, the raw counts decide what was reported: an omitted count stays unknown, the provider's total is used when present, and a derived total counts only when input and output were both reported. The graph tests run the real Deep Agents graph and ChatOpenAI against a scripted Chat Completions endpoint for summarization, parallel task subagents, 400, 408, 503 and timeout failures, the recursion limit, partial reports and explicit zeros. Separate ChatOpenAI.with_retry tests cover 429 then success and 503 then success. The existing usage tests now produce their messages through a LangChain chat-model call so the callbacks fire. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
mnajafian-nv
marked this pull request as ready for review
October 23, 2026 15:51
mnajafian-nv
marked this pull request as draft
October 23, 2026 15:53
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Use the system Bash path so macOS tests exercise Bash 3.2 even when PATH contains a newer Bash. This keeps the empty-array regression meaningful in CI. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
mnajafian-nv
marked this pull request as ready for review
October 23, 2026 18:37
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
SDK and CLI callers need a saved result to investigate a run after it finishes. Without recording, they must arrange their own result storage and cannot inspect an invocation record from that run.
Add optional local recording to NVIDIA NeMo Fabric. Save a bounded invocation record and a separate full result, then provide a command to inspect the available evidence, including failed results.
Dependencies: #478 supplies catalog pricing and includes #442 shared accounting. The diff against
mainincludes unmerged prerequisites.Details
Enable recording with
runtime.record.enabledand an artifact root. CLI users can pass--record --artifacts <directory>. Recording supports local Python and process adapter hosts. It is disabled by default; unsupported locations or a missing artifact root fail validation.The invocation record saves start/end lines with identities, configured model names, sanitized error codes and validated accounting. Each line is limited to 64 KiB. It excludes prompts, output, exception messages and arbitrary adapter metadata. The separate full-result file can contain sensitive output. Existing files and symlinks are not overwritten.
One-shot results are saved after shutdown. Persistent-runtime results describe the invocation before later shutdown. The invocation end line keeps that earlier boundary. Recording uses synchronous, best-effort writes; it does not guarantee a record after a process crash. Storage failures add diagnostics without retrying model work or changing the execution outcome.
python -m nemo_fabric.reporting <path>shows execution status, elapsed-time scope, complete and observed usage, cost coverage and evidence paths.--detailsadds available resource observations and pricing information;--jsonkeeps exact values. A record can supply saved accounting when the full result is unavailable. Logs and traces never fill missing counts.Rust callers that construct
RuntimeConfigexhaustively must add its new optional field. One-shot SDK errors retain their specific plan, preparation or startup stage.Validation
Regression tests cover opt-in recording, failed-result retention, warm runtimes, exact line-size limits, existing files and symlinks, and storage failures that preserve the execution outcome. Reader tests cover saved accounting and full-result precedence.
Where should the reviewer start?
Start with publication in
crates/fabric-core/src/record/results.rsand the lifecycle boundaries inruntime.rs. Then checktests/python/test_recording.pyand the reader insdk/python/nemo-fabric-runtime/src/nemo_fabric/reporting.py.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to: none
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.