fix(experimentalist): mirror insights auth handling in experimentalist - #1053
Merged
Merged
Conversation
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Contributor
📝 WalkthroughWalkthroughChangesPlatform client flow
Sequence Diagram(s)sequenceDiagram
participant Doctor
participant make_client
participant AsyncNeMoPlatform
Doctor->>make_client: construct client with effective base URL
make_client->>AsyncNeMoPlatform: create direct or authenticated client
AsyncNeMoPlatform-->>Doctor: initialized client
Doctor->>AsyncNeMoPlatform: close client
AsyncNeMoPlatform-->>Doctor: cleanup result
Doctor-->>Doctor: append bootstrap check to report
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/client.py (1)
47-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify the complete loopback range.
127.0.0.2is loopback but bypasses direct mode. It triggers authentication discovery and can apply local OAuth configuration to a local target. Classify IP addresses with loopback semantics, not fixed literals.
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/client.py#L47-L52: detect all loopback IP addresses before authentication discovery.plugins/nemo-experimentalist/tests/test_client.py#L25-L26: add127.0.0.2to the direct-mode cases.🤖 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/src/nemo_experimentalist_plugin/client.py` around lines 47 - 52, Update the client host classification before discover_nmp_config in client.py lines 47-52 to recognize all loopback IP addresses, including 127.0.0.2, rather than relying only on fixed LOOPBACK_HOSTS entries; preserve direct mode for loopback targets. Add 127.0.0.2 to the direct-mode cases in tests/test_client.py lines 25-26.
🤖 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.
Outside diff comments:
In `@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/client.py`:
- Around line 47-52: Update the client host classification before
discover_nmp_config in client.py lines 47-52 to recognize all loopback IP
addresses, including 127.0.0.2, rather than relying only on fixed LOOPBACK_HOSTS
entries; preserve direct mode for loopback targets. Add 127.0.0.2 to the
direct-mode cases in tests/test_client.py lines 25-26.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1940adc-638b-4600-8c47-c39a843f4b71
📒 Files selected for processing (4)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/cli.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/client.pyplugins/nemo-experimentalist/tests/test_cli_profile.pyplugins/nemo-experimentalist/tests/test_client.py
Contributor
|
jeremysilva1098
approved these changes
Aug 3, 2026
sklinglernv
added a commit
that referenced
this pull request
Aug 4, 2026
Two commits on main landed in files this branch rewrites: - #1047 (eval-author: narrate insight-mode progress via RunReporter) added `nemo_experimentalist_plugin.experimentalist.reporting` to the Eval Author boundary ratchet, which this branch had split into `_SHARED_LAYER_A` and `_BORROWED_BEHAVIOUR`. Resolved by keeping the split and filing the new import under borrowed behaviour: RunReporter is reused behaviour, not entity contract, so it belongs on the list that may only shrink. - #1053 (experimentalist: honor remote auth discovery) auto-merged. Also updates docs/agents/insight-driven-optimization.mdx, new on main, which documented the pre-rename variables and model defaults this branch removes. Signed-off-by: Severin Klingler <sklingler@nvidia.com>
9 tasks
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
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 experimentalist doctorconstruct and close the same Platform client used byrunReproduction
~/.config/nmp/config.yamlOAuth context for one Platform deployment.--base-url.doctorreports the raw health probe as healthy.rununconditionally passes the unrelated config path toAsyncNeMoPlatform, which attempts OAuth token refresh and can fail before the experiment starts (for example,httpx.UnsupportedProtocolfrom an invalid refresh endpoint).Root cause
Experimentalist classified all non-loopback remote URLs with a local config file as authenticated. Insights already avoids this by querying
/apis/auth/discoveryand only supplyingconfig_pathwhen the target reportsauth_enabled=true.Doctor only exercised a raw health URL, so it did not cover the SDK/auth bootstrap path used by a run.
Fix
Experimentalist now mirrors the Insights client policy:
Doctor additionally constructs and closes the same client helper used by
run. Initialization and cleanup errors are surfaced without echoing potentially sensitive exception details.Validation
pytest plugins/nemo-experimentalist/tests -q— 576 passedty checkfor changed production files and new client tests — passedpre-commit run -a— all hooks passed, including repository type checksNMP_CONFIG_FILEworkaround — passed, including the new client-bootstrap checkLimitations
The new doctor check verifies SDK construction and cleanup through the effective auth path. It does not add a workspace-specific API query; the existing Platform reachability probe remains responsible for network readiness.
Summary by CodeRabbit
New Features
Bug Fixes