feat(scripts): add Ultra script prerequisites - #4223
Conversation
Signed-off-by: Chen Cui <chcui@nvidia.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Light Code Review --- Bug in pack_sft_data.py: silent drop of --seq-length / --hf-path overrides (inline comment posted). When the user passes these flags but the recipe function does not accept them, the values are silently ignored. This should error or warn. --- text_generation.py: Distributed init changes look correct. --- Test coverage: Neither script has tests; the new argument-forwarding logic would benefit from unit tests. --- Suggested test cases: No perf tests impacted. |
yaoyu-33
added
area:data
Signed-off-by: Chen Cui <chcui@nvidia.com>
|
/claude review |
| assert kwargs["output_metadata_path"] == metadata_output | ||
| assert kwargs["packed_sequence_size"] == 2048 | ||
| assert kwargs["max_seq_length"] == 4096 | ||
| assert kwargs["num_tokenizer_workers"] == 1 |
There was a problem hiding this comment.
Bug: _PackedSequenceSpecs defaults num_tokenizer_workers to -1, and the script passes it through unchanged to prepare_packed_sequence_data. This assertion will fail.
| assert kwargs["num_tokenizer_workers"] == 1 | |
| assert kwargs["num_tokenizer_workers"] == -1 |
There was a problem hiding this comment.
I think the existing assertion is intentional here. The script sets cfg.dataset.packed_sequence_specs.num_tokenizer_workers = 1 before it calls prepare_packed_sequence_data(...) (see scripts/training/pack_sft_data.py around the worker cap), so the value forwarded to the packer should be 1 rather than the dataclass default -1. I reran the targeted test and it passes with the current assertion.
|
Light Code Review -- Bug: tests/unit_tests/scripts/test_pack_sft_data.py:220 -- The assertion num_tokenizer_workers == 1 will fail. The _PackedSequenceSpecs stub defaults num_tokenizer_workers to -1, and the script passes it through unchanged. The assertion should be == -1. -- Suggested test cases: test_pack_sft_data_packs_validation_split_when_both_paths_provided, test_pack_sft_data_rejects_val_input_without_val_output, test_pack_sft_data_falls_back_to_builder_pack_metadata, test_maybe_initialize_distributed_preserves_existing_env_vars. No perf tests impacted. |
Signed-off-by: Chen Cui <chcui@nvidia.com>
|
/ok to test f2f017c |
Signed-off-by: Chen Cui <chcui@nvidia.com> Signed-off-by: Vasudevan Rengasamy <vrengasamy@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com> (cherry picked from commit 1d0dab6)
2 participants
Summary
pack_sft_data.pyaccept recipe-supported sequence length and HF path overridesBlast Radius / Test Assessment
Validation
uv run --no-sync pre-commit run --files scripts/training/pack_sft_data.py scripts/inference/text_generation.pyuv run --no-sync python -m py_compile scripts/training/pack_sft_data.py scripts/inference/text_generation.py