Skip to content

fix(exporters): scope fp16 external-data cleanup to this model (data-loss) - #226

Merged
rylinjames merged 2 commits into
mainfrom
fix/fp16-convert-data-loss
Aug 28, 2026
Merged

fix(exporters): scope fp16 external-data cleanup to this model (data-loss)#226
rylinjames merged 2 commits into
mainfrom
fix/fp16-convert-data-loss

Conversation

@rylinjames

Copy link
Copy Markdown
Collaborator

Audit §3.3 / Part 1 #12 — a real data-loss bug.

Problem

convert_fp32_to_fp16's pre-save cleanup globbed every *.bin and *.data in dst.parent and unlinked them:

for pat in ("*.bin", "*.data"):
    for old in dst.parent.glob(pat):
        old.unlink()

The intent was only to clear leftover external data from a prior run of this model. But converting into a directory that also holds another model's weights (a shared export dir, an unrelated model.onnx.data) destroys those files.

Fix

Extracted _remove_stale_external_data(dst) — deletes only .bin/.data files whose name starts with dst.stem, never dst itself. Pure pathlib, unit-testable without onnx/torch.

Tests

tests/test_fp16_convert_external_data.py (5 cases, all pass):

  • removes this model's stale .bin
  • removes legacy .data / .onnx.data variants for this model
  • preserves sibling models' weights (the data-loss guard)
  • never unlinks dst itself
  • no-op when the dir is clean

ruff clean on touched files.

🤖 Generated with Claude Code

…loss)

Audit §3.3 / Part 1 #12.

convert_fp32_to_fp16's pre-save cleanup globbed every `*.bin` and `*.data`
file in `dst.parent` and unlinked them. The intent was only to clear leftover
external data from a prior run of the model being converted — but when a user
converts into a directory that also holds OTHER models' weight files (a shared
export dir, a folder with an unrelated `model.onnx.data`), those get destroyed.

Extracted `_remove_stale_external_data(dst)`: deletes only `.bin`/`.data`
files whose name starts with `dst.stem`, never `dst` itself. Pure pathlib, so
it's unit-testable without onnx/torch.

Added tests/test_fp16_convert_external_data.py (5 cases): removes this model's
stale .bin + legacy .data variants, preserves sibling models' weights (the
data-loss guard), never unlinks dst, no-op when clean. All pass; ruff clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@rylinjames

Copy link
Copy Markdown
Collaborator Author

I reproduced the remaining prefix-collision deletion (model2.bin and model_backup.data) and updated this branch. Cleanup now uses an exact allowlist for the current model.bin, legacy model.data, and legacy model.onnx.data paths; it no longer uses a prefix glob. The new regression preserves model2.bin, model_backup.data, and model-old.bin. Focused result: 6 passed; Ruff and diff checks pass.

@rylinjames
rylinjames merged commit 383fa24 into main Aug 28, 2026
7 checks passed
@rylinjames
rylinjames deleted the fix/fp16-convert-data-loss branch August 28, 2026 07:06
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