Add a checked registry for engine environment variables - #800
ZacharyZcR wants to merge 11 commits into
Conversation
df4af78 to
5eb550c
Compare
|
Rebased on current
Six variables in two days, from three different engines, none of them mine. That is the drift rate this exists to catch, measured rather than argued — and it is the reason a registry has to be checked by a machine instead of maintained by intention. Types are taken from the call sites rather than the names: the three read through One judgement call worth flaggingTwo of the six belong to
Stale numbers in the header commentThe comment said "212 environment variables from 187 scattered getenv() call sites" — both already wrong, in precisely the way the table would have been without this check. The count is now 220. The The PR title still says 214; it is 220 now. Happy to retitle, or leave it since the count moves with Verified: |
|
Same blocker as #849, from the same cause — the branch carries the identical 16 compiled objects:
The registry itself I want to read properly rather than wave through — 214 environment variables is most of the engine's real interface, and a check that rejects an unknown name is the kind of thing that is excellent until the day someone's working |
|
Two separate things here, and I want to check the second rather than assume. 1. The objects clear themselves now. #868 is on 2. The body still opens with Asking because I have got this wrong before in the other direction. I merged #804 and #806 without noticing the same marker in the body and had to say so publicly on #801. I would rather be the person who asks a redundant question than the one who does that twice. If it is ready, rebase and say so and I will review it properly. On the substance I want to read it carefully rather than wave it through: 214 environment variables is most of the engine's real interface, and a registry that rejects unknown names is excellent right up until the day it rejects someone's working |
5eb550c to
f9bc14c
Compare
#868 closed most of this: c/COLI_V4_UNIT_*.o covers the twelve objects the amalgamated deepseek_v4 Makefile leaves next to the sources. Four more go somewhere else. c/Makefile:895: V4_OWN_DIR = build/ownership so `make check` also writes build/ownership/COLI_V4_UNIT_{RUNTIME,CONFIG, ST,NATIVE_QUANT}.o, which no rule matched. On current dev a clean checkout plus `make check` still leaves `?? c/build/` in git status -- one `git add -A` away from the same accident #868 was written to stop, and the one I made on #849 and #800. Ignoring c/build/ wholesale rather than the four names: nothing under it is tracked, and it is a build output directory, so a rule per object would need editing every time the ownership suite grows. Worth stating because it caught me on #849: an ignore rule does not untrack what is already committed. #868 made the twelve invisible in git status while leaving them in the tree; git rm --cached is what removed them. Same shape as #835. This prevents the next one, it does not clean up an existing one.
f9bc14c to
d8ca421
Compare
|
Ready for review. Rebased onto current |
d8ca421 to
0d4e648
Compare
dae908a to
c8425d3
Compare
ZacharyZcR
changed the title
Two failure modes, both silent today:
COLI_PREFIL_CHUNK=512 ./colibri ... # typo
K3_BITS=8 ./colibri ... # wrong engine
Neither prints anything. The run proceeds on the default, produces entirely
plausible output, and the number that gets written into a benchmark table is
for a configuration nobody set. Anyone who has tuned this engine has lost
an afternoon to one of these.
The cause is that nothing in the tree knew which variables exist: 214 of
them, read from 187 scattered getenv() sites, with no list anywhere. So:
c/coli_env.h -- one table: name, value shape, which engines read it, and
whether it is deprecated. coli_env_check() runs once at startup in all
four engines and reports what it cannot use, with a "did you mean X?"
from an edit-distance match:
[env] unknown variable COLI_PREFIL_CHUNK -- did you mean COLI_PREFILL_CHUNK?
[env] K3_BITS is not read by colibri (it belongs to kimi_k3) -- it will have no effect
It warns, it does not exit: an unrecognised variable has never stopped a
run and making it fatal would break scripts that export a knob for
whichever engine they may launch. COLI_ENV_STRICT=1 makes it fatal for
those who want the guarantee. COLI_ENV_DUMP=1 prints every variable the
engine reads with its current value, which answers "is my export actually
arriving" without a debugger.
Variables we do not own are never touched -- only COLI_*, K3_* and INK_*
are checked, so an unrelated EDITOR in the environment is not our
business.
This is the shape vLLM arrived at (vllm/envs.py): one declaration point, one
prefix convention, validation at the boundary. Worth recording that vLLM
carries 284 of these to our 214 -- the count was never the problem in either
project, the missing registry was.
WHAT THIS DELIBERATELY DOES NOT DO
It does not rewrite the 187 getenv() call sites. That is mechanical but
large, and one mistyped default in the middle of it silently changes engine
behaviour; it belongs in its own commits, engine by engine, with this table
as the precondition for doing it safely.
So the table could drift from the code the moment it lands -- which is what
`make -C c check-env` (and a CI job) prevents. It fails on a getenv() with
no row, a row nothing reads, a duplicate, or a table that is not sorted
(coli_env_find binary-searches it, so an unsorted table silently stops
finding valid names). All three failure modes were verified by breaking the
table on purpose and confirming the check catches each one. It caught its
own first bug during development: COLI_ENV_DUMP was added to the code and
not to the table.
Only TEMP is marked deprecated -- it is the one the docs already call a
deprecated alias, and the one that actually bites, since $TEMP is the
temp-DIRECTORY path on Windows and under the ROCm runtime (JustVugg#509). SNAP,
SNAP_MIRROR and PROMPT are fallbacks but remain the mainstream spelling
(README, CONTRIBUTING's oracle command and CI all use SNAP=), so flagging
them would have printed a warning on every existing run and every CI job for
no benefit. Verified: today's usage is completely silent.
All four engines build warning-free and parse under every backend
configuration (default, CUDA, Vulkan, Metal, ANS); make test-c passes; the
Python suite is 288 tests OK (18 skipped).
Rebasing on dev turned check-env red, which is the point of it: - read by the code but missing from coli_env.h: COLI_MTP_GUARD_PCT - read by the code but missing from coli_env.h: COLI_MTP_GUARD_WINDOW - read by the code but missing from coli_env.h: COLI_V4_EXPERT_PREFETCH - read by the code but missing from coli_env.h: CUDA_EXPERT_LOAD_BALANCE - read by the code but missing from coli_env.h: INK_METAL_SHARED - read by the code but missing from coli_env.h: V4_PREFIX_LOG Types come from the call sites, not from the names: the three read through atoi() are CE_INT, INK_METAL_SHARED and COLI_V4_EXPERT_PREFETCH are tested as switches so CE_BOOL, and V4_PREFIX_LOG is presence-checked like the INK_PREFIX_LOG row already in the table, so CE_STR. Two of them belong to deepseek_v4.c, which JustVugg#165 added and which has no engine bit here, so add CE_DSV4. It is deliberately NOT folded into CE_ALL: that flag means the four engines sharing route_trace.h, rans.h and omp_tune.h, and deepseek_v4.c includes none of the three -- putting it in CE_ALL would claim every shared-header knob is read by V4 too. Also refresh the header comment, which had gone stale in the same way the table would have: 212 -> 220. The "187 scattered getenv() call sites" figure is removed rather than corrected -- nothing verifies it, so it can only rot again. `make check-env` prints the live count and is the one number in that comment that cannot go stale. check-env: 220 variables, registry matches the sources.
Rebasing again turned check-env red again, which is the argument for the
PR restating itself: COLI_V4_{AUTOPIN,DIRECT,MARKOV_BLOCK,MARKOV_KEEP,
MARKOV_SPEC,PREWARM,SAVE_USAGE}, K3_CUDA, V4_{DRAFT,MTP,MTP_DRAFT,MTP_GB,
MTP_MIN,MTP_PARTIAL_KEEP,NGRAM,NGRAM_PARTIAL_KEEP} -- most of them from
JustVugg#839's MTP/DSpark work, landed since the last rebase.
Types from the call sites: the ones tested as atoi()!=0 or atoi()==0
switches are CE_BOOL, V4_MTP_GB parses with atof() and defaults to 0.45
so CE_FLOAT, the rest read a number with atoi() so CE_INT. K3_CUDA is
CE_KIMI; every V4_/COLI_V4_ row is CE_DSV4.
Header count 220 -> 236.
c8425d3 to
77408c2
Compare
|
Still wanted and still absent from dev: The conflicts look worse than they are. Eleven of thirteen files conflict, but almost all of it is a one-line One warning so the rebase does not go red on arrival: the real work is not the textual merge, it is re-running |
This branch has not been deployed
2 participants
Problem and behavior
Misspelled or wrong-engine environment variables silently leave defaults active and can produce incorrectly labelled runs.
c/coli_env.hadds a sorted registry with variable names, value shapes, engine ownership and deprecation metadata. Engine startup warns about unknown project-prefixed variables and registered variables belonging to a different engine;COLI_ENV_STRICT=1makes those errors fatal.COLI_ENV_DUMP=1prints the selected engine's registered variables and values. Value shapes are reporting metadata, not value validation.The registry now covers nine engines: Colibri, Kimi K3, Inkling, OLMoE, DeepSeek V4, DeepSeek V4.1, Qwen3.6, GLM-5.3 and Qwen3.8. A 16-bit ownership mask preserves the ninth engine's bit. Existing getenv call sites and defaults remain in place. Unknown unrelated variables are ignored; active legacy spellings remain accepted.
Drift prevention and integration
make -C c check-envand CI check missing/stale names, duplicates and ordering. The scanner recognizes Qwen3.8's typed bool wrapper. This checks name coverage, not completeness of engine ownership or value types.Merged current
dev(d99dc06) without rewriting branch history. Resolved conflicts while retaining the current Qwen CUDA tiers, GLM backend linkage and OpenMP initialization. Added 39 missing variables, refreshed shared ownership from current source/include paths, and wired DeepSeek V4.1 startup validation. GLM-5.3 and Qwen3.8 now also honor the dump option.Current validation
test-cdiscovery.GPU model execution was not rerun for this integration revision. CI results must be evaluated at the latest commit.
Follow-up: strict-mode unknown-name detection also covers
COLIBRI_,KIMI_,Q36_,QWEN_,DSV4_andV4_. Previously a misspelling such asQWEN_EXPERT_KERNLbypassed validation. External prefixes (CUDA_,OMP_,QT_,VK_) remain unclaimed. Added the registry header to engine-source consumers in Makefile so edits cannot leave stale binaries.Validation: registry C regression and 5 Makefile CUDA-scope tests pass; Qwen3.6 rebuild passes; short strict-mode startup checks reject six added-prefix typos and ignore four external-prefix variables; a header-only change schedules a rebuild; MinGW registry cross-compilation passes. Registry remains at 393 names.
Full C-suite validation at 9ad07b2:
make -C c -j4 test-ccompleted successfully on Linux/WSL. The runner invoked 146 test binaries with no nonzero exits. Three binaries explicitly skipped their checks: Qwen3.8 vision (reference fixture absent), int4 AVX-512 (not enabled on this build), and the Windows direct-I/O compatibility check (POSIX uses native O_DIRECT). These skips are not counted as validated behavior. This run covers the complete configured CPU C test target; it does not establish full-model GPU or Windows runtime correctness.Registry scanner correction: scan whole source files, skip comments/string/character literals, and require a function-name boundary. This detects whitespace and multiline calls, prevents a commented-out read from keeping a stale registry row alive, and avoids mistaking
my_getenvforgetenv. Four scanner regressions pass; three reproduced incorrect behavior before the fix. The production source check still matches 393 names. The scanner handles literal names and known wrappers; it does not evaluate macros or dynamically assembled names. The earlier full C-suite result remains tied to 9ad07b2; this follow-up changes only the Python checker and its tests.DeepSeek V4 build dependency follow-up: add
coli_env.hto all four engine-source rules in the separateMakefile.deepseek-v4(normal/test units, hot-row test unit, batch test unit and serve-framing executable). After buildingCOLI_V4_UNIT_GENERATE_STATS.o, the original rule returns 0 (up to date) even withmake -q -W coli_env.h; the corrected rule returns 1 (rebuild needed), while the unchanged-header check returns 0. The object compiles successfully, all 10 Makefile Python tests pass, and the source registry remains consistent at 393 names. No engine runtime behavior changed.Dump flag semantics
At 061fe47, the shared dump function checks the numeric COLI_ENV_DUMP value, and all nine engines call it consistently. Previously COLI_ENV_DUMP=0 still printed the registry (60 lines in the Qwen fixture), despite the documented disabled default. Unset, empty and zero now suppress output; one enables it. Validation: four shared-header cases and four Qwen3.6 no-model startup cases pass; Qwen3.6 builds, the C registry test and four scanner tests pass, all 393 registered names remain in sync, and the header fixture cross-compiles with MinGW. No full-model run was performed.
Combined follow-up at 061fe47: applied the latest registry/scanner/build-dependency/dump-switch changes cleanly to the eleven-PR CUDA integration checkout based on dev d99dc06. Qwen3.6 and Qwen3.8 CPU binaries both compile; both suppress dump output at zero and enable it at one during no-model startup. Four scanner tests and the 393-variable coverage check pass on this combined checkout. This checks integration with the pending CUDA/tier changes; it is not a CUDA or full-model runtime acceptance result.