fix(onboard): default the interactive Ollama menu to the requested model - #6876
Closed
laitingsheng wants to merge 5 commits into
Closed
fix(onboard): default the interactive Ollama menu to the requested model#6876laitingsheng wants to merge 5 commits into
laitingsheng wants to merge 5 commits into
Conversation
The interactive onboarding wizard chose the Ollama model menu default from installed-model memory fit alone, so a set NEMOCLAW_MODEL was ignored and the first fitting entry was pre-selected. Thread the requested model (NEMOCLAW_MODEL, then any recovered model) into the menu and pre-select it when it is one of the offered options. Also report the auto-selected model during non-interactive onboarding when no model is requested, so an unintended default is not applied silently. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughOllama onboarding now logs non-interactive auto-selected models and supports ChangesOllama model selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant PreferenceResolver
participant OllamaPrompt
participant InstalledModels
Onboarding->>PreferenceResolver: resolve requested, environment, or recovered model
Onboarding->>OllamaPrompt: pass preferredModel and excluded probe failures
OllamaPrompt->>InstalledModels: match preferredModel against installed options
InstalledModels-->>OllamaPrompt: matching or fallback options
OllamaPrompt-->>Onboarding: selected Ollama model
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6876.docs.buildwithfern.com/nemoclaw |
laitingsheng
added
NV QA
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
![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: 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 `@src/lib/inference/ollama/proxy.test.ts`:
- Around line 223-235: Strengthen the test around promptOllamaModel so the
not-installed preferredModel case asserts the exact computed fallback selected
by this fixture, rather than accepting either installed model. Also assert the
prompt default index if it is exposed by the existing setup, while keeping the
test focused on fallback behavior rather than implementation details.
🪄 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: 0d30493f-f539-4a64-be17-e00b70ee80a2
📒 Files selected for processing (6)
docs/inference/set-up-ollama.mdxsrc/lib/inference/local.test.tssrc/lib/inference/local.tssrc/lib/inference/ollama/proxy.test.tssrc/lib/inference/ollama/proxy.tssrc/lib/onboard.ts
…active-model-default
…oard.ts Keeps the top-level onboard entrypoint net-neutral per the codebase growth guardrail. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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 `@src/lib/onboard/ollama-model-preference.ts`:
- Around line 15-24: Update promptOllamaModel in the Ollama proxy flow so
preferredModel is matched only when fitting installed models are available;
otherwise do not use the unavailable preference when selecting from bootstrap
options. Add a regression test covering an unavailable requested preference that
appears in the bootstrap list, ensuring it is not selected by default.
🪄 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: 89d319fd-7e98-4a52-8a01-d085b6e6d184
📒 Files selected for processing (2)
src/lib/onboard.tssrc/lib/onboard/ollama-model-preference.ts
…ptions promptOllamaModel matched preferredModel against bootstrap fallback options too, so an uninstalled requested model could become the Enter-key default and trigger an unrequested pull. Gate the match on usingInstalled so the preference only applies to installed options. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…active-model-default
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 (2)
src/lib/inference/ollama/proxy.test.ts (1)
195-195: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAppend the issue reference to both test titles.
These titles are behavior-oriented, but the repository guideline requires local issue references as a final
(#1234)suffix. These tests cover issue#6869.Proposed fix
- it("defaults the menu to the requested model rather than a fixed computed default", async () => { + it("defaults the menu to the requested model rather than a fixed computed default (`#6869`)", async () => { - it("ignores a preferred model that is only present in the bootstrap fallback, never the installed menu", async () => { + it("ignores a preferred model that is only present in the bootstrap fallback, never the installed menu (`#6869`)", async () => {Also applies to: 237-237
🤖 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 `@src/lib/inference/ollama/proxy.test.ts` at line 195, Append the issue reference suffix “(`#6869`)” to both affected test titles in the Ollama proxy test, including the test at the shown location and the additional matching test, while preserving their existing behavior-oriented wording.Source: Coding guidelines
src/lib/onboard.ts (1)
3022-3022: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd onboarding-boundary coverage for preferred Ollama models.
proxy.promptOllamaModeltests don’t cover theselectAndValidateOllamaModel→promptOllamaModelWithPreferencewiring. Add a public-boundary test for installed, unavailable, and probe-excluded preferred models so onboarding exercises the preference/exclusion inputs end to end.🤖 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 `@src/lib/onboard.ts` at line 3022, Add public-boundary tests for the onboarding flow that reaches selectAndValidateOllamaModel and promptOllamaModelWithPreference, covering preferred models that are installed, unavailable, and excluded by probing. Assert each scenario passes preference and exclusion inputs through the public proxy.promptOllamaModel path and preserves the expected model-selection outcome.Source: Path instructions
🤖 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 `@src/lib/inference/ollama/proxy.test.ts`:
- Line 195: Append the issue reference suffix “(`#6869`)” to both affected test
titles in the Ollama proxy test, including the test at the shown location and
the additional matching test, while preserving their existing behavior-oriented
wording.
In `@src/lib/onboard.ts`:
- Line 3022: Add public-boundary tests for the onboarding flow that reaches
selectAndValidateOllamaModel and promptOllamaModelWithPreference, covering
preferred models that are installed, unavailable, and excluded by probing.
Assert each scenario passes preference and exclusion inputs through the public
proxy.promptOllamaModel path and preserves the expected model-selection outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d57bd574-f613-4e35-be95-7850860a50dc
📒 Files selected for processing (3)
src/lib/inference/ollama/proxy.test.tssrc/lib/inference/ollama/proxy.tssrc/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/inference/ollama/proxy.ts
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
The interactive onboarding wizard chose the Ollama model menu default from installed-model memory fit alone, so a
NEMOCLAW_MODELset by the user was ignored and the first fitting model was pre-selected. The menu now pre-selectsNEMOCLAW_MODEL(then any recovered model) when it is one of the offered options, and a non-interactive run reports the model it auto-selects when none is requested.Related Issue
Fixes #6869
Changes
promptOllamaModelaccepts an optionalpreferredModeland uses it as the menu default when it matches an offered option (src/lib/inference/ollama/proxy.ts).NEMOCLAW_MODEL, falling back to any recovered model, and passes it to the menu (src/lib/onboard.ts).NEMOCLAW_MODELwhen no model is requested (src/lib/inference/local.ts).docs/inference/set-up-ollama.mdx).src/lib/inference/ollama/proxy.test.ts,src/lib/inference/local.test.ts).Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/inference/ollama/proxy.test.ts src/lib/inference/local.test.ts→ 75 passednpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
NEMOCLAW_MODEL.Bug Fixes
Documentation / Tests