feat(cli): improve multi-context UX and Config.write API - #1072
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds context-aware configuration writes, context-selection precedence, write-result reporting, shared CLI help, expanded tests, and deployment 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
Suggested labels: Suggested reviewers: 🚥 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: 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
|
1c110ba to
afc0e37
Compare
|
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. |
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 `@docs/cli/reference.mdx`:
- Line 143: The example was edited directly in generated CLI documentation.
Locate its generator source, move or update the example there, then regenerate
docs/cli/reference.mdx using the designated documentation generation command; do
not hand-edit the generated page.
In `@packages/nemo_platform_ext/tests/cli/commands/test_config.py`:
- Around line 335-346: Update
test_set_rejects_unknown_context_selected_by_environment to load and snapshot
the full configuration before invoking the failing command, then assert the
configuration loaded afterward equals that snapshot. Keep the existing exit-code
and error-output assertions while replacing the narrow absence check with
verification that no configuration fields or contexts changed.
🪄 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: 6b1be2b0-36c8-4e48-be7f-84aa364b4e64
⛔ 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 (14)
docs/cli/configuration.mdxdocs/cli/connect-to-deployments.mdxdocs/cli/reference.mdxdocs/fern/versions/latest.ymldocs/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
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config_help.py
- packages/nemo_platform_ext/README.md
- docs/get-started/setup.mdx
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/manifest_registry.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config.py
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/auth.py
- docs/cli/configuration.mdx
- packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py
- packages/nemo_platform_ext/docs/runbook.md
- packages/nemo_platform_ext/tests/config/test_config.py
afc0e37 to
64416fb
Compare
|
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nemo_platform_ext/tests/cli/commands/test_config.py (1)
304-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the persisted switch for
-qa.Lines 304-305 only assert successful command execution. Load the configuration and assert
current_context == "-qa"to verify that the separator command switches 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/tests/cli/commands/test_config.py` around lines 304 - 305, Update the test around the use-context command invocation to load the persisted configuration after the successful switch and assert that current_context equals "-qa", using the existing configuration-loading helper and preserving the exit-code assertion.
🤖 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.
Nitpick comments:
In `@packages/nemo_platform_ext/tests/cli/commands/test_config.py`:
- Around line 304-305: Update the test around the use-context command invocation
to load the persisted configuration after the successful switch and assert that
current_context equals "-qa", using the existing configuration-loading helper
and preserving the exit-code assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 617c01e1-434f-41bb-a3ad-0c3e4e865cb9
⛔ 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 (14)
docs/cli/configuration.mdxdocs/cli/connect-to-deployments.mdxdocs/cli/reference.mdxdocs/fern/versions/latest.ymldocs/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
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/cli/reference.mdx
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/auth.py
- docs/fern/versions/latest.yml
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config_help.py
- docs/cli/connect-to-deployments.mdx
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/manifest_registry.py
- docs/get-started/setup.mdx
- packages/nemo_platform_ext/README.md
- packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/config.py
- packages/nemo_platform_ext/docs/runbook.md
- packages/nemo_platform_ext/tests/config/test_config.py
- packages/nemo_platform_ext/src/nemo_platform_ext/config/config.py
- docs/cli/configuration.mdx
64416fb to
f9d9fc6
Compare
|
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. |
f4f56eb to
b844036
Compare
Signed-off-by: anastasia-nesterenko <anesterenko@nvidia.com>
b844036 to
945a2e2
Compare
…-cli-command-for-remote-access Signed-off-by: anastasia-nesterenko <anesterenko@nvidia.com>
8082972 to
3d10c86
Compare
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