docs: correct observability and optimization guides - #1070
Conversation
a05bb30 to
70bf357
Compare
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
70bf357 to
86a3471
Compare
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesDocumentation workflows
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/agents/insight-driven-optimization.mdx (1)
160-179: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the lifecycle of Insight evidence.
trace_refsremain persisted after the 90-day TTL removes their spans and trace-index rows. The evidence table omits unavailable traces and reports how many could not be loaded. Update both guides to describe this behavior for platform Insights and local YAML files.🤖 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 `@docs/agents/insight-driven-optimization.mdx` around lines 160 - 179, Update the Insight evidence lifecycle documentation in docs/agents/insight-driven-optimization.mdx (lines 160-179) and docs/agents/observability.mdx (lines 362-363) to state that trace_refs remain persisted after the 90-day TTL removes their spans and trace-index rows, while unavailable traces are omitted from the evidence table and the number of omitted traces is reported; describe this behavior for both platform Insights and local YAML files.
🤖 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 `@docs/agents/insight-driven-optimization.mdx`:
- Around line 223-235: Update the dataset descriptions in the Experimenter
workflow around the validation and training evaluation steps (Lines 126–128) so
they reflect that survivors are first ranked by validation rewards, then
evaluated on the training split. Ensure the documented ordering matches the
implementation and does not describe repeated selection based on held-out
validation scores.
- Around line 428-434: Update the Experimenter evaluation-results description in
docs/agents/insight-driven-optimization.mdx (428-434) to state that each trial
is retrieved by trace_id and every evaluator result is attached to
trace.root_span_id with session_id=trace.session_id, replacing the generic
“attached to the trace” wording. In docs/agents/observability.mdx (254-272),
distinguish this explicit API attachment path from the ATIF-synthesized
harbor.verifier span, with no other behavioral changes.
In `@docs/agents/observability.mdx`:
- Around line 98-108: Update docs/agents/observability.mdx lines 98-108 to use
NMP_BASE_URL=http://localhost:8080 and add the required existing-instance and
/health/ready preflight before CLI/API operations; update lines 124-126 to start
services only when no ready instance exists. In
docs/agents/insight-driven-optimization.mdx lines 299-336, verify /health/ready
before running token and agent commands, preserving the documented
authentication behavior.
In `@docs/evaluator/experiments.mdx`:
- Line 87: Update docs/evaluator/experiments.mdx at lines 87-87 to define each
session without a test_case_id as its own test case and include it in
test_case_count and rollups; at lines 249-252, describe test_case_id as grouping
repeated sessions into one test case without requiring it for rollups; and at
lines 504-504, remove missing test_case_id from the causes of zero metrics.
- Around line 126-131: Update the local setup sections around the environment
exports and CLI examples to use NMP_BASE_URL=http://localhost:8080, check
whether an existing instance is available, and direct users to SETUP.md when it
is not ready. Add a /health/ready verification before running nemo auth token or
other API commands, while keeping the deployed endpoint configurable in a
separate setup branch and applying the same sequence to the referenced sections.
- Around line 126-136: Update the authentication examples around NMP_BASE_URL
and NMP_ACCESS_TOKEN to require a trusted https:// base URL whenever the bearer
token is used. Keep http:// examples only for deployments with authentication
disabled, where the Authorization header and NMP_ACCESS_TOKEN are omitted.
---
Outside diff comments:
In `@docs/agents/insight-driven-optimization.mdx`:
- Around line 160-179: Update the Insight evidence lifecycle documentation in
docs/agents/insight-driven-optimization.mdx (lines 160-179) and
docs/agents/observability.mdx (lines 362-363) to state that trace_refs remain
persisted after the 90-day TTL removes their spans and trace-index rows, while
unavailable traces are omitted from the evidence table and the number of omitted
traces is reported; describe this behavior for both platform Insights and local
YAML files.
🪄 Autofix
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: ced3c009-b19a-4d96-b457-3942b1997282
📒 Files selected for processing (3)
docs/agents/insight-driven-optimization.mdxdocs/agents/observability.mdxdocs/evaluator/experiments.mdx
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
CLI and API example verificationPushed commit
I also rechecked the remaining command surfaces against their Typer registrations and implementation code:
Validation results:
Verification was source-based because this checkout does not contain a synced |
|
Summary
Why
This is a documentation correctness follow-up to #1048 and #1014. Several examples and behavioral descriptions no longer matched the merged implementation, and remote-deployment examples assumed unauthenticated localhost defaults.
Root cause
The guides described intended behavior while the Intake rollup, deletion, and optimizer command implementations continued to evolve. The examples also did not account for curl URL globbing, authenticated deployments, or the Analyst's profile-based local persistence mode.
Impact
Readers can now run the documented commands against authenticated local or remote deployments and verify the correct persistence target. The Experiments and optimization behavior described in the guides matches the current checked-in implementation.
Validation
make docs-check(215 MDX files parsed cleanly; gated-link validation passed)bash -ngit diff --checkmake docs-broken-linksreported no issues in the modified pages; four existing errors remain in unrelated authentication and Studio pagesSummary by CodeRabbit