Skip to content

fix(tools): k3_tokenizer --ctest writes its fixture to a hardcoded /tmp path - #1229

Merged
JustVugg merged 2 commits into
JustVugg:devfrom
Unknown-Findout:fix/k3-tokenizer-tmp-path
Aug 28, 2026
Merged

JustVugg merged 2 commits into
JustVugg:devfrom
Unknown-Findout:fix/k3-tokenizer-tmp-path

Conversation

@Unknown-Findout

@Unknown-Findout Unknown-Findout commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

k3_tokenizer.py --ctest writes its case fixture to a hardcoded path:

cases = "/tmp/k3_tok_cases.bin"
with open(cases, "wb") as f:

This is the same pattern the repo has already moved away from twice, and I only found it because the existing pins pointed at it.

The rule is already written in this repo, twice

tests/test_stops.c:84 and tests/test_pipe_block.c:146:

Relative to the CWD, like test_compat_direct's TMPF - NOT "/tmp/...". These binaries are built by MinGW into native Windows .exe files, which resolve Windows paths: "/tmp" is not one, so mkdtemp there fails ENOENT and the whole make check goes red on the windows job (and only there).

And setup.sh was moved off its own /tmp probe to OMP_PROBE=".colibri-omp-probe-$$", with test_cuda_test_makefile.test_setup_openmp_probe_does_not_require_tmp pinning it there. tools/k3_tokenizer.py is the file that pass missed.

One correction to the obvious framing, because I checked

I expected this to fail outright on Windows and it did not. fopen(..., "w") creates the file but not the directory, and Git Bash creates C:/tmp on install, so on my box the old line writes fine. The failure is conditional on whether /tmp already exists, which is exactly why it survived this long: it works on most dev machines and breaks on clean ones. I measured it both ways rather than assuming, since assuming is what would have made this a bogus report.

The fixed filename is the platform-independent half, and it is unconditional: two concurrent --ctest runs collide on the same path, and nothing ever removed the file.

The change

tempfile.mkstemp next to the output file, in a try/finally:

fd, cases = tempfile.mkstemp(prefix=".colibri-k3-cases-", suffix=".bin",
                             dir=os.path.dirname(os.path.abspath(out)))

The output directory rather than the system temp dir on purpose: it is provably writable, because tokenizer.json was written to it successfully a few lines earlier. So this cannot trade one unwritable location for another.

Verification

Three tests in tests/test_k3_tokenizer_tmp.py, same shape as the existing setup.sh pin: no quoted /tmp path in the file, the fixture goes through mkstemp, and os.unlink sits inside the finally rather than on the happy path.

Negative control run rather than assumed - reverting tools/k3_tokenizer.py alone fails all three, restoring it passes all three:

revert tool -> FAILED (failures=3)
restore     -> OK

Full python suite, before and after:

Ran 591 tests   errors=2, skipped=64      (identical both ways)

Both errors are pre-existing and unrelated: test_cuda_test_makefile shells out to make, which is absent on this host.

The --ctest path itself is not exercised here, and I want to be straight about that - it needs tiktoken plus a compiled test_tok_kimi, so the tests read the source as text the way the setup.sh pin does. The edit is small enough to read, but CI is the check that matters for the runtime path.

Tested on Windows, RTX 3090 (sm_86), CUDA 13.1, Python 3.11.

Unknown-Findout and others added 2 commits August 25, 2026 14:17
…/tmp

k3_tokenizer.py --ctest wrote its case fixture to a hardcoded
"/tmp/k3_tok_cases.bin". That is the same pattern the repo has already
moved away from twice:

  tests/test_stops.c and tests/test_pipe_block.c both carry the rule in
  the source, that a path must not be rooted at /tmp because the windows
  job builds native .exe files and that is not a Windows path, and that
  the whole `make check` goes red on the windows job and only there.

  setup.sh was moved off its own /tmp probe to
  OMP_PROBE=".colibri-omp-probe-$$", and
  test_cuda_test_makefile.test_setup_openmp_probe_does_not_require_tmp
  pins it there.

The failure is conditional rather than universal, and worth stating
precisely: fopen(..., "w") creates the file but not the directory, so on
a box where something has already created C:/tmp (Git Bash does) the old
line writes fine, and on a clean runner it does not. Measured both ways
rather than assumed.

The fixed FILENAME is a second and platform-independent bug: two
concurrent runs of --ctest collide on it, and nothing ever removed it.

Now tempfile.mkstemp next to the output file, in a try/finally so a
failing ctest still cleans up. That directory is provably writable
because the tokenizer.json was written to it successfully a few lines
earlier, so this cannot trade one unwritable location for another.

Not touched, but the same family and flagged for a maintainer decision:
kimi_k3.c:1045 defaults K3_VALIDATE_OUT to "/tmp/k3_val". That one
degrades cleanly, printing "[K3-VAL] cannot open ... disabled" and
carrying on, so it is a lower-severity instance and changing a
documented env-var default is not mine to make unasked.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…test

Same shape as
test_cuda_test_makefile.test_setup_openmp_probe_does_not_require_tmp,
which pins setup.sh off its old /tmp probe. Without a pin the line comes
back the next time someone needs a scratch file, since the failure is
invisible on any dev box that happens to have /tmp.

Three assertions: no quoted /tmp path anywhere in the file, the fixture
is created via tempfile.mkstemp, and os.unlink sits inside the finally
rather than on the happy path.

The source is read as text rather than executed because --ctest needs
tiktoken and a compiled test_tok_kimi, neither of which the python job
has.

Negative control run rather than assumed: reverting tools/k3_tokenizer.py
alone fails all three, restoring it passes all three. Full suite is 591
tests with the same 2 pre-existing errors before and after, both of them
test_cuda_test_makefile shelling out to a make that is absent on this
host.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@JustVugg
JustVugg merged commit 2d27a29 into JustVugg:dev Aug 28, 2026
23 checks passed
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.

2 participants