default to writing insights to platform - #1054
Open
jeremysilva1098 wants to merge 3 commits into
Open
Conversation
Contributor
|
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 (14)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthrough
ChangesInsights persistence workflow
Sequence Diagram(s)sequenceDiagram
participant AnalystCLI
participant run_analyst
participant PlatformInsights
participant InsightsFileStore
AnalystCLI->>run_analyst: pass insights_output and local_only
run_analyst->>PlatformInsights: persist insight in platform mode
PlatformInsights-->>run_analyst: return platform insight
run_analyst->>InsightsFileStore: mirror platform record
InsightsFileStore-->>run_analyst: warn on mirror failure
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py (1)
339-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse concrete types for insight records.
The new storage API uses bare
dictandlist[dict]. Define anInsightRecordTypedDict, then use it for records and store methods.As per coding guidelines, “Prefer concrete type hints over string-based type hints.”
🤖 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 339 - 411, Define an InsightRecord TypedDict for the serialized insight fields used by _record_to_insight and _insight_to_record, then replace bare dict and list[dict] annotations with InsightRecord and list[InsightRecord] throughout InsightsFileStore and the conversion helpers. Keep optional or nullable fields aligned with the actual record values and avoid string-based type hints.Source: Coding guidelines
plugins/nemo-insights/tests/test_analyst_run.py (1)
24-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert both
local_onlymodes.The stub records
local_only, buttest_injected_client_is_used_and_closednever checks it. This file also has nolocal_only=Truecall. A regression could drop the forwarding or forceFalseand still pass. Assert the defaultFalsevalue and add a true-mode test withinsights_output.🤖 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_analyst_run.py` around lines 24 - 26, Update fake_make_backend and the analyst tests so local_only forwarding is verified in both modes: assert test_injected_client_is_used_and_closed uses the default False value, and add a test invoking the flow with local_only=True and an insights_output value, confirming the stub observes True and the injected client behavior remains correct.plugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.py (1)
62-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
local_onlywithinsights_outputbefore serialization.AnalyzeSpecacceptslocal_only=Truewithoutinsights_output, andAnalyzeJob.compileserializes the invalid spec. The error occurs later inmake_analyst_backend. Add cross-field validation at the spec or compile boundary and cover invalid compilation with a targeted job test.🤖 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/jobs/analyze.py` around lines 62 - 64, Add cross-field validation for AnalyzeSpec.local_only and insights_output so local_only=True without insights_output is rejected before serialization or backend creation. Enforce this at the AnalyzeSpec validation or AnalyzeJob.compile boundary, preserving valid configurations, and add a targeted job test confirming invalid compilation fails.
🤖 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-experimentalist/README.md`:
- Around line 45-48: Update the Experimentalist output documentation to state
that combining --local-only with --insights-file-output writes to the explicitly
provided path, while .nemo-optimizer/insights.yaml is used only when
--insights-file-output is omitted.
In `@plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py`:
- Around line 391-411: Update the store methods around write_records and merge
in plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py
lines 391-411 to serialize the complete read-modify-write merge transaction with
a file lock, and write via a temporary file followed by atomic replacement.
Update the local-only mutation flow around lines 577-612 to use this same locked
store transaction; both sites require changes so concurrent analysts cannot
overwrite each other’s records.
- Around line 385-389: Update AnalystBackend.read_records to validate that the
YAML document is a mapping, that insights is a non-null list, and that every
entry is a record before returning it; reject malformed shapes with a clear
validation error. In _mirror, catch these read/validation failures and emit a
mirror warning instead of allowing a successful platform write to fail.
---
Nitpick comments:
In `@plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py`:
- Around line 339-411: Define an InsightRecord TypedDict for the serialized
insight fields used by _record_to_insight and _insight_to_record, then replace
bare dict and list[dict] annotations with InsightRecord and list[InsightRecord]
throughout InsightsFileStore and the conversion helpers. Keep optional or
nullable fields aligned with the actual record values and avoid string-based
type hints.
In `@plugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.py`:
- Around line 62-64: Add cross-field validation for AnalyzeSpec.local_only and
insights_output so local_only=True without insights_output is rejected before
serialization or backend creation. Enforce this at the AnalyzeSpec validation or
AnalyzeJob.compile boundary, preserving valid configurations, and add a targeted
job test confirming invalid compilation fails.
In `@plugins/nemo-insights/tests/test_analyst_run.py`:
- Around line 24-26: Update fake_make_backend and the analyst tests so
local_only forwarding is verified in both modes: assert
test_injected_client_is_used_and_closed uses the default False value, and add a
test invoking the flow with local_only=True and an insights_output value,
confirming the stub observes True and the injected client behavior remains
correct.
🪄 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: 5ddd8ef8-54ea-4f62-8aab-51568c73b2d2
📒 Files selected for processing (13)
plugins/nemo-experimentalist/AGENTS.mdplugins/nemo-experimentalist/README.mdplugins/nemo-insights/README.mdplugins/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/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/testbed/adapters.pyplugins/nemo-insights/tests/test_analyst_run.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.pyplugins/nemo-insights/tests/testbed/test_adapters.py
Comment on lines
+45
to
+48
| documented trace, workspace, and output options. It persists an Insight on | ||
| Platform by default and reports its ID; `--insights-file-output` additionally | ||
| mirrors those rows into a local file, and `--local-only` writes the local | ||
| profile default, `.nemo-optimizer/insights.yaml`, and nothing else. The Experimentalist does not analyze traces, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the explicit local-only output path.
--local-only --insights-file-output <path> writes to <path>, not the profile default. State that .nemo-optimizer/insights.yaml is used only when --insights-file-output is omitted.
🤖 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-experimentalist/README.md` around lines 45 - 48, Update the
Experimentalist output documentation to state that combining --local-only with
--insights-file-output writes to the explicitly provided path, while
.nemo-optimizer/insights.yaml is used only when --insights-file-output is
omitted.
Comment on lines
+385
to
+389
| def read_records(self) -> list[dict]: | ||
| if not self.path.exists(): | ||
| return [] | ||
| raw = yaml.safe_load(self.path.read_text(encoding="utf-8")) or {} | ||
| return list(raw.get("insights", [])) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent malformed mirror files from failing completed platform writes.
read_records() raises AttributeError or TypeError for a YAML list, scalar, insights: null, or non-record entries. _mirror() does not catch those errors. The platform write can succeed, but the run then fails instead of reporting a mirror warning.
Validate the document shape. In mirror mode, convert these validation failures into warnings.
🤖 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 385 - 389, Update AnalystBackend.read_records to validate that the
YAML document is a mapping, that insights is a non-null list, and that every
entry is a record before returning it; reject malformed shapes with a clear
validation error. In _mirror, catch these read/validation failures and emit a
mirror warning instead of allowing a successful platform write to fail.
Comment on lines
+391
to
+411
| def write_records(self, records: list[dict]) -> None: | ||
| self.path.parent.mkdir(parents=True, exist_ok=True) | ||
| document = yaml.safe_load(self.path.read_text(encoding="utf-8")) if self.path.exists() else None | ||
| if not isinstance(document, dict): | ||
| document = {} | ||
| document["insights"] = records | ||
| self.path.write_text(yaml.safe_dump(document, sort_keys=False, allow_unicode=True), encoding="utf-8") | ||
|
|
||
| def merge(self, records: list[dict]) -> None: | ||
| """Upsert *records* by ``(workspace, id)`` into the existing document.""" | ||
| existing = self.read_records() | ||
| index = {(r.get("workspace"), r.get("id")): position for position, r in enumerate(existing)} | ||
| for record in records: | ||
| key = (record.get("workspace"), record.get("id")) | ||
| position = index.get(key) | ||
| if position is None: | ||
| index[key] = len(existing) | ||
| existing.append(record) | ||
| else: | ||
| existing[position] = record | ||
| self.write_records(existing) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize YAML read-modify-write operations.
Two analysts targeting one file can read the same state and then overwrite each other’s records. This loses local-only insights or mirrored platform records.
plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py#L391-L411: lock the complete merge transaction and write through a temporary file with atomic replacement.plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py#L577-L612: route local-only mutations through the same locked store transaction.
📍 Affects 1 file
plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py#L391-L411(this comment)plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py#L577-L612
🤖 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 391 - 411, Update the store methods around write_records and merge
in plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py
lines 391-411 to serialize the complete read-modify-write merge transaction with
a file lock, and write via a temporary file followed by atomic replacement.
Update the local-only mutation flow around lines 577-612 to use this same locked
store transaction; both sites require changes so concurrent analysts cannot
overwrite each other’s records.
Contributor
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli.py`:
- Around line 151-153: Update the mirror setup in the analysis output flow so a
failure from analysis.insights_output.parent.mkdir does not abort run_analyst or
platform persistence. Catch the local directory-creation error and continue with
mirror output disabled, while preserving normal mirror behavior when setup
succeeds.
🪄 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: 769f8bc7-900f-4d9f-8223-456f43463ba7
📒 Files selected for processing (10)
plugins/nemo-experimentalist/AGENTS.mdplugins/nemo-experimentalist/README.mdplugins/nemo-insights/README.mdplugins/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/run.pyplugins/nemo-insights/src/nemo_insights_plugin/cli.pyplugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.pyplugins/nemo-insights/tests/test_cli_profile.pyplugins/nemo-insights/tests/test_periodic_analysis.py
💤 Files with no reviewable changes (1)
- plugins/nemo-insights/src/nemo_insights_plugin/jobs/analyze.py
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/nemo-experimentalist/AGENTS.md
- plugins/nemo-experimentalist/README.md
- plugins/nemo-insights/tests/test_periodic_analysis.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/run.py
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/deps.py
- plugins/nemo-insights/README.md
- plugins/nemo-insights/src/nemo_insights_plugin/analyst/analyst_backend.py
Comment on lines
+151
to
+153
| if analysis.insights_output is not None: | ||
| analysis.insights_output.parent.mkdir(parents=True, exist_ok=True) | ||
| typer.echo(f"Insights file (mirror of the platform): {analysis.insights_output}", err=True) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep mirror setup from blocking platform persistence.
If the mirror parent cannot be created, parent.mkdir(...) raises before run_analyst. The outer handler then exits with code 1, so the platform write never occurs. Catch this local setup failure and continue without the mirror, or move directory setup into the backend’s warning-only mirror path.
🤖 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 151 -
153, Update the mirror setup in the analysis output flow so a failure from
analysis.insights_output.parent.mkdir does not abort run_analyst or platform
persistence. Catch the local directory-creation error and continue with mirror
output disabled, while preserving normal mirror behavior when setup succeeds.
Signed-off-by: Jeremy Silva <jesilva@nvidia.com>
Insights always go to the platform now; --insights-file-output only ever mirrors what was stored. Review concluded that a fully local persistence mode is not something users should be choosing between. LocalAnalystBackend stays as maintainer plumbing for the insights testbed, which treats the YAML as the artifact under test (checked in, hashed, diffed) and analyzes subjects such as glamr that expose Intake but not the Insights plugin. No CLI flag reaches it. Signed-off-by: Jeremy Silva <jesilva@nvidia.com>
…guide The flag table still described the removed behavior, where a discovered profile diverted Insight writes to a local file instead of the platform. Signed-off-by: Jeremy Silva <jesilva@nvidia.com>
jeremysilva1098
force-pushed
the
ASE-728-default-write-insights/jesilva
branch
from
August 4, 2026 23:33
4d52d69 to
d689d1e
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
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
1 participant
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.
Summary
nemo agents analyst run (and its alias nemo insights analyze) now always persists insights to the platform. Previously, discovering an optimizer.yaml profile silently diverted all insight writes to a local /.nemo-optimizer/insights.yaml and skipped the platform entirely — so the common flag-free invocation from an agent directory never populated the Insights API or the Studio insights tab.
--insights-file-output is now purely additive: it mirrors what the platform stored into a local file. There is no longer any flag combination that keeps insights off the platform.
Behavior
┌─────────────────────────────────────────────┬────────────────────┬────────────┐
│ Command │ Platform │ Local file │
├─────────────────────────────────────────────┼────────────────────┼────────────┤
│ analyst run │ ✅ │ — │
├─────────────────────────────────────────────┼────────────────────┼────────────┤
│ analyst run --insights-file-output out.yaml │ ✅ (written first) │ ✅ mirror │
└─────────────────────────────────────────────┴────────────────────┴────────────┘
Mirroring: platform is the source of truth
The platform is written first, and the file receives the rows it returned — platform-assigned ids, name, and timestamps included.
That ordering is what makes the mirror correct across runs. The analyst's change-set keys updates by insight id (AnalystResult.updated_insights[].id), and list_insights reads from the platform. Had the two stores been written independently, each would mint its own ids, every subsequent update would carry an id the file had never seen, and the mirror would degrade into a create-only log that silently diverges. test_mirror_updates_match_platform_ids_across_runs guards this directly.
A mirror write that fails (unwritable path, bad permissions) is reported as a warning on the run report rather than exiting non-zero — the platform writes have already landed, so failing the run would misreport what happened.
Changes
Note on LocalAnalystBackend
The class and the local_only argument on make_analyst_backend / run_analyst are retained as maintainer plumbing for the insights testbed only — no CLI flag or job field reaches them, and all three docstrings say so.
The testbed genuinely needs file-only persistence: it treats the YAML as the artifact under test (checked into testbed/insights/.yaml, hashed as insights_sha256, diffed across runs, promoted by atomic swap), --update-insights seeds a run by listing priors from that file, and the glamr subject targets an external Intake behind basic auth that does not host the Insights plugin API at all.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation