feat(core): add opt-in provider hooks for generated ids and timestamps - #2114
Merged
rapids-bot[bot] merged 3 commits intoJul 18, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughProcess-wide providers now control runtime-generated identifiers and timestamps. Workflow execution, contexts, intermediate-step payloads, and spans use these helpers, with deterministic behavior documented and covered by provider propagation tests. ChangesDeterministic runtime providers
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant Providers
participant IntermediateStepPayload
participant Context
Runner->>Providers: generate workflow ID and trace ID
Providers-->>Runner: return configured identifiers
Runner->>IntermediateStepPayload: create workflow start/end payloads
IntermediateStepPayload->>Providers: generate payload ID and timestamp
Runner->>Context: store workflow run and trace identifiers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The runtime stamps workflow runs, intermediate steps, spans, and function invocations with uuid.uuid4() and time.time() values called inline at each site. That makes two otherwise identical runs produce different identifiers and timings, which hurts record/replay style testing, golden-file trace comparison, and integrations with runtimes that re-execute workflow code and need identifiers to remain stable across re-executions. Add nat.utils.providers with process-wide, opt-in id and time provider hooks defaulting to uuid.uuid4/time.time, and route the runtime call sites through them: * Runner workflow run id, trace id, and workflow step id (single and streaming paths) * Context.push_active_function invocation id * IntermediateStepPayload UUID and event_timestamp default factories * Span, SpanContext, and SpanEvent trace ids, span ids, and timestamps Pydantic default factories reference the module-level functions so the installed provider is resolved lazily at model construction time. Behavior is unchanged when no provider is installed. Signed-off-by: David Hyde <DABH@users.noreply.github.com>
DABH
force-pushed
the
deterministic-id-time-providers
branch
from
July 14, 2026 06:00
057e30b to
f4fafaf
Compare
DABH
marked this pull request as ready for review
July 14, 2026 06:31
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nvidia_nat_core/tests/nat/utils/test_providers.py (1)
42-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required explicit fixture name and fixture suffix.
Rename the implementation to
restore_providers_fixtureand setname="restore_providers"on the decorator. As per coding guidelines, “Pytest fixtures should define the name argument” and fixture functions must use afixture_prefix or_fixturesuffix.Proposed fix
-@pytest.fixture(autouse=True) -def restore_providers(): +@pytest.fixture(name="restore_providers", autouse=True) +def restore_providers_fixture():🤖 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 `@packages/nvidia_nat_core/tests/nat/utils/test_providers.py` around lines 42 - 51, Rename the fixture function from restore_providers to restore_providers_fixture and set the decorator’s explicit name to restore_providers, preserving its autouse behavior and provider restoration logic.Sources: Coding guidelines, Path instructions
🤖 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 `@packages/nvidia_nat_core/src/nat/utils/providers.py`:
- Around line 108-121: Update generate_trace_id and generate_span_id to compute
their derived integer IDs, reject a zero result by raising ValueError, and
return only non-zero IDs. Add coverage for the nil UUID and UUIDs whose high
word is zero, preserving the existing provider-based generation behavior.
---
Nitpick comments:
In `@packages/nvidia_nat_core/tests/nat/utils/test_providers.py`:
- Around line 42-51: Rename the fixture function from restore_providers to
restore_providers_fixture and set the decorator’s explicit name to
restore_providers, preserving its autouse behavior and provider restoration
logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b11fc0b7-2353-42b9-a001-54a9fbe2894b
📒 Files selected for processing (7)
docs/source/run-workflows/observe/observe.mdpackages/nvidia_nat_core/src/nat/builder/context.pypackages/nvidia_nat_core/src/nat/data_models/intermediate_step.pypackages/nvidia_nat_core/src/nat/data_models/span.pypackages/nvidia_nat_core/src/nat/runtime/runner.pypackages/nvidia_nat_core/src/nat/utils/providers.pypackages/nvidia_nat_core/tests/nat/utils/test_providers.py
willkill07
added
feature request
Member
|
@DABH coderabbit feedback seems reasonable here. No need to address yet. Otherwise, I'll kick off CI to see how things go. We can defer updates based on the initial CI trigger. |
Member
|
/ok to test f4fafaf |
Member
|
@DABH in the documentation can you rephrase "record/replay" to "record or replay"? Also, i do think addressing the coderabbit feedback should be done. |
Contributor
Author
|
Thanks so much for taking a look!! Yes - will address that change and the coderabbit feedback shortly. Thanks again! |
generate_trace_id and generate_span_id now raise ValueError when the installed id provider yields a UUID whose derived 128-bit trace ID or 64-bit span ID is zero (the nil UUID, or any UUID with an all-zero high word for span ids), upholding the non-zero contract enforced by SpanContext instead of propagating an invalid telemetry id. Adds tests for both shapes, renames the test fixture to follow the fixture naming guideline, and rephrases record/replay wording in the observability guide (which also satisfies the docs path checker). Signed-off-by: David Hyde <DABH@users.noreply.github.com>
Signed-off-by: David Hyde <DABH@users.noreply.github.com>
Contributor
Author
|
@willkill07 Should be all set - thanks again for taking a look! |
Member
|
/ok to test 7f819a4 |
willkill07
approved these changes
Jul 18, 2026
Member
|
/merge |
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
2 participants
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.
Description
The runtime currently stamps workflow runs, intermediate steps, spans, and function invocations with
uuid.uuid4()andtime.time()values called inline at each site (runtime/runner.py,builder/context.py,data_models/intermediate_step.py,data_models/span.py). Two otherwise identical runs therefore never produce the same identifiers or timings. That gets in the way of several third-party and testing use cases:This PR adds a small additive module,
nat.utils.providers, with process-wide, opt-in hooks:set_id_provider/get_id_provider— a zero-argument callable returning a new identifier as a canonical UUID string (default:uuid.uuid4).set_time_provider/get_time_provider— a zero-argument callable returning the current time in fractional seconds (default:time.time).generate_id,generate_trace_id(128-bit),generate_span_id(64-bit),current_time, andcurrent_time_ns. Integer identifiers are derived by parsing the id provider's UUID string, so the default distribution is identical touuid.uuid4().int.The existing call sites are routed through these hooks:
Runnerworkflow run id, trace id, and workflow step id (single and streaming paths).Context.push_active_functioninvocation id.IntermediateStepPayloadUUIDandevent_timestampdefault factories.Span/SpanContext/SpanEventtrace ids, span ids, and start/end/event timestamps.Pydantic
default_factorysites reference the module-level functions, so the installed provider is resolved lazily at model construction time rather than captured at import.Zero behavior change by default: when no provider is installed, the defaults are
uuid.uuid4andtime.time, exactly as before. Setters return the previously installed provider so callers can restore it. The hook is documented briefly in the observability guide, and the module is picked up by the auto-generated API reference.No tracking issue exists for this yet; happy to file one if the team prefers.
Testing
packages/nvidia_nat_core/tests/nat/utils/test_providers.py(8 tests): default provider behavior, setter install/restore semantics, UUID-string contract for integer id derivation, lazy resolution through Pydantic default factories, span model injection,push_active_functioninjection, and an end-to-end minimalRunnerrun (single and streaming) asserting deterministic run ids, trace ids, step UUIDs, and timestamps.uv run pytest packages/nvidia_nat_core— 2725 passed, 54 skipped (defaults unchanged).pre-commit run yapf|ruff-check --files <touched files>,python ci/scripts/copyright.py --verify-apache-v2,valeon the touched Markdown, andpython ci/scripts/path_checks.pyall pass.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Tests