Repository navigation
Conversation
9e342fa to
71c35e2
Compare
YidaWeng
left a comment
There was a problem hiding this comment.
The production fix is correct, but the new test is not compatible with current main after rebase. Requesting changes for that test failure.
Verification
On main (e7c847821), the issue's exact save/reload matrix fails:
routed_experts/sampling_mask/top_logprobs:PydanticSerializationError: invalid utf-8 sequence...sampling_mask=[[1]]: write succeeds,read_episodesraisesTypeError: a bytes-like object is required, not 'str'- The CLI path (
trace.model_dump_json(indent=2, exclude_none=True)) fails the same way.
On this PR head, all four cases save and reload successfully, raw fields are absent from traces.jsonl, node arrays are None, and logprobs is preserved. The CLI dump with EXCLUDE_FIELDS also succeeds. write_episode's nested exclude={"traces": {"__all__": EXCLUDE_FIELDS}} does not over-exclude typed task data or normal trace fields.
Problem after rebase
This branch is based on c134007 and is behind main. I merged current main into a local scratch branch. The four new parametrizations then fail at:
assert saved.nodes[-1].token_ids == [10, 20]with ValueError: The truth value of an array with more than one element is ambiguous, because main now stores token_ids / logprobs as numpy arrays (from #2961). The full merged suite reports 254 tests, 4 failed, 19 skipped.
Applying this compatibility change makes the test pass:
- assert saved.nodes[-1].token_ids == [10, 20]
- assert saved.nodes[-1].logprobs == [-0.5]
+ assert saved.nodes[-1].token_ids.tolist() == [10, 20]
+ assert saved.nodes[-1].logprobs.tolist() == [-0.5]With that change:
- the four new params: 4 passed
- full merged
tests/unit: 254 tests, 235 passed, 19 skipped, 0 failed pre-commit run --files tests/unit/test_trace.py: ruff check/format pass
Please rebase onto current main and apply the tolist() assertions. I did not push to your branch.
|
Thanks, reproduced all four NumPy assertion failures after merging The four save/reload cases now pass, and the full unit suite is 235 passed, 19 skipped. Ruff check/format and the pre-push type check pass across the repository. Full pre-commit setup hits an npm |
write_episode dumped routed-experts, sampling-mask and top-logprob arrays through the msgpack byte serializer into JSON, so saving a train-client trace raised on non-UTF-8 bytes or wrote a row read_episodes could not load. Apply the EXCLUDE_FIELDS record contract that Episode.to_record already uses. Fixes PrimeIntellect-ai#2956
702ec3e to
2c72e01
Compare
Fixes #2956
Saving an episode containing routed experts, sampling masks, or top logprobs could fail on non-UTF-8 array bytes, or write a row that
read_episodescould not load.Apply the existing
EXCLUDE_FIELDSrecord exclusions inwrite_episode. It keeps its typed adapter, preserving task/trace subclass fields and the existing float precision. The msgpack wire continues to carry the training arrays.Rebased onto
mainatf5f5c9a.vf-evalwas removed in #2610, so the earlier change to its per-trace JSON output is dropped;vf-rundoes not print traces. The regression assertions use.tolist()for the NumPy streams introduced by #2961.Validation on Python 3.14.2 with
uv sync --frozen:tests/unit: 238 passed, 19 skipped. Skips: Harbor unavailable (1), Docker unavailable (9), missingPRIME_API_KEY(9).ty check verifierspass. The markdownlint hook could not be installed locally (nodeenv rejects the alpha Node release); no Markdown files change in this PR.Live model/runtime integration tests were not run.
Note
Exclude raw training arrays from episodes written by
trace_store.write_episodewrite_episodein trace_store.py now passesEXCLUDE_FIELDSto the JSON dump for every trace, so routed experts, sampling masks, and top logprobs are not saved in episode filestest_saved_episode_omits_raw_training_arraysin test_trace.py that writes and reloads an episode and checks the excluded fields stay unsetMacroscope summarized 2c72e01.