fix(mtp): gate legacy --draft-min with --draft-max on chained ngram
Codex flagged that the legacy chained mtp+ngram path emits --draft-min 48 while --draft-max is suppressed and later reused for MTP (spec_draft_n_max, typically 2/3). That produces an inverted legacy ngram range (--draft-min 48 --draft-max 2/3) on affected binaries, which can break or effectively disable ngram-mod for auto CPU MTP loads and forced mtp+ngram requests. Both --draft-min and --draft-max are generic flags on legacy llama-server builds, so either one would race with MTP's own values. Gate the pair together: when chain_with_mtp=True on the legacy flavor we drop both flags and rely on MTP's emission for the chained range. Standalone ngram still emits both, preserving a valid min<=max window. Updated test_build_ngram_mod_flags_legacy_chained_omits_draft_max (now omits_draft_min_and_max) and added a min<=max guard on the standalone case. Full suite (test_llama_cpp_mtp_detection + test_llama_server_args) passes locally: 229 / 229.
This commit is contained in:
parent
a54396a675
commit
068e129cb2
2 changed files with 30 additions and 21 deletions
|
|
@ -523,17 +523,18 @@ def _build_ngram_mod_flags(
|
|||
# Legacy llama.cpp before the spec arg rename: same knobs lived
|
||||
# under --spec-ngram-size-n (lookup length) and the generic
|
||||
# --draft-min / --draft-max (ngram size N range).
|
||||
out = [
|
||||
"--spec-ngram-size-n",
|
||||
str(n_match),
|
||||
"--draft-min",
|
||||
str(n_min),
|
||||
]
|
||||
out = ["--spec-ngram-size-n", str(n_match)]
|
||||
if not chain_with_mtp:
|
||||
# Only safe to set --draft-max here when MTP is NOT in the
|
||||
# same emission; otherwise this duplicates MTP's --draft-max
|
||||
# and last-wins clobbers the MTP draft length.
|
||||
out.extend(["--draft-max", str(n_max)])
|
||||
# Gate --draft-min AND --draft-max together. Both flags are
|
||||
# generic on legacy binaries, so emitting either in the same
|
||||
# invocation as MTP would race with MTP's own --draft-min /
|
||||
# --draft-max (typically 2/3 from spec_draft_n_max). Earlier
|
||||
# we suppressed only --draft-max, which produced an inverted
|
||||
# legacy range (--draft-min 48 --draft-max 2/3) and disabled
|
||||
# ngram-mod entirely. Keeping the pair together preserves a
|
||||
# valid range when ngram is standalone and avoids the clobber
|
||||
# when it is chained with MTP.
|
||||
out.extend(["--draft-min", str(n_min), "--draft-max", str(n_max)])
|
||||
return out
|
||||
return []
|
||||
|
||||
|
|
|
|||
|
|
@ -1269,28 +1269,36 @@ def test_canonicalize_spec_mode_none_aliases_map_to_off(value):
|
|||
# ---- Bug C: legacy chained MTP+ngram does not duplicate --draft-max ----
|
||||
|
||||
|
||||
def test_build_ngram_mod_flags_legacy_chained_omits_draft_max():
|
||||
"""When chaining ngram-mod alongside MTP on a legacy llama-server,
|
||||
--draft-max collides with MTP's --draft-max (same flag, different
|
||||
semantic in old builds). The chain_with_mtp flag must suppress the
|
||||
ngram-mod --draft-max so MTP's draft length wins."""
|
||||
def test_build_ngram_mod_flags_legacy_chained_omits_draft_min_and_max():
|
||||
"""Legacy chained ngram+MTP must drop BOTH --draft-min and --draft-max.
|
||||
Both are generic flags on legacy binaries, so MTP's own --draft-min /
|
||||
--draft-max (e.g. 2/3 from spec_draft_n_max) would race with ngram's
|
||||
larger size-N range. Suppressing only --draft-max produced an inverted
|
||||
legacy range (--draft-min 48 --draft-max 2/3) that disabled ngram-mod
|
||||
on affected builds; gate them together. --spec-ngram-size-n is a
|
||||
distinct flag and must remain so ngram-mod still tunes the chain."""
|
||||
caps = {"ngram_mod_flavor": "legacy"}
|
||||
chained = _build_ngram_mod_flags(caps, chain_with_mtp = True)
|
||||
assert (
|
||||
"--draft-max" not in chained
|
||||
), f"chain_with_mtp=True must drop --draft-max on legacy; got {chained}"
|
||||
# The other two knobs must still be present so ngram-mod actually
|
||||
# tunes the chain.
|
||||
assert (
|
||||
"--draft-min" not in chained
|
||||
), f"chain_with_mtp=True must drop --draft-min on legacy; got {chained}"
|
||||
assert "--spec-ngram-size-n" in chained
|
||||
assert "--draft-min" in chained
|
||||
|
||||
|
||||
def test_build_ngram_mod_flags_legacy_standalone_keeps_draft_max():
|
||||
"""When NOT chaining, --draft-max is the ngram-mod size-N max and must
|
||||
still be emitted on legacy."""
|
||||
def test_build_ngram_mod_flags_legacy_standalone_keeps_draft_min_and_max():
|
||||
"""Standalone ngram on legacy must emit --draft-min AND --draft-max as
|
||||
a valid range (size-N min/max) so ngram-mod actually has a window."""
|
||||
caps = {"ngram_mod_flavor": "legacy"}
|
||||
standalone = _build_ngram_mod_flags(caps, chain_with_mtp = False)
|
||||
assert "--draft-min" in standalone
|
||||
assert "--draft-max" in standalone
|
||||
# The range must not be inverted: min <= max.
|
||||
i_min = standalone.index("--draft-min")
|
||||
i_max = standalone.index("--draft-max")
|
||||
assert int(standalone[i_min + 1]) <= int(standalone[i_max + 1])
|
||||
|
||||
|
||||
def test_build_ngram_mod_flags_new_flavor_always_emits_distinct_names():
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue