[orestis-z]

Purpose

The root logger's RichHandler formats and writes synchronously inside whatever thread logged. With the default log_freq of 1 that lands in every training step: flattening the metric dict, formatting ~20 values, and rendering rich markup costs milliseconds, and the GIL it holds while doing so delays the next step's first CUDA call. This puts a QueueHandler on the root and runs the RichHandler from a QueueListener thread, so the step pays an enqueue and the rendering happens while the device is busy.

QueueHandler.prepare renders the message to a string before enqueuing, which would hand FormatDictFilter a str and print a bare dict repr instead of the train/k=v form downstream tooling greps. _SnapshotQueueHandler instead flattens mapping messages into a fresh dict, which also freezes the values at log time so a caller that reuses its metrics dict cannot have later values printed against an earlier step. The listener only starts when the root has no handlers yet — the case where basicConfig would otherwise be a no-op, leaving a listener thread with nothing feeding it — and is stopped at exit so queued records still reach the console.

Measured

Single H100, eagle3, synthetic batches, full 151936-token vocab, total_seq_len 2048, 10 warmup + 50 measured steps, 3 reps, median of the per-run means, log_freq=1, scripts/benchmark.py. This PR against #1296:

Qwen3-0.6B	Qwen3-1.7B	Qwen3-8B	Qwen3-32B
1.029x	1.042x	1.015x	1.006x

A local wall-clock harness that drives the same trainer with the step timer's torch.accelerator.synchronize() patched out — so the device is free to run ahead of the host — reports the opposite sign, 0.990x at 2048 and 0.987x at 4096 on Qwen3-0.6B. Read together: a 1–4% win when the host is on the critical path, a wash when it is not. The benchmark's console is a fast local pipe, so most of what is measured here is the GIL the rendering holds rather than the write itself; whether it is worth more when logs go somewhere slower is untested.

It touches only train/logger.py, so it stands or falls on its own.

Tests

pytest tests/unit/train/test_logger.py

New cases cover that records reach the handler through the listener, that mapping messages still format as train/k=v, that the values are snapshotted at log time, that nothing is installed when the root already has handlers, and that the listener is stopped at exit. Passes.

Checklist

I have filled in:

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan/results, such as providing test command and pasting the results.
• (Optional) The necessary documentation update.
• I (a human) have written or reviewed the code in this pr to the best of my ability.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mergify]

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

	Protection	Waiting on
🔴	Require approval from approved reviewers list	👀 reviews

🔴 Require approval from approved reviewers list

Waiting for any of

• approved-reviews-by = dsikka
• approved-reviews-by = fynnsu
• approved-reviews-by = orestis-z
• approved-reviews-by = rahul-tuli
• approved-reviews-by = shanjiaz

This rule is failing.

All pull requests must have at least one approving review from a member of the approved reviewers list before merging.

• any of:
  • approved-reviews-by = dsikka
  • approved-reviews-by = fynnsu
  • approved-reviews-by = orestis-z
  • approved-reviews-by = rahul-tuli
  • approved-reviews-by = shanjiaz

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @orestis-z.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork