feat(cli): improve multi-context UX and Config.write API - #1072
Open
anastasia-nesterenko wants to merge 1 commit into
Open
feat(cli): improve multi-context UX and Config.write API#1072anastasia-nesterenko wants to merge 1 commit into
anastasia-nesterenko wants to merge 1 commit into
Conversation
Signed-off-by: anastasia-nesterenko <anesterenko@nvidia.com>
Contributor
Contributor
📝 WalkthroughWalkthroughThe PR adds named-context configuration writes, context-selection precedence, write-result reporting, shared CLI help, expanded tests, and updated configuration documentation. ChangesContext-aware configuration
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigCLI
participant Config
participant ConfigFile
User->>ConfigCLI: Invoke config command with optional --context
ConfigCLI->>Config: Resolve runtime or persisted context
Config->>ConfigFile: Read and write selected context
Config-->>ConfigCLI: Return context name and creation status
ConfigCLI-->>User: Report context state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 4
🧹 Nitpick comments (1)
packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py (1)
597-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a concrete
Configbound.
Configis defined before this declaration. Replacebound="Config"withbound=Config.🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py` around lines 597 - 601, Update the _T TypeVar declaration used by ConfigWriteResult to bind directly to the already-defined Config class, replacing the string forward reference with the concrete Config symbol.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 `@docs/cli/configuration.mdx`:
- Around line 7-77: Split the connection workflows from the configuration
reference: move the remote and multi-deployment setup instructions, including
authentication, context switching, and verification commands, into a dedicated
HOW-TO page. Keep this reference page focused on configuration fields,
precedence, telemetry, and completion content; add a prerequisites section at
the top and a Next Steps section at the end, following the documentation
structure guidelines.
In `@packages/nemo_platform_ext/README.md`:
- Line 89: Update the README command examples so creating the production context
does not first modify the current context; remove the preceding context-setting
command or present it as an alternative to the production creation command.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config.py`:
- Around line 137-139: Update the config command help text describing
current-context selection to state that the first context becomes current only
when no current context is already saved, while preserving that additional
contexts do not switch the current context and can be selected with --activate
or use-context.
In `@packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py`:
- Around line 379-381: Update the missing-config branch in the config creation
flow to preserve the context selected by Config.create(): prefer
config.current_context when assigning context_name, and only fall back to
DEFAULT_CONTEXT when it is unset. Add a regression test covering a first write
with NMP_CURRENT_CONTEXT set and no explicit context, verifying the
configuration is persisted under and resolves through that environment-selected
context.
---
Nitpick comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py`:
- Around line 597-601: Update the _T TypeVar declaration used by
ConfigWriteResult to bind directly to the already-defined Config class,
replacing the string forward reference with the concrete Config symbol.
🪄 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: c68cd8fb-a9c1-431a-8522-4ecae96a1756
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/auth.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/config_help.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/cli/commands/manifest_registry.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/config/config.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_config.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/config/test_config.pyis excluded by!sdk/**
📒 Files selected for processing (11)
docs/cli/configuration.mdxdocs/get-started/setup.mdxpackages/nemo_platform_ext/README.mdpackages/nemo_platform_ext/docs/runbook.mdpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/auth.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config_help.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/manifest_registry.pypackages/nemo_platform_ext/src/nemo_platform_ext/config/config.pypackages/nemo_platform_ext/tests/cli/commands/test_config.pypackages/nemo_platform_ext/tests/config/test_config.py
Comment on lines
+7
to
+77
| ## Connect to a remote deployment | ||
|
|
||
| The quickest way to connect to an existing deployment is: | ||
| Save the deployment URL in the current CLI context: | ||
|
|
||
| ```bash | ||
| # Set base URL and authenticate in one command | ||
| nemo auth login --base-url https://nmp.example.com | ||
| nemo config set --base-url https://nmp.example.com | ||
| ``` | ||
|
|
||
| Verify the saved URL before running other commands: | ||
|
|
||
| ```bash | ||
| nemo config view | ||
| ``` | ||
|
|
||
| If the deployment requires authentication, log in after configuring the URL: | ||
|
|
||
| ```bash | ||
| nemo auth login | ||
| ``` | ||
|
|
||
| During interactive onboarding, `nemo setup` also offers to connect to a remote deployment when the currently configured platform is unreachable. It verifies and saves the new URL in the active context, then runs the same authentication flow. | ||
| You can also run `nemo setup`. During interactive onboarding, setup offers to connect to a remote deployment when the currently configured platform is unreachable. It verifies and saves the URL, then runs the same authentication flow. | ||
|
|
||
| To configure a named context: | ||
| ## Connect to multiple deployments | ||
|
|
||
| A context is a user-defined name for a set of connection settings. The name has no built-in meaning: names such as `staging`, `production`, and `local` are labels you choose. Each context keeps its own URL, authentication credentials, workspace, and output preferences. | ||
|
|
||
| Create a context for each remote deployment. Use `--activate` to make one of them the current context: | ||
|
|
||
| ```bash | ||
| nemo config set --context staging --base-url https://nmp.staging.example.com | ||
| nemo config set --context production --base-url https://nmp.example.com --activate | ||
| ``` | ||
|
|
||
| The first context becomes current automatically. After a current context exists, creating another named context does not switch it. Use `--activate` when creating the context or run `nemo config use-context <name>` later to switch. | ||
|
|
||
| For authenticated deployments, log in to each context separately so its credentials are saved with the correct connection: | ||
|
|
||
| ```bash | ||
| nemo --context prod auth login --base-url https://nmp.prod.example.com | ||
| nemo auth login --context staging | ||
| nemo auth login --context production | ||
| ``` | ||
|
|
||
| ## Configuration File | ||
| Switch the current context at any time: | ||
|
|
||
| ```bash | ||
| nemo config use-context staging | ||
| nemo config use-context production | ||
| ``` | ||
|
|
||
| Confirm which context is current and inspect all saved connections: | ||
|
|
||
| ```bash | ||
| nemo config current-context | ||
| nemo config view --all-contexts | ||
| ``` | ||
|
|
||
| To target a context for only one command without changing the current context, use the global `--context` option: | ||
|
|
||
| ```bash | ||
| nemo --context staging models list | ||
| ``` | ||
|
|
||
| A local platform is configured the same way and can coexist with any number of remote contexts: | ||
|
|
||
| ```bash | ||
| nemo config set --context local --base-url http://localhost:8080 | ||
| ``` | ||
|
|
||
| <Note> | ||
| `NMP_BASE_URL` and `NMP_CURRENT_CONTEXT` override saved configuration. If switching contexts does not change the target deployment, unset those variables or update them for the current shell. | ||
| </Note> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split workflow and reference content.
This page mixes HOW-TO workflows with REFERENCE content. Move connection workflows to a HOW-TO page. Keep configuration fields, precedence, telemetry, and completion in REFERENCE pages. Add prerequisites and a Next Steps section.
As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant” and “Always list prerequisites at the top of documentation pages before other content.”
🤖 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/cli/configuration.mdx` around lines 7 - 77, Split the connection
workflows from the configuration reference: move the remote and multi-deployment
setup instructions, including authentication, context switching, and
verification commands, into a dedicated HOW-TO page. Keep this reference page
focused on configuration fields, precedence, telemetry, and completion content;
add a prerequisites section at the top and a Next Steps section at the end,
following the documentation structure guidelines.
Source: Coding guidelines
| ```bash | ||
| nemo config set --base-url https://nmp.example.com | ||
| nemo config set --context prod --base-url https://nmp.prod.example.com --activate | ||
| nemo config set --context production --base-url https://nmp.example.com --activate |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not modify the current context before creating production.
Line 88 and Line 89 run sequentially. The first command can overwrite the current context URL before the second command creates production. Remove the first command or present the commands as alternatives.
🤖 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 `@packages/nemo_platform_ext/README.md` at line 89, Update the README command
examples so creating the production context does not first modify the current
context; remove the preceding context-setting command or present it as an
alternative to the production creation command.
Source: Coding guidelines
Comment on lines
+137
to
+139
| If no config file exists, creates one. Without --context, the context is named 'default'. | ||
| The first context becomes current automatically. Creating additional contexts | ||
| does not switch the current context; use --activate or use-context. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the unset-current-context behavior.
Config.write_with_result at Line 394 sets the saved current context whenever it is unset. An existing config can contain contexts with no saved current context. This help text says only the first context activates. State the actual condition.
Proposed text
- The first context becomes current automatically. Creating additional contexts
- does not switch the current context; use --activate or use-context.
+ The first context becomes current automatically. If the saved config has no
+ current context, the next written context becomes current. Otherwise, creating
+ additional contexts do not switch the current context; use --activate or use-context.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| If no config file exists, creates one. Without --context, the context is named 'default'. | |
| The first context becomes current automatically. Creating additional contexts | |
| does not switch the current context; use --activate or use-context. | |
| If no config file exists, creates one. Without --context, the context is named 'default'. | |
| The first context becomes current automatically. If the saved config has no | |
| current context, the next written context becomes current. Otherwise, creating | |
| additional contexts do not switch the current context; use --activate or use-context. |
🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config.py`
around lines 137 - 139, Update the config command help text describing
current-context selection to state that the first context becomes current only
when no current context is already saved, while preserving that additional
contexts do not switch the current context and can be selected with --activate
or use-context.
Comment on lines
379
to
381
| else: | ||
| config = cls.create(path, ConfigFile()) | ||
| context_name = context_name or DEFAULT_CONTEXT |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor NMP_CURRENT_CONTEXT on a first write.
If no explicit context is supplied and the config file is missing, Config.create() loads NMP_CURRENT_CONTEXT, but Line 381 discards it. The write persists default; later resolution selects the environment context and fails because that context was not created. Select config.current_context before DEFAULT_CONTEXT. Add a fresh-file environment-override regression test.
Proposed fix
else:
config = cls.create(path, ConfigFile())
- context_name = context_name or DEFAULT_CONTEXT
+ context_name = context_name or config.current_context or DEFAULT_CONTEXT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| else: | |
| config = cls.create(path, ConfigFile()) | |
| context_name = context_name or DEFAULT_CONTEXT | |
| else: | |
| config = cls.create(path, ConfigFile()) | |
| context_name = context_name or config.current_context or DEFAULT_CONTEXT |
🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py` around
lines 379 - 381, Update the missing-config branch in the config creation flow to
preserve the context selected by Config.create(): prefer config.current_context
when assigning context_name, and only fall back to DEFAULT_CONTEXT when it is
unset. Add a regression test covering a first write with NMP_CURRENT_CONTEXT set
and no explicit context, verifying the configuration is persisted under and
resolves through that environment-selected context.
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
Improves the
nemo configmulti-context workflow and refactorsConfig.write()so the CLI can report configuration changes precisely.Creating the first context makes it current automatically. Creating additional named contexts preserves the existing current context unless
--activateis supplied.CLI behavior changes
Predictable context activation
--activateexplicitly creates or updates a context and makes it current.Context-specific output
Context 'staging' created and set as currentContext 'staging' created, followed by ause-contexthintConfiguration updated for context 'staging' and set as currentSwitched to context 'staging'Configuration updated for context 'staging'Effective current context
nemo config current-contextnow follows the documented precedence:--contextNMP_CURRENT_CONTEXTcurrent_contextSafer environment overrides
NMP_CURRENT_CONTEXTmust reference an existing context.--context <name>still permits creating a new context.Safe context-switch hints
nemo config use-context -- <name>Config.write()APIConfig.write_with_result(), returning a frozenConfigWriteResultcontaining:configcontext_namecreatedConfig.write()backward-compatible by delegating towrite_with_result().set_current_if_unsetto control whether a write establishes the initial current context.set_current_on_createfor existing callers.GenericandTypeVarto preserve Python 3.11 compatibility.Documentation and maintenance
docs/cli/configuration.mdxaround single- and multi-deployment workflows.nemo configdocumentation.config_help.pyso the command and manifest registry share one source of truth.Test plan
git diff --checkpassed--activateswitches explicitlyNMP_CURRENT_CONTEXTvalues fail without modifying configcurrent-contextfollows CLI > environment > saved precedenceSummary by CodeRabbit
New Features
Documentation