Repository navigation
feat: compare task attempts with accounting coverage - #480
Open
mnajafian-nv wants to merge 172 commits into
Open
mnajafian-nv wants to merge 172 commits into
mnajafian-nv wants to merge 172 commits into
Conversation
Applications can read a runtime's identity and status but not how much memory or CPU its adapter host tree uses. Issue NVIDIA#267 asks for a timestamped snapshot on the runtime API and a final snapshot on RunResult that reuses the same type. Add RuntimeResourceSnapshot and runtime_resource_snapshot(plan, runtime) with typed ResourceObservationUnavailable and ResourceObservationUnsupported errors. Linux reads /proc and macOS reads proc_pid_rusage, so no dependency is added; other platforms return Unsupported. The host PID lives in a map beside LOCAL_HOSTS, so a snapshot never waits for the host mutex a running invocation holds. Each local invocation's RunResult carries one final snapshot, and a failed read leaves it unset instead of failing the run. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Python applications need the same snapshot as Rust callers, with the same typed failures, to monitor a runtime while an invocation runs. Add Runtime.resource_snapshot(), the RuntimeResourceSnapshot type, RunResult.resource_snapshot, and ResourceObservationUnavailableError and ResourceObservationUnsupportedError (FabricRuntimeError subclasses). The PyO3 binding raises dedicated native exceptions so the SDK maps them by type rather than by message text. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Issue NVIDIA#267 requires the fixed measurement scope to be documented and observation to be kept distinct from resource enforcement. Describe the fields, the adapter host plus descendants scope, the typed errors, the CPU caveats for per-invocation deltas, and that limits belong to the operating system or execution environment, in the SDK guide and the integration skill reference. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Linux counts an exited descendant's CPU before its parent waits for it, while macOS counts it only after the wait. The field docs said only waited-for descendants count, which understated Linux. Say both, and re-wrap the integration skill paragraph. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Every entry the macOS reader queues already carries the rusage it was filtered on, so the optional value and the fallback re-read were dead paths. Queue the usage directly. Behavior is unchanged. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The macOS reader listed each child's PID and parent, then read the child's usage later by PID alone. A child that exited after the listing could have its PID reused by an unrelated process, whose usage was then counted in the runtime's snapshot. The listing now keeps each child's parent and start time, and the child's usage is counted only when the same parent and start time read back after the usage read. The host is read after the listing, so its listed children are known to be its own. The libproc calls sit behind a small trait so tests can script the reuse. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
…e read Both readers turned every failed read into "skip this process", so a child that had exited and a live child that could not be inspected gave the same successful, smaller snapshot. NVIDIA#267 asks for an inaccessible runtime to return ResourceObservationUnavailable and for unavailable measurements never to read as zero. Each read now reports whether the process is gone or unreadable. A gone or replaced child is still skipped. A live process in the tree that cannot be read, or a process table that cannot be listed in full, now returns ResourceObservationUnavailable with the reason. On macOS the listing falls back to the short BSD record for another user's process, so its parent is still known, and such a process inside the tree makes the snapshot unavailable. The Linux reads move behind the same kind of trait as macOS so tests can script denied, reaped and replaced processes. A new test covers a snapshot after an invocation timeout. The SDK guide, error docstrings and skill reference now name the new condition, and say that discovery reads other processes' parent IDs and start times while the snapshot exposes only its own fields. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Each local invocation reads a snapshot while it holds its host's lock. The Linux reader parsed /proc/<pid>/stat for every process on the machine, so the cost grew with the machine's process count. In a release build in Docker (rust:1.94, aarch64), the median over 60 reads was 0.66 ms with 108 processes, 5.8 to 7.1 ms with 1,008, 35 to 38 ms with 5,008 and 72 to 74 ms with 10,008. The reader now lists each process's children from /proc/<pid>/task/<tid>/children and reads only the runtime's tree. The same benchmark reads 0.027 to 0.028 ms at every process count. A child counts only when its stat, read after the listing, still names the expected parent. The kernel can skip a live child when a sibling is reaped during the read, so the children list is read until two reads agree, and the snapshot is unavailable after four reads that differ. Kernels without children files keep the full scan. Also name the macOS children map type, which clippy flagged. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The FabricError, runtime_resource_snapshot, resource_snapshot module and Python error docstrings now name an unreadable live process as a cause of ResourceObservationUnavailable, and the module docs describe how each reader finds the tree. Regenerated with Rust 1.94.0 and the locked docs tools; only the changed sentences are applied. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The macOS reader listed every process on the machine and read each one's parent to find the runtime's tree. That needed a short BSD record fallback for other users' processes, made a snapshot unavailable when any process on the machine refused both records, and dropped an exited child that its parent had not yet waited for out of CPU time, because PROC_PIDTBSDINFO fails on a zombie. The reader now lists each process's children with proc_listchildpids, as the Linux reader does with /proc children files. proc_listchildpids lists zombies and proc_pid_rusage reads them, so an unreaped child's CPU time counts on macOS the same way it does on Linux. A child counts only when it started before its parent's children were listed, which proves its PID was not reused after the listing. The walk now lives outside the platform modules behind one ProcessTable trait, so its scripted tests run on Linux CI for both identity rules. The Linux reader reports a children list that never settles as a changing tree instead of an unreadable process. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
On macOS, a descendant that runs as another user, including the setuid programs ps, top, and sudo, cannot be read, so the snapshot is unavailable while it runs. The docs now name those programs. Unsupported is also returned for the http and native_plugin adapter kinds, which the error docs left out. RunResult.resource_snapshot is None when the final read fails, and the docs now say that the snapshot call returns the error behind it. The SDK API inventory lists Runtime.resource_snapshot(). Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The test checks that a snapshot mapping round-trips and that invalid fields raise FabricConfigError, so its name now says both. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Picks up the macOS reader, Unsupported adapter kind, and RunResult snapshot wording from the Rust and Python docstrings. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
RunResult.resource_snapshot docs said that runtime_resource_snapshot returns the error behind a None. It does not: the final read's error is not kept, run_plan stops the runtime before a caller can ask, and a transient cause can be gone by the next read. The field now says only that it is absent when the runtime's resources could not be observed. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The Linux reader returns ResourceObservationUnavailable when 4 reads of a process's children never give two equal results in a row, because the runtime's process tree kept changing during the read. The public error docs listed only a stopped runtime, an exited host, and unreadable processes. They now list the changing tree too. On Linux with /proc mounted hidepid=invisible, another user's descendant reads as gone, so the snapshot leaves it out instead of reporting it unavailable. The module doc and the Python guide now say so. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Picks up the corrected RunResult.resource_snapshot field doc, the changing-tree unavailable reason, and the hidepid note from the Rust and Python docstrings. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The run record needs the same millisecond RFC 3339 timestamps as resource snapshots. Moving the formatter into crate::time gives core one copy of the civil date algorithm, so a later feature imports it instead of copying it. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
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 Linux tree walk accepted a child when its stat named the queued parent PID. If the parent and child both exited and other processes reused both PIDs with the same parent-child relation, the walk counted the unrelated process. The walk now reads the parent again after the child and counts the child only while the parent still has the start time it was counted with: a parent that is the same process after the child's read held its PID the whole time, so the child's parent PID names it. A parent that exited no longer shows that, so the child is skipped like a reparented one, and a parent that cannot be read makes the snapshot unavailable. Both Linux listings use the shared walk, so the check covers the children files and the fallback scan. The scripted regressions fail without it with 3 processes and 900 ns of CPU time from the unrelated process. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
With /proc mounted hidepid=invisible or hidepid=ptraceable, another user's process has no /proc entry, so the reader took a live descendant that changed user for an exited one and left it out of a successful snapshot. A missing /proc entry now counts as exited only when no process holds the PID, checked with signal 0; a PID that still names a process makes the snapshot unavailable. The kernel docs name kill -0 as the way to learn that a hidden PID exists. The fallback for a kernel without children files lists every process from /proc, where hidden processes are missing altogether. It now reads the /proc mount options and is unavailable under those two modes. With hidepid=noaccess, other users' processes stay listed and reading them fails, so the scan already reports the table as unreadable. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The SDK guide said a descendant hidden by hidepid=invisible is not counted. The reader now makes the snapshot unavailable instead, and the fallback for a kernel without children lists is unavailable under that mount, so the guide states both, for hidepid=ptraceable too. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The resource_snapshot module doc now says a descendant hidden by hidepid makes a snapshot unavailable. Regenerated with Rust 1.94.0; only this page changes. 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>
A run record's end line keeps the error code but no cause. A reader has to know core's and each adapter's codes to separate infrastructure failures from the agent's own, and an out-of-memory kill leaves no trace. The end line gains `stop`: a cause from Fabric's own closed list (fabric_failed, sandbox_failed, agent_exhausted_budget, unclassified, agent_failed), `reason: timeout` with the invoke timeout that applied, the time used and whether Fabric force-stopped the host, and the host's exit code or signal when it exited. Following the adapter contract, only a failed result reads agent_failed; every raised error other than a timeout or a host exit reads fabric_failed, except that an error from the exchange with the host during an out-of-memory rise reads sandbox_failed, because a kill can make the adapter raise. Both lines record the container's cgroup v2 oom_kill count when Fabric owns its cgroup. A rise sets a cause only with runtime.record.exclusive_container, which the start line records; set it only when nothing else runs in the container during an invocation, including other runtimes in the same process. The host's exit status is read only when a record is on. An ignored Linux test drives a real out-of-memory kill of the adapter host; it runs only in a memory-limited cgroup v2 container. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
InvocationRecorder::stop never sets fabric_failed together with host_exited, and a timeout and a host exit come from one error code, so they are never both true either. Two rule tests used those states, which taught the rule from cases that cannot happen. The second test now checks the real boundary: an out-of-memory rise outranks a timeout. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
terminate_local_host waits 2 seconds for the host to exit on its own before it kills it, so a host that exits in that window records its own status. On Windows the Job Object stop gives exit code 1 and no signal. State both instead of saying exit_signal is always the signal Fabric sent. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
finish read the container's oom_kill count after core had released the host lock and processed the response. In that window another caller on the same runtime can start the next exchange, and its out-of-memory kill would enter this record's rise and could set sandbox_failed on the wrong invocation. Read the count under the host lock as soon as exchange_lifecycle_message returns, before a timed-out host is stopped, and carry it to finish in an ExchangeEnd value together with the exchange's elapsed time and, after a failure, the host's wait status. ExchangeEnd replaces exchange_failed and is set on success too. After a host_crashed error the count is read again once the exit is observed and the larger value is kept, because the kernel counts a kill after it sends SIGKILL. File writes and the sync stay outside the lock. 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>
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>
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>
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
Comparing only returned results can hide missing attempts and incomplete usage. Execution success also does not establish whether a task's answer was correct.
Add a reader for an explicit list of task attempts and expected request results. It compares configurations while keeping missing evidence, usage coverage and evaluator verdicts visible. It also works for custom NVIDIA NeMo Fabric tasks without Harbor.
Dependencies: #424 supplies request records; #426 supplies resource observations in records. Both include #423 recording. The diff against
mainincludes unmerged prerequisites.Details
The application lists task, revision, attempt and configuration IDs in a manifest, with paths to expected result or request-record files. It separately declares whether the attempt list and each request list are complete. Optional evaluator identity and verdicts remain separate from execution status.
Report observed token and cost sums beside completeness counts and gap reasons. Missing results or unverified lists prevent a complete total. Full results remain authoritative; invocation records can supply saved accounting when the full result is unavailable. Legacy counters remain diagnostics.
Report differences in task/revision lists instead of silently excluding unmatched attempts. Mixed evaluators within a configuration have no combined pass fraction; different evaluators across configurations receive a warning. Different or unverified pricing bases also receive a warning.
Keep caller task duration separate from invocation timing and resource measurements. Do not combine incompatible timing scopes or add overlapping invocation durations. Preserve individual resource observations without inventing aggregate CPU or memory usage.
Run
python -m nemo_fabric.comparisons <manifest.json>for text,--detailsor--json. The example uses synthetic evidence without model calls or measured benchmark claims. The reader does not schedule tasks or evaluate correctness.Validation
Regression tests cover missing attempts and results, incomplete request lists, evaluator differences, pricing bases, parallel invocations and incompatible timing scopes. An SDK example uses synthetic complete, partial and missing evidence without making model calls.
Where should the reviewer start?
Start with
summarize_attemptsinsdk/python/nemo-fabric-runtime/src/nemo_fabric/comparisons.py. Review missing-result, inventory, evaluator, timing and pricing cases intests/python/test_comparisons.py, thenexamples/attempt_comparison.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.