Skip to content

Add a registry for the 214 environment variables, and check the environment against it - #800

Open
ZacharyZcR wants to merge 2 commits into
JustVugg:devfrom
ZacharyZcR:feat/env-registry
Open

Add a registry for the 214 environment variables, and check the environment against it#800
ZacharyZcR wants to merge 2 commits into
JustVugg:devfrom
ZacharyZcR:feat/env-registry

Conversation

@ZacharyZcR

Copy link
Copy Markdown
Contributor

Draft.

The problem

Two things are 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 ends up in a benchmark table is for a configuration nobody set.

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.

What this adds

c/coli_env.h β€” one table: name, value shape, which engines read it, whether it is deprecated. coli_env_check() runs once at startup in all four engines:

[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

The suggestion is an edit-distance match against the table.

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 might 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.

On the count, since it is the obvious first question

This is the shape vLLM arrived at (vllm/envs.py): one declaration point, one prefix convention, validation at the boundary.

Worth recording what that file actually contains, because it inverts the intuition: vLLM carries 284 environment variables to our 214. It stays manageable through structure, not restraint β€” 94% under a single VLLM_ prefix (the exceptions are all borrowed conventions: CUDA_VISIBLE_DEVICES, LD_LIBRARY_PATH, NO_COLOR, S3_*), one dict as the single definition point, types declared under if TYPE_CHECKING:, env_with_choices() validators, and __getattr__ raising AttributeError on an unknown name.

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 change is mechanical but large, and one mistyped default in the middle of it silently alters engine behaviour. It belongs in its own commits, engine by engine, with this table as the precondition for doing it safely.

Which means the table could drift from the code the moment it lands. That 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 name
  • a table that is not sorted β€” coli_env_find() binary-searches it, so an unsorted table silently stops finding valid names

All four failure modes were verified by breaking the table on purpose and confirming the check catches each. It also caught its own first bug during development: COLI_ENV_DUMP was added to the code and not to the table.

On deprecation, and on not being noisy

Only TEMP is marked deprecated. It is the one the docs already call a deprecated alias and the one that actually bites β€” $TEMP is the temp-directory path on Windows and under the ROCm runtime (#509).

SNAP, SNAP_MIRROR and PROMPT are fallbacks in the code, and my first pass flagged them too. That was wrong: SNAP= is the mainstream spelling in the README, in CONTRIBUTING's oracle command and in CI itself, so flagging it would have printed a warning on every existing run and every CI job for no benefit. Verified explicitly β€” today's usage is completely silent, including the SNAP=tiny_inkling ./inkling line in inkling-oracle.

Verification

  • All four engines build warning-free
  • All four parse under every backend configuration: default, COLI_CUDA, COLI_VULKAN, COLI_METAL, COLI_ANS
  • make test-c passes; Python suite 288 tests OK (18 skipped)
  • make check-env: 214 variables, registry matches the sources
  • Behaviour checked on the real binaries, not just a stub: typo, wrong-engine, deprecated alias, unrelated variable, clean environment, and COLI_ENV_STRICT=1 exit code

Not verified here: no GPU path was run (no CUDA/Metal/Vulkan device available) β€” the backend configurations are compile-checked, not executed.

Suggested follow-ups (not in this PR)

  1. Replace the 187 getenv() sites with table lookups, one engine per commit.
  2. The 69 bare-name variables (TEMP, CHAT, HOT, WIDE, DIRECT, PIPE, CAP, SEED, …) are the ones that can genuinely collide in a real shell β€” TEMP already did. Renaming them under COLI_* needs a deprecation cycle, and the registry now has the machinery for it.
  3. EXPERT_BUDGET / EXPERT_BUDGET_EXPERIMENTAL is the one knob I can point at as a removal candidate: the code already disables it (measured empty operating window, issue #303). Whether it stays is a call for whoever owns that experiment.

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).
A single shared .PHONY makes every PR that adds a target conflict with every
other one on that line -- the same failure the TEST_RULES comment above
describes for TEST_BINS and fixes there. make accumulates multiple .PHONY
declarations, so a new target adds a line instead of editing everyone's.
@ZacharyZcR
ZacharyZcR marked this pull request as ready for review August 3, 2026 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant