feat(insights): enable profile-driven analysis - #718
Conversation
|
🌿 Preview your docs: https://nvidia-preview-feat-insights-profile-dx.docs.buildwithfern.com/nemo-platform |
|
97b09f2 to
04d6dc0
Compare
📝 WalkthroughWalkthroughThe change adds profile-driven Insights analysis and readiness checks, direct empty-target testbed restoration, and repository-scoped release workflows with updated documentation and tests. ChangesInsights analysis workflow
Direct testbed restore
Testbed release workflow
Sequence Diagram(s)sequenceDiagram
participant User
participant InsightsCLI
participant Profile
participant Preflight
participant Analyst
User->>InsightsCLI: run analyze or doctor
InsightsCLI->>Profile: discover and load optimizer.yaml
Profile-->>InsightsCLI: resolved analysis inputs
InsightsCLI->>Preflight: run readiness checks
Preflight-->>InsightsCLI: required failures and advisories
InsightsCLI->>Analyst: run analysis when required checks pass
Analyst-->>InsightsCLI: insights result or execution error
sequenceDiagram
participant User
participant RestoreCLI
participant WorkspaceMapping
participant IngestBundle
User->>RestoreCLI: restore with --into WORKSPACE
RestoreCLI->>WorkspaceMapping: validate target and single-workspace bundle
WorkspaceMapping-->>RestoreCLI: exact source-to-target mapping
RestoreCLI->>IngestBundle: ingest with require_empty=True
IngestBundle-->>RestoreCLI: restore outcome or empty-target failure
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
plugins/nemo-insights/README.md (2)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured product-name substitutions.
Replace the hardcoded
Insightsproduct references with the project’s Sphinx substitutions.As per coding guidelines, “Never hardcode product names; use substitutions in Sphinx configuration to maintain consistency across documentation.”
Also applies to: 25-25, 33-36
🤖 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 `@plugins/nemo-insights/README.md` at line 15, Update the product-name references in the README, including the discovery description and the sections around the additional referenced lines, to use the project’s configured Sphinx substitution instead of hardcoded “Insights” text. Preserve the existing documentation wording and behavior while applying the established substitution consistently.Source: Coding guidelines
15-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate the CLI how-to from the profile reference.
Lines 15-23 are HOW-TO content; Lines 25-49 are REFERENCE content. Move the contract details to a reference page and cross-link it.
As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead.”
🤖 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 `@plugins/nemo-insights/README.md` around lines 15 - 49, Separate the command walkthrough from the profile contract in the README: retain only the user-facing how-to steps there, move the YAML fields, environment resolution, profile discovery, and output behavior into a dedicated reference page, and add a clear cross-link from the README to that page. Keep the documented behavior unchanged.Source: Coding guidelines
🤖 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 `@plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py`:
- Around line 479-483: The _write_records method currently reconstructs YAML
with only the insights key, discarding other validated top-level fields. Update
analyst_backend.py lines 479-483 to preserve the original document mapping when
writing, or update validation to reject unsupported top-level fields; in either
case, update test_cli_profile.py lines 594-613 to exercise real persistence and
verify metadata preservation or the expected rejection.
In `@plugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.py`:
- Around line 81-86: Update the environment assignment flow around the key/value
parsing and env population logic to catch ValueError raised when a parsed value
contains invalid characters such as NUL bytes, and re-raise it as the
established EnvFileError type so the CLI retains clean error handling. Preserve
valid assignments and duplicate-key behavior.
In `@plugins/nemo-insights/src/nemo_insights_plugin/profile.py`:
- Line 17: Update the agent field in the profile model to reject whitespace-only
values, while preserving nonblank names; implement the validation at the
field/model validation layer and add a profile test covering a whitespace-only
agent name.
In `@plugins/nemo-insights/testbed/cli.py`:
- Around line 708-710: Update the CLI flow around _resolve_bundle and
reingest.explicit_workspace_map so --into syntax is validated before resolving
or downloading the bundle. After loading the manifest, retain the workspace-map
validation needed for the single-workspace constraint. Add a test covering an
invalid --into value with a published --state, in addition to the existing
local-file coverage.
In `@plugins/nemo-insights/testbed/README.md`:
- Around line 112-126: Update the restore documentation so the HOW-TO section no
longer claims every restore targets <ws>-<ref> or cannot affect other
workspaces; retain only workflow instructions there. Move the default-restore
and direct --into mode semantics to a single REFERENCE section/page, then add
cross-links between the HOW-TO and reference locations.
In `@plugins/nemo-insights/testbed/reingest.py`:
- Around line 635-643: Make fresh-target enforcement atomic in
test_require_empty_ingests_all_nonempty_collections_in_order by replacing the
client-side annotation/result recount and later span write sequence with the
server-side workspace reservation or atomic emptiness operation. Ensure all
non-empty collections are validated and reserved together before any
direct-restore writes, so concurrent population causes the operation to fail
without persisting partial spans, annotations, or evaluator results.
---
Nitpick comments:
In `@plugins/nemo-insights/README.md`:
- Line 15: Update the product-name references in the README, including the
discovery description and the sections around the additional referenced lines,
to use the project’s configured Sphinx substitution instead of hardcoded
“Insights” text. Preserve the existing documentation wording and behavior while
applying the established substitution consistently.
- Around line 15-49: Separate the command walkthrough from the profile contract
in the README: retain only the user-facing how-to steps there, move the YAML
fields, environment resolution, profile discovery, and output behavior into a
dedicated reference page, and add a clear cross-link from the README to that
page. Keep the documented behavior unchanged.
🪄 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: 9172cfd5-22ae-4c2a-97aa-94964d5732f9
📒 Files selected for processing (22)
plugins/nemo-insights/README.mdplugins/nemo-insights/pyproject.tomlplugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/checks.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/insights.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.pyplugins/nemo-insights/src/nemo_insights_plugin/preflight.pyplugins/nemo-insights/src/nemo_insights_plugin/profile.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/reingest.pyplugins/nemo-insights/tests/contracts/test_checks.pyplugins/nemo-insights/tests/contracts/test_insights.pyplugins/nemo-insights/tests/contracts/test_profile_contract.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/test_preflight.pyplugins/nemo-insights/tests/test_profile.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_reingest.py
💤 Files with no reviewable changes (1)
- plugins/nemo-insights/pyproject.toml
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Replace a nested ternary in make_check_result with an explicit if/elif/else, and fold load_env_file's path.is_file() probe into its existing try/except so a permission error while checking existence is wrapped as EnvFileError instead of leaking a raw OSError. Missing-file no-op behavior is unchanged; adds one focused regression test. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Read environment files directly so missing files remain a no-op while other metadata and read failures are consistently wrapped. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
load_insights_document and validate_insights_file now both go through one internal read/parse/validate helper instead of validate doing a separate stat() before the read, closing a TOCTOU window; a file that disappears at the read itself is treated as missing, same as before. Generic OSError failures now say "could not be read" instead of the misleading "not readable as UTF-8" (UnicodeError keeps that wording). LocalAnalystBackend's file read/write now pass encoding="utf-8" explicitly, matching the shared validator. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
04d6dc0 to
9498b45
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/nemo-insights/README.md (1)
15-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate the HOW-TO from the profile reference.
Lines 15-23 are a HOW-TO, while Lines 25-49 are reference material. Split them into distinct sections or pages, add prerequisites before the commands, and add a Next Steps section with cross-links.
As per coding guidelines: each documentation page should fit one Diataxis quadrant, list prerequisites at the top, and include Next Steps.🤖 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 `@plugins/nemo-insights/README.md` around lines 15 - 49, Restructure the README so the command walkthrough and profile contract are separate documentation sections or pages, keeping each focused on a single Diataxis quadrant. Add a prerequisites section before the command examples, and finish with a Next Steps section containing relevant cross-links to the complementary documentation.Source: Coding guidelines
plugins/nemo-insights/tests/contracts/test_profile_contract.py (1)
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing home-path assertion.
The test name promises
~expansion coverage but never supplies a home-relative path.Proposed test
-def test_resolve_profile_path_handles_relative_absolute_and_home(tmp_path: Path) -> None: +def test_resolve_profile_path_handles_relative_absolute_and_home( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("HOME", str(tmp_path)) relative = resolve_profile_path("./agent", tmp_path) absolute = resolve_profile_path(str(tmp_path / "agent"), Path("/elsewhere")) + home = resolve_profile_path("~/agent", Path("/elsewhere")) assert relative == (tmp_path / "agent").resolve() assert absolute == (tmp_path / "agent").resolve() + assert home == (tmp_path / "agent").resolve()🤖 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 `@plugins/nemo-insights/tests/contracts/test_profile_contract.py` around lines 73 - 78, Extend test_resolve_profile_path_handles_relative_absolute_and_home to call resolve_profile_path with a home-relative "~" path and assert it resolves to the expected home-directory location, preserving the existing relative and absolute assertions.
🤖 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 `@plugins/nemo-insights/README.md`:
- Around line 41-44: Update the README precedence statement to match the CLI
handling in the argument-resolution logic: state that agent is required and
raises when absent from both --agent and the profile, workspace and the base URL
use their built-in defaults, and agent_spec falls back to conventional
profile-local files rather than a built-in default. Preserve the documented
precedence of explicit flags over profile values and NMP_BASE_URL, including
--base-url over NMP_BASE_URL.
---
Nitpick comments:
In `@plugins/nemo-insights/README.md`:
- Around line 15-49: Restructure the README so the command walkthrough and
profile contract are separate documentation sections or pages, keeping each
focused on a single Diataxis quadrant. Add a prerequisites section before the
command examples, and finish with a Next Steps section containing relevant
cross-links to the complementary documentation.
In `@plugins/nemo-insights/tests/contracts/test_profile_contract.py`:
- Around line 73-78: Extend
test_resolve_profile_path_handles_relative_absolute_and_home to call
resolve_profile_path with a home-relative "~" path and assert it resolves to the
expected home-directory location, preserving the existing relative and absolute
assertions.
🪄 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: e4100963-cc51-4b49-98e0-b5dd2b5f68ca
📒 Files selected for processing (22)
plugins/nemo-insights/README.mdplugins/nemo-insights/pyproject.tomlplugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/checks.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/insights.pyplugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.pyplugins/nemo-insights/src/nemo_insights_plugin/preflight.pyplugins/nemo-insights/src/nemo_insights_plugin/profile.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/reingest.pyplugins/nemo-insights/tests/contracts/test_checks.pyplugins/nemo-insights/tests/contracts/test_insights.pyplugins/nemo-insights/tests/contracts/test_profile_contract.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/test_preflight.pyplugins/nemo-insights/tests/test_profile.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_reingest.py
💤 Files with no reviewable changes (1)
- plugins/nemo-insights/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (14)
- plugins/nemo-insights/src/nemo_insights_plugin/profile.py
- plugins/nemo-insights/tests/contracts/test_checks.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/run.py
- plugins/nemo-insights/src/nemo_insights_plugin/contracts/checks.py
- plugins/nemo-insights/tests/contracts/test_insights.py
- plugins/nemo-insights/testbed/README.md
- plugins/nemo-insights/tests/test_periodic_analysis.py
- plugins/nemo-insights/src/nemo_insights_plugin/contracts/profile.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py
- plugins/nemo-insights/src/nemo_insights_plugin/cli.py
- plugins/nemo-insights/tests/testbed/test_cli.py
- plugins/nemo-insights/tests/test_profile.py
- plugins/nemo-insights/tests/test_cli_profile.py
- plugins/nemo-insights/testbed/reingest.py
Route fixture operations explicitly and require protected credentials so cross-repository state remains immutable and safely accessible. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
e249993 to
154c289
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plugins/nemo-insights/testbed/README.md`:
- Around line 69-76: Replace the literal NVIDIA-dev/NeMo-Optimizer references in
the README with the configured Sphinx substitution for the fixture repository,
including the additionally applied section. Preserve the existing guidance about
TESTBED_STATE_REPO, token permissions, and canonical automated publishing.
- Around line 348-353: Add a “Next Steps” section at the end of the README after
the secrets documentation, with cross-links to the related restore, publish, and
workflow documentation. Use the repository’s existing documentation paths and
link conventions.
🪄 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: bc8a64bf-3487-4fd0-b925-3e587f343a17
📒 Files selected for processing (6)
.github/workflows/insights-testbed.ymlplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/publish.pyplugins/nemo-insights/testbed/release.pyplugins/nemo-insights/tests/testbed/test_publish.pyplugins/nemo-insights/tests/testbed/test_release.py
* feat(insights): add profile-driven analysis workflow Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Add shared readiness result contracts Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Add shared agent profile mechanics Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Add shared Insights document contracts Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Keep Insights YAML errors single-line Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Use shared contracts in Insights preflight Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Keep Insights CLI imports working Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Use shared contracts in Insights profiles Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Use unique profile contract test name Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Render Insights environment errors directly Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Route Insights CLI through shared contracts Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Cover doctor base URL resolution Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Clarify check-status branching and harden env-file existence check Replace a nested ternary in make_check_result with an explicit if/elif/else, and fold load_env_file's path.is_file() probe into its existing try/except so a permission error while checking existence is wrapped as EnvFileError instead of leaking a raw OSError. Missing-file no-op behavior is unchanged; adds one focused regression test. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Use one read boundary for environment files Read environment files directly so missing files remain a no-op while other metadata and read failures are consistently wrapped. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Share one read boundary for Insights files load_insights_document and validate_insights_file now both go through one internal read/parse/validate helper instead of validate doing a separate stat() before the read, closing a TOCTOU window; a file that disappears at the read itself is treated as missing, same as before. Generic OSError failures now say "could not be read" instead of the misleading "not readable as UTF-8" (UnicodeError keeps that wording). LocalAnalystBackend's file read/write now pass encoding="utf-8" explicitly, matching the shared validator. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Keep Insights Git package sources caller-owned Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> * Harden testbed fixture publishing Route fixture operations explicitly and require protected credentials so cross-repository state remains immutable and safely accessible. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> --------- Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Post-merge review follow-up to #718. Analyze keeps other top level keys when writing the shared insights file, .env values keep quotes that are part of the secret, --agent alone bypasses a broken profile, local file errors are no longer reported as platform errors, doctor runs its checks even without a profile, analyze drops the duplicate network probes, restore drops the mid-run recheck, the download cache is split per repo, and the first-span guard compares timestamps chronologically. Restores --clobber and substring release matching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
Post-merge review follow-up to #718. Analyze keeps other top level keys when writing the shared insights file, .env values keep quotes that are part of the secret, --agent alone bypasses a broken profile, local file errors are no longer reported as platform errors, doctor runs its checks even without a profile, analyze drops the duplicate network probes, restore drops the mid-run recheck, the download cache is split per repo, and the first-span guard compares timestamps chronologically. Restores --clobber and substring release matching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Aditya Pandey <aditypandey@nvidia.com>
…Mo#752) Post-merge review follow-up to NVIDIA-NeMo#718. Analyze keeps other top level keys when writing the shared insights file, .env values keep quotes that are part of the secret, --agent alone bypasses a broken profile, local file errors are no longer reported as platform errors, doctor runs its checks even without a profile, analyze drops the duplicate network probes, restore drops the mid-run recheck, the download cache is split per repo, and the first-span guard compares timestamps chronologically. Restores --clobber and substring release matching. Signed-off-by: Aditya Pandey <aditypandey@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
optimizer.yamlwith local.envsupport.--into.Test plan
uv run --group insights pytest plugins/nemo-insights/tests/ -quv run ruff check plugins/nemo-insights/uv run --frozen ty check plugins/nemo-insights/src/nemo_insights_plugin/Summary by CodeRabbit
.envsupport, base-URL precedence, and grouped preflight readiness checks.nemo insights analysis/doctorflows with runtime resolution of agent/workspace/base-URL and improved target insights output handling.testbed restore --intofor fresh/empty single-workspace restores, plus direct-restore validation and fidelity guard workflow behavior.