Skip to content

fix(config): preserve model.context_length on same-model re-pick - #59191

Closed
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:simplicio/fix-59050-context-length-preserve
Closed

fix(config): preserve model.context_length on same-model re-pick#59191
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:simplicio/fix-59050-context-length-preserve

Conversation

@wesleysimplicio

Copy link
Copy Markdown
Contributor

Closes #59050

_apply_main_model_assignment() unconditionally drops context_length
on every call, even when the model and provider are unchanged. For
self-hosted OpenAI-compatible endpoints, auto-detection is unreliable
and the lost override leaves users with a wrong context window.

This captures prev_model before overwriting it and only pops
context_length when the model or provider actually changed, mirroring
the same-provider preserve behavior already used for base_url and
api_key in the same function.

Changes:

  • hermes_cli/web_server.py:_apply_main_model_assignment()

…OST /api/model/set

The _apply_main_model_assignment() function unconditionally drops
context_length on every call, even when the model and provider are
unchanged.  For self-hosted OpenAI-compatible endpoints (llama.cpp,
etc.), auto-detection is unreliable and the silently-lost override
leaves users with a wrong context window and hard mid-session failures.

Capture prev_model before overwriting it and only pop context_length
when the model or provider actually changed, mirroring the same-provider
preserve behavior already used for base_url and api_key.

Closes NousResearch#59050
@alt-glitch alt-glitch added type/bug Something isn't working comp/dashboard Web dashboard / control panel UI (dashboard/, landing) area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 5, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #59125 — both fix #59050 by preserving model.context_length on a same-model/same-provider re-pick in _apply_main_model_assignment(). #59125 was opened first and is canonical.

@AmirF194 AmirF194 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself is right. The unconditional model_cfg.pop("context_length", None) was wiping a user's manual context override on every re-pick, and gating it on new_provider != prev_provider or model != prev_model correctly preserves it on a same-model/same-provider re-pick while still clearing it whenever the model or provider actually changes. I checked the provider-change-with-same-model-name case too, and it drops as it should, and the change does not disturb the _denormalize_config_from_web path, which sets context_length explicitly after this helper runs.

Two things hold me back from a plain approval. First, this looks like a duplicate of #59125, which was opened a bit earlier, covers the same function with the same condition, and additionally ships regression tests and updates the docstring. Given the triage note pointing there, it probably makes sense to fold into that one. Second, if this PR moves forward instead, it needs a test: _apply_main_model_assignment's contract is pinned by test_apply_main_model_assignment_base_url_and_context_reconcile, and changing the behavior without extending that test leaves the new preserve path unguarded against a future regression of #59050. Worth fixing the docstring too, which still says the context_length override "is always dropped" and now contradicts the code.

I verified the fix is load-bearing in a clean Python 3.11 container matching CI with a scratch test: same-model re-pick preserves and different-model/different-provider clears, and reverting the source makes the two preserve cases fail. (The one unrelated test_get_status failure I saw also fails on pristine main, so that is environmental, not from this change.)

Minor: model is compared raw against a stripped prev_model. It is fine in the normalized request path, but stripping both sides (as #59125 does) removes the asymmetry.

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Fechando como duplicada de #59125, conforme review/triagem. A versão canônica já cobre o mesmo ajuste com testes/docstring.

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Reopening — this was closed in error by the automated stale-PR check, which only inspected GitHub's reviewDecision field (APPROVED/CHANGES_REQUESTED) and missed this PR's existing COMMENTED maintainer review confirming the fix is correct. Apologies for the noise.

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Re-closing: this was mistakenly reopened citing the stale-close automation bug (that bug affects #59194/#59189, which is a separate issue), but this PR's actual close reason was different and still valid — it is a duplicate of #59125 (opened earlier, same fix for #59050, and #59125 additionally ships regression tests and a docstring update per @AmirF194's review).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. The premise is confirmed on current main: _apply_main_model_assignment() still unconditionally removes context_length at hermes_cli/web_server.py:1169, and the dashboard main-slot route reaches that helper at hermes_cli/web_server.py:5820-5823.

Problems

  • The PR does not update the helper regression contract. tests/hermes_cli/test_web_server.py:3308-3323 currently documents and asserts that context_length is always dropped, leaving the new same-model preservation behavior unguarded.
  • The helper docstring still says the override is always dropped at hermes_cli/web_server.py:1139-1140, which would contradict the proposed behavior.

Suggested changes

  • Add same-model/same-provider preservation and same-provider/different-model clearing cases to the shared helper test.
  • Update the docstring and compare a normalized incoming model value, matching the more complete implementation in canonical duplicate [codex] preserve context length on model re-pick #59125.

This is an automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 15, 2026
@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Re-closing (again) — this PR is a duplicate of #59125 (opened earlier for the same #59050 root cause, and #59125 additionally ships regression tests + a docstring update per @AmirF194's and teknium1's reviews). Already confirmed in my 2026-07-15T06:34Z close comment; something reopened it again without new information. #59125 remains open and is the canonical fix to review/merge. Leaving closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/dashboard Web dashboard / control panel UI (dashboard/, landing) duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: POST /api/model/set always drops model.context_length, even on a same-model/same-provider re-pick

4 participants