fix: assorted correctness bugs across utils, data, models, and tooling - #2
Open
xy200303 wants to merge 5 commits into
Open
fix: assorted correctness bugs across utils, data, models, and tooling#2xy200303 wants to merge 5 commits into
xy200303 wants to merge 5 commits into
Conversation
…and USP crash - _get_logger_level caught ValueError, but getattr() raises AttributeError for an unknown level name, and the handler never assigned the fallback; an invalid ANGELSPEC_LOG_LEVEL crashed at import time. Catch AttributeError and fall back to WARNING. - split_usp_batch assigned a local variable named usp_chunk_size, shadowing the module-level function and making every call raise UnboundLocalError. Rename the local to chunk_len. - _create_torch_profiler produced active=0 with the default profile_step_start/end, tripping torch's schedule assertion. Validate the step range up front with a clear error.
…sues - GeneralParser.parse called re.escape() on end_of_turn_token, which is None for the registered deepseek-r1-distill template, raising TypeError during dataset tokenization. Guard with 'or ""'; the existing |$ alternative already covers a missing end token. - _ensure_pad_token used a truthiness check that clobbered a legitimate pad_token_id of 0; check for None instead. - The Hub dataset path dropped every column outside a small keep-list, silently discarding per-sample 'tools' and 'reasoning_effort' fields that dataset.py threads into the chat template. Keep them. - .csv/.tsv/.txt are advertised as supported local extensions but were fed to the JSON loader; route them to the csv/text loaders instead. - The tokenized-dataset cache key omitted prompt_key, so re-running with a different prompt_key silently reused a stale cache.
The linear and dynamic scaling branches in Llama3 Eagle3 _init_rope, and the linear, dynamic, and yarn branches in DeepSeek Eagle3 _init_rope, omitted base=, silently defaulting to 10000 while every other branch passes the configured rope_theta. A draft config combining these scaling types with rope_theta != 10000 (e.g. Llama-3's 500000) would train with the wrong rotation frequencies.
The last_hidden_states tensor spec looked up dtypes['hidden_states'] instead of dtypes['last_hidden_states']. If the two are ever stored with different dtypes the receiver computes the wrong byte size (the host-buffer path raises, the GPU-direct path silently misinterprets).
- Both Hy3 example scripts passed training.num_nodes, which is not a schema field (the field is training_num_nodes), so the CLI override crashed at startup with ConfigKeyError. - generate_data.py read resp after the message loop even though resp is only assigned for user turns; a conversation with no user message crashed with UnboundLocalError. Return an error record instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR bundles several small, independently verifiable bug fixes (one commit per area, easy to cherry-pick):
_get_logger_levelcaughtValueError, butgetattr(logging, name)raisesAttributeErrorfor an unknown level name, and the handler never assigned the fallback — an invalidANGELSPEC_LOG_LEVELcrashed at import time. Now catchesAttributeErrorand falls back toWARNING.split_usp_batchassigned a local variable namedusp_chunk_size, shadowing the module-level function and making every call raiseUnboundLocalError. Renamed the local tochunk_len.profile_step_start=0, profile_step_end=0, the torch profiler schedule gotactive=0and tripped torch's schedule assertion. The step range is now validated up front with a clear error message.deepseek-r1-distilltemplate hasend_of_turn_token=None, soGeneralParser.parsecrashed withTypeErroronre.escape(None)during dataset tokenization (the existing|$alternative already covers a missing end token). Also_ensure_pad_tokenno longer clobbers a legitimatepad_token_idof0(truthiness check →is None).tools/reasoning_effortfields thatdataset.pythreads into the chat template. Also.csv/.tsv/.txtare advertised as supported local extensions but were fed to the JSON loader; they now route to thecsv/textloaders.prompt_key, so re-running the same data file with a differentprompt_keysilently reused a stale cache.linearanddynamicRoPE-scaling branches in Llama3-Eagle3_init_rope, and thelinear,dynamic, andyarnbranches in DeepSeek-Eagle3_init_rope, omittedbase=, silently defaulting to 10000 while every sibling branch passes the configuredrope_theta. A draft config combining these scaling types withrope_theta != 10000(e.g. Llama-3's 500000) would train with the wrong rotation frequencies.last_hidden_statestensor spec on GET looked updtypes["hidden_states"]instead ofdtypes["last_hidden_states"]; if the two are ever stored with different dtypes the receiver computes the wrong byte size.training.num_nodes, which is not a schema field (the field istraining_num_nodes), so the CLI override crashed at startup withConfigKeyError.tools/generate_data.pyreadrespafter the message loop even though it is only assigned for user turns — a conversation with no user message crashed withUnboundLocalError; it now returns an error record like the other validation failures.Verification
ruff checkclean (also fixed the pre-existing F823 inutils/usp.pyit flagged).mainand purely environmental (missingmooncakepackage, andtorch.compileneeds MSVC on this Windows box).split_usp_batchUnboundLocalErrorand the invalid-log-level crash before/after the fix.