feat(insights): Move the insights-powered analysis into the platform repo - #679
Conversation
Migrate telemetry analysis, persistent insight APIs, SDK resources, scheduling, and the research-agent example without installing the plugin in default or all-package environments. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Migrate pinned Intake replay, Tau2 trace generation, artifact round trips, and opt-in CI without Harbor adapters or vendored agent runtimes. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
|
Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Keep default test discovery independent of optional dependencies while adding dedicated plugin coverage, and remove user-controlled values from exception logs. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Document the optional dependency group required for tests and remove stale optimizer terminology from testbed help. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Avoid applying an unrelated cached OAuth context when the target platform reports authentication disabled, allowing local analyst runs against freeplay telemetry. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
|
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)
📝 WalkthroughWalkthroughAdds the ChangesInsights Plugin
Testbed and CI
Workspace Tooling
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
plugins/nemo-insights/examples/research-agent/README.md (1)
115-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a "Next Steps" section with cross-links.
Per coding guidelines, docs should "Include 'Next Steps' section at the end with cross-links to related documentation content" — this file ends at "Gotchas" with none.
🤖 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/examples/research-agent/README.md` around lines 115 - 119, Add a “Next Steps” section after the existing “Gotchas” section in the research-agent README, including cross-links to relevant related documentation using the repository’s established documentation link conventions.Source: Coding guidelines
plugins/nemo-insights/README.md (1)
1-62: 📐 Maintainability & Code Quality | 🔵 TrivialMissing prerequisites/Next Steps, and CLI+SDK mixed with reference content.
No prerequisites list up top, no closing "Next Steps" cross-links, and the SDK section only names resources without a runnable example alongside the CLI one.
As per coding guidelines: "Always list prerequisites at the top of documentation pages before other content", "Include 'Next Steps' section at the end with cross-links to related documentation content", and "Provide both Python SDK and CLI examples in tab-sets for consistency and to support multiple user workflows."
🤖 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 1 - 62, Update the NeMo Insights README by adding a prerequisites section at the top, separating the CLI and SDK guidance into consistent tabbed examples with a runnable Python SDK example alongside the existing CLI commands, and ending with a Next Steps section containing cross-links to related documentation. Preserve the existing installation, configuration, development, and testbed content while organizing the reference material around these additions.Source: Coding guidelines
plugins/nemo-insights/src/nemo_insights_plugin/cli.py (1)
62-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
base_urlOption block across 4 commands.Same
os.environ.get(NMP_BASE_URL, DEFAULT_BASE_URL)+envvarpattern repeated in analyze/enable/disable/status.♻️ Proposed fix: shared Option factory
+def _base_url_option() -> str: + return typer.Option( + os.environ.get("NMP_BASE_URL", DEFAULT_BASE_URL), + "--base-url", + help="Base URL of the running NMP instance.", + envvar="NMP_BASE_URL", + )Then replace each inline
base_url: str = typer.Option(...)withbase_url: str = _base_url_option().Also applies to: 121-126, 152-157, 183-188
🤖 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/src/nemo_insights_plugin/cli.py` around lines 62 - 67, Introduce a shared _base_url_option() factory encapsulating the existing NMP_BASE_URL, DEFAULT_BASE_URL, CLI flag, help text, and envvar configuration, then replace the duplicated base_url typer.Option definitions in the analyze, enable, disable, and status commands with calls to that factory.plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py (1)
193-231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
list_span_groupshard-caps at 1000 groups regardless oflimit, contradicting its own guidance.The docstring tells the model a truncated survey can be fixed by narrowing the filter "or raise the limit," but
page_size = max(1, min(limit, 1000))caps at 1000 and only page 1 is ever fetched — raisinglimitpast 1000 has no effect. Either drain multiple pages (likelist_spans/list_annotationsdo via_drain) up tolimit, or clarify the docstring that 1000 is a hard ceiling.🤖 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/src/nemo_insights_plugin/analyst/analyst_backend.py` around lines 193 - 231, Update list_span_groups so limits above 1000 can retrieve additional group pages: use the existing pagination-draining approach from list_spans/list_annotations, fetching successive pages until limit groups are collected or no more results remain. Preserve the current filtering, sorting, response shape, and truncated semantics while ensuring the requested limit is honored.plugins/nemo-insights/testbed/ingest.py (1)
66-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClient-lifecycle boilerplate duplicated 4x.
ensure_workspace,ensure_experiment_group,create_experiment,poll_visibleall repeat the "own-or-reuse httpx.Client, close infinally" pattern (two different local-variable names for the same idea:active_clientvs. reassigningclient). Worth extracting into a small helper/contextmanager to reduce duplication and unify naming.🤖 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/testbed/ingest.py` around lines 66 - 146, The HTTP client ownership and cleanup logic is duplicated across ensure_workspace, ensure_experiment_group, create_experiment, and poll_visible. Extract a small context manager/helper that yields either the injected client or a newly created client and closes only newly owned clients, then update all four functions to use it with consistent client naming while preserving their existing request and error behavior.plugins/nemo-insights/tests/test_sdk_entity_hydration.py (1)
43-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert all metadata in the resource-wrapper tests.
These tests only check subsets, so
_analysis_config_page_from_responseor_insight_from_responsecould drop timestamps, updater fields, or_db_versionunnoticed. Reuse a helper that asserts every_METADATAfield.🤖 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/test_sdk_entity_hydration.py` around lines 43 - 88, Extend test_analysis_config_page_restores_item_metadata and test_insight_response_restores_all_store_metadata to validate every field in _METADATA, reusing an existing metadata assertion helper if available. Ensure the checks cover timestamps, updater fields, _db_version, and all other metadata fields rather than only id, created_by, and parent..github/workflows/insights-testbed.yml (1)
1-225: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit least-privilege
permissionstoplan/stack-checkand a workflow-level default.Both jobs rely on implicit default
GITHUB_TOKENpermissions (zizmor: excessive-permissions), which can be broader than needed org-wide. Addpermissions: {}at workflow level andpermissions: { contents: read }(or none) per job, matching whatplugin-tests/analyzealready do.Also applies to: 63-78, 80-100
🤖 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 @.github/workflows/insights-testbed.yml around lines 1 - 225, Add workflow-level permissions: {} near the top of insights-testbed, then explicitly set permissions: { contents: read } or permissions: {} on the plan and stack-check jobs. Preserve the existing narrower permissions on plugin-tests, produce, and analyze.Source: Linters/SAST tools
🤖 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 @.github/workflows/insights-testbed.yml:
- Around line 54-55: Update every actions/checkout step in the workflow,
including the steps near the referenced checkout blocks, to set
persist-credentials to false in their with configuration. Preserve each step’s
existing paths and other checkout options.
In `@plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py`:
- Around line 351-362: Update the updated-insights loop in the analyst backend
to validate or fetch each insight ID before reporting it as updated, including
when upd.trace_refs is empty. Reuse the existing store lookup or validation path
used by LocalAnalystBackend.persist_result, while preserving the
InsightNotFoundError skip message and continuing to use _add_trace_refs for
non-empty trace_refs.
- Around line 94-109: Update _merge_datetime_lower_bound to parse the existing
gte value and compare datetime objects rather than raw strings, handling
equivalent ISO representations such as Z suffixes and differing precision
correctly. Preserve the current bound when it is already at or after since;
otherwise set gte to since.isoformat().
In `@plugins/nemo-insights/src/nemo_insights_plugin/cli.py`:
- Line 21: Remove the stray TODO comment about remote train/validation datasets
and remote experiment mode from the CLI module; it is unrelated to the telemetry
analysis CLI and should not remain after the NeMo-Optimizer migration.
- Around line 202-203: Guard each entry point’s loading and registration in the
CLI construction flow around get_cli(), so an exception from entry_point.load()
or the resulting command setup is isolated to that plugin. Skip the malformed
entry point and allow the main insights CLI, including analyze and analysis, to
initialize normally; preserve successful registration for valid entry points.
In `@plugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.py`:
- Around line 101-112: The fallback in the analyze job environment construction
currently embeds the raw INFERENCE_API_KEY in the job spec. Update the `elif
inference_api_key := os.environ.get("INFERENCE_API_KEY")` path to prevent
plaintext secrets from being submitted, such as restricting it to an explicitly
non-production/local mode and emitting an appropriate warning; retain the
`canonical.inference_api_key_secret_name` `from_secret` path unchanged.
In `@plugins/nemo-insights/testbed/eval/prep.py`:
- Around line 22-27: Update repoint_judge so each re.subn call uses a callable
replacement that returns the complete assignment string, preventing judge_model
backslashes from being interpreted as replacement escapes. Preserve the existing
JUDGE_VARS iteration and total replacement count behavior.
In `@plugins/nemo-insights/testbed/eval/run_subjects.py`:
- Around line 53-57: Update the subprocess retry handling around
CalledProcessError in the subject evaluation flow to inspect the captured
failure status/output and retry only when the failure is confirmed as HTTP 429
rate limiting. Immediately re-raise authentication, configuration, data, and
other non-429 failures without cooling down or rerunning the command; preserve
the existing retry count and delay for confirmed rate-limit failures.
In `@plugins/nemo-insights/testbed/ingest.py`:
- Around line 60-63: Update mint_agent_id to generate its timestamp using UTC
rather than local time, matching the timestamp conventions in the plugin’s other
utilities. Preserve the existing YYYYMMDD-HHMMSS formatting and random suffix.
In `@plugins/nemo-insights/testbed/README.md`:
- Around line 205-209: Correct the repository slug in all three GitHub Actions
links in the README run list by replacing the literal space in “nemo-insights
plugin” with the valid slug separator. Preserve each run ID and the surrounding
descriptions unchanged.
In `@plugins/nemo-insights/testbed/reingest.py`:
- Around line 557-597: Validate every workspace’s manifest counts against the
preloaded spans, annotations, and evaluator_results collections before entering
the API-writing loop or calling ensure_workspace. Reject any mismatch, including
missing or truncated JSONL files that produce empty lists, and only proceed with
workspace creation and ingestion after all workspaces pass validation.
- Around line 700-713: Update _diff_collection’s per-key comparison to
distinguish an absent key from a present key whose value is None, using
membership checks alongside value comparison. Ensure mismatches are reported
when either document contains the key and the other does not, while preserving
the existing comparison and reporting behavior for keys present in both
documents.
- Around line 626-630: Update the healing logic around evaluator_result_count to
compare stable (session_id, span_id, name) identities, not only aggregate
counts. Validate that every existing evaluator result belongs to the expected
bundle, reject unknown rows, and only enable full-bundle reposting when the
existing rows are a valid expected subset.
- Around line 839-848: Update the re-export call in the test flow around
ingest_bundle and export.export_workspaces to pass the manifest’s time-bound
value instead of since=None. Reuse the existing manifest symbol and preserve the
current workspace and comparison logic.
---
Nitpick comments:
In @.github/workflows/insights-testbed.yml:
- Around line 1-225: Add workflow-level permissions: {} near the top of
insights-testbed, then explicitly set permissions: { contents: read } or
permissions: {} on the plan and stack-check jobs. Preserve the existing narrower
permissions on plugin-tests, produce, and analyze.
In `@plugins/nemo-insights/examples/research-agent/README.md`:
- Around line 115-119: Add a “Next Steps” section after the existing “Gotchas”
section in the research-agent README, including cross-links to relevant related
documentation using the repository’s established documentation link conventions.
In `@plugins/nemo-insights/README.md`:
- Around line 1-62: Update the NeMo Insights README by adding a prerequisites
section at the top, separating the CLI and SDK guidance into consistent tabbed
examples with a runnable Python SDK example alongside the existing CLI commands,
and ending with a Next Steps section containing cross-links to related
documentation. Preserve the existing installation, configuration, development,
and testbed content while organizing the reference material around these
additions.
In `@plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py`:
- Around line 193-231: Update list_span_groups so limits above 1000 can retrieve
additional group pages: use the existing pagination-draining approach from
list_spans/list_annotations, fetching successive pages until limit groups are
collected or no more results remain. Preserve the current filtering, sorting,
response shape, and truncated semantics while ensuring the requested limit is
honored.
In `@plugins/nemo-insights/src/nemo_insights_plugin/cli.py`:
- Around line 62-67: Introduce a shared _base_url_option() factory encapsulating
the existing NMP_BASE_URL, DEFAULT_BASE_URL, CLI flag, help text, and envvar
configuration, then replace the duplicated base_url typer.Option definitions in
the analyze, enable, disable, and status commands with calls to that factory.
In `@plugins/nemo-insights/testbed/ingest.py`:
- Around line 66-146: The HTTP client ownership and cleanup logic is duplicated
across ensure_workspace, ensure_experiment_group, create_experiment, and
poll_visible. Extract a small context manager/helper that yields either the
injected client or a newly created client and closes only newly owned clients,
then update all four functions to use it with consistent client naming while
preserving their existing request and error behavior.
In `@plugins/nemo-insights/tests/test_sdk_entity_hydration.py`:
- Around line 43-88: Extend test_analysis_config_page_restores_item_metadata and
test_insight_response_restores_all_store_metadata to validate every field in
_METADATA, reusing an existing metadata assertion helper if available. Ensure
the checks cover timestamps, updater fields, _db_version, and all other metadata
fields rather than only id, created_by, and parent.
🪄 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: f0ac9b25-94ff-45f3-9da5-77b4b3474583
⛔ Files ignored due to path filters (2)
plugins/nemo-insights/testbed/state.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (83)
.github/actions/insights-intake-stack/action.yml.github/actions/insights-testbed-prep/action.yml.github/workflows/insights-testbed.ymlplugins/nemo-insights/README.mdplugins/nemo-insights/examples/insight-example.yamlplugins/nemo-insights/examples/research-agent/.gitignoreplugins/nemo-insights/examples/research-agent/README.mdplugins/nemo-insights/examples/research-agent/main.pyplugins/nemo-insights/examples/research-agent/pyproject.tomlplugins/nemo-insights/examples/research-agent/tests/test_analyst_e2e.pyplugins/nemo-insights/examples/research-agent/workflow.ymlplugins/nemo-insights/pyproject.tomlplugins/nemo-insights/src/nemo_insights_plugin/_perms.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/agent.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/deps.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/functions/annotations.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/functions/insights.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/functions/spans.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/observability.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/result.pyplugins/nemo-insights/src/nemo_insights_plugin/analyst/run.pyplugins/nemo-insights/src/nemo_insights_plugin/authz.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/client.pyplugins/nemo-insights/src/nemo_insights_plugin/config.pyplugins/nemo-insights/src/nemo_insights_plugin/controller.pyplugins/nemo-insights/src/nemo_insights_plugin/entities.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/bridge.pyplugins/nemo-insights/src/nemo_insights_plugin/py.typedplugins/nemo-insights/src/nemo_insights_plugin/schedule.pyplugins/nemo-insights/src/nemo_insights_plugin/schema.pyplugins/nemo-insights/src/nemo_insights_plugin/sdk.pyplugins/nemo-insights/src/nemo_insights_plugin/sdk_resources/_entity.pyplugins/nemo-insights/src/nemo_insights_plugin/sdk_resources/analysis_configs.pyplugins/nemo-insights/src/nemo_insights_plugin/sdk_resources/insights.pyplugins/nemo-insights/src/nemo_insights_plugin/service.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/__main__.pyplugins/nemo-insights/testbed/adapters.pyplugins/nemo-insights/testbed/artifact.pyplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/eval/plan.pyplugins/nemo-insights/testbed/eval/prep.pyplugins/nemo-insights/testbed/eval/run_subjects.pyplugins/nemo-insights/testbed/eval/stack.pyplugins/nemo-insights/testbed/export.pyplugins/nemo-insights/testbed/ingest.pyplugins/nemo-insights/testbed/otlp_build.pyplugins/nemo-insights/testbed/otlp_ingest.pyplugins/nemo-insights/testbed/publish.pyplugins/nemo-insights/testbed/registry.pyplugins/nemo-insights/testbed/reingest.pyplugins/nemo-insights/testbed/release.pyplugins/nemo-insights/testbed/runstore.pyplugins/nemo-insights/testbed/summary.pyplugins/nemo-insights/testbed/tau2run.pyplugins/nemo-insights/testbed/testbeds.tomlplugins/nemo-insights/testbed/timeparse.pyplugins/nemo-insights/tests/test_client.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/test_sdk_entity_hydration.pyplugins/nemo-insights/tests/testbed/conftest.pyplugins/nemo-insights/tests/testbed/test_adapters.pyplugins/nemo-insights/tests/testbed/test_artifact.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_eval_scripts.pyplugins/nemo-insights/tests/testbed/test_export.pyplugins/nemo-insights/tests/testbed/test_ingest.pyplugins/nemo-insights/tests/testbed/test_otlp_build.pyplugins/nemo-insights/tests/testbed/test_otlp_ingest.pyplugins/nemo-insights/tests/testbed/test_publish.pyplugins/nemo-insights/tests/testbed/test_registry.pyplugins/nemo-insights/tests/testbed/test_reingest.pyplugins/nemo-insights/tests/testbed/test_release.pyplugins/nemo-insights/tests/testbed/test_runstore.pyplugins/nemo-insights/tests/testbed/test_summary.pyplugins/nemo-insights/tests/testbed/test_tau2run.pyplugins/nemo-insights/tests/testbed/test_timeparse.pypyproject.tomltests/discovery_exclusions.pytools/lint/lint-python-types.sh
Use the standard workspace dependency model while keeping Insights outside the default and enabled plugin groups. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Harden workflow checkouts and correct analyst timestamp, persistence, and testbed utility edge cases identified during PR review. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Wire the testbed jobs to the inference credentials available across NeMo repositories. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Trigger inference only when the run-insights label is applied so later commits do not repeat costly analysis. Signed-off-by: Nico Tonozzi <ntonozzi@nvidia.com>
Move the analyzer out of https://github.com/NVIDIA-dev/NeMo-Optimizer into the main platform repo. The plugin is not enabled or installed by default.
This move will let us develop the analyzer more consistently with the rest of the platform and platform plugins and make it easier to release as a research preview when we are ready.
It includes the test bed, which is the way we evaluate the quality of the analyzer.
This does not move the rest of the optimization stack yet. In a future PR to the Optimizer plugin, we will change the optimizer to depend on the insights-driven analysis plugin in this repository.
There are no substantive changes here, this is just a move from the other repository.
Summary by CodeRabbit
nemo insightsCLI to run analysis and manage enablement/status and reporting.