Skip to content

fix: assorted correctness bugs across utils, data, models, and tooling - #2

Open
xy200303 wants to merge 5 commits into
Tencent:mainfrom
xy200303:fix/assorted-correctness-bugs
Open

fix: assorted correctness bugs across utils, data, models, and tooling#2
xy200303 wants to merge 5 commits into
Tencent:mainfrom
xy200303:fix/assorted-correctness-bugs

Conversation

@xy200303

Copy link
Copy Markdown

This PR bundles several small, independently verifiable bug fixes (one commit per area, easy to cherry-pick):

  • utils/logging: _get_logger_level caught ValueError, but getattr(logging, name) raises AttributeError for an unknown level name, and the handler never assigned the fallback — an invalid ANGELSPEC_LOG_LEVEL crashed at import time. Now catches AttributeError and falls back to WARNING.
  • utils/usp: split_usp_batch assigned a local variable named usp_chunk_size, shadowing the module-level function and making every call raise UnboundLocalError. Renamed the local to chunk_len.
  • utils/profiling: with the default profile_step_start=0, profile_step_end=0, the torch profiler schedule got active=0 and tripped torch's schedule assertion. The step range is now validated up front with a clear error message.
  • data/parse: the registered deepseek-r1-distill template has end_of_turn_token=None, so GeneralParser.parse crashed with TypeError on re.escape(None) during dataset tokenization (the existing |$ alternative already covers a missing end token). Also _ensure_pad_token no longer clobbers a legitimate pad_token_id of 0 (truthiness check → is None).
  • data/utils: the Hub dataset path dropped every column outside a small keep-list, silently discarding per-sample tools / reasoning_effort fields that dataset.py threads into the chat template. Also .csv / .tsv / .txt are advertised as supported local extensions but were fed to the JSON loader; they now route to the csv / text loaders.
  • data/dataset: the tokenized-dataset cache key omitted prompt_key, so re-running the same data file with a different prompt_key silently reused a stale cache.
  • models/draft: the linear and dynamic RoPE-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 sibling 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.
  • transfer/mooncake: the last_hidden_states tensor spec on GET 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.
  • examples/tools: 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. tools/generate_data.py read resp after the message loop even though it is only assigned for user turns — a conversation with no user message crashed with UnboundLocalError; it now returns an error record like the other validation failures.

Verification

  • ruff check clean (also fixed the pre-existing F823 in utils/usp.py it flagged).
  • Test suite: 150 passed / 1 skipped; the 24 failures are identical to main and purely environmental (missing mooncake package, and torch.compile needs MSVC on this Windows box).
  • Reproduced the split_usp_batch UnboundLocalError and the invalid-log-level crash before/after the fix.

xy200303 added 5 commits July 30, 2026 20:05
…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.
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