Skip to content

[fix] small improvements for dataloader, metrics, optimizer robustness, and ops - #797

Open
sherlockwu wants to merge 1 commit into
mainfrom
kan/pr1-infra
Open

[fix] small improvements for dataloader, metrics, optimizer robustness, and ops#797
sherlockwu wants to merge 1 commit into
mainfrom
kan/pr1-infra

Conversation

@sherlockwu

@sherlockwu sherlockwu commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Robustness

  • optimizer.py: a non-finite global grad norm now skips the optimizer step deterministically on every rank (clipping cannot rescale a NaN norm; one such Adam step NaNs the weights permanently).
  • mooncake_store.py: transient get_into failures (TRANSFER_FAIL/-800 under source-side contention) are retried with backoff instead of killing the run as a KeyError; the receive buffer of a failed attempt is quarantined — a timed-out transfer can still write into its destination after the call returns, and reusing that storage has corrupted the heap. Observed rate: ~9 per 100 steps on long 32K runs; an 8K validation run on this branch without the retry died within 12 steps.

Performance

  • feature_dataloader.py: make dataloader_num_workers actually multi-threaded. Previously the value only sized the buffer of finished batches — a single background thread fetched one batch at a time, capping delivery at 1/fetch-latency no matter how deep the buffer. Now the prefetch worker submits fetches to a num_workers-sized thread pool and the buffer carries futures, so up to num_workers batches materialize concurrently while leasing continues.

Useful for long context feature passing. On the 32K DSV4-Flash disaggregated recipe (2-rank consumer, 3× TP2 capture, identical stack; steady-state tails):

loader step time exposed fetch wait / step
one concurrent fetch (the previous design's behavior) 122.6 s 92.8 s
this PR, num_workers: 16 31.4 s 14.6 s
  • dflash_family_model.py: masked_fill instead of a per-microbatch pageable H2D copy; a device-side assert instead of a host sync right after a collective.

Observability

  • controller.py + loader perf counters: perf/data_wait_producer_s, perf/data_wait_fetch_s, perf/fetch_seconds_per_sample, perf/fetch_delivered_gib_per_s — attributes trainer data waits to "capture cannot keep up" vs "transfer is slow".

@sherlockwu
sherlockwu marked this pull request as draft September 1, 2026 19:33
@sherlockwu sherlockwu changed the title Data-plane robustness, perf attribution metrics, and FSDP memory knobs Data-plane robustness, perf attribution metrics, and DFlash perf fixes Sep 1, 2026
@sherlockwu
sherlockwu force-pushed the kan/pr1-infra branch 6 times, most recently from 6cb1549 to 0e58308 Compare September 2, 2026 06:23
@sherlockwu sherlockwu changed the title Data-plane robustness, perf attribution metrics, and DFlash perf fixes Infra fix for fetcher, metrics Sep 2, 2026
@sherlockwu sherlockwu changed the title Infra fix for fetcher, metrics Infra improvement for fetcher, metrics, optimizer robustness, and ops Sep 2, 2026
@sherlockwu
sherlockwu marked this pull request as ready for review September 2, 2026 06:28
…ution metrics

Infra-only changes, independent of any drafter architecture:

- feature_dataloader: make dataloader_num_workers actually multi-threaded.
  Previously it only sized the buffer of finished batches — one background
  thread fetched a batch at a time, capping delivery at 1/fetch-latency
  regardless of buffer depth. The prefetch worker now submits fetches to a
  num_workers-sized pool and the buffer carries futures (measured on the
  32K DSV4-Flash recipe: 122.6 s/step single-fetcher -> 31.4 s at 16)
- optimizer: skip the step deterministically on a non-finite global grad
  norm instead of NaN-ing the weights permanently
- controller + loader perf counters: perf/data_wait_producer_s,
  perf/data_wait_fetch_s, perf/fetch_seconds_per_sample,
  perf/fetch_delivered_gib_per_s — attribute trainer data waits to
  "capture cannot keep up" vs "transfer is slow"
- dflash_family_model: masked_fill instead of a per-microbatch pageable
  H2D copy; device-side assert instead of a host sync after all_reduce

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sherlockwu sherlockwu changed the title Infra improvement for fetcher, metrics, optimizer robustness, and ops Infra improvement for dataloader, metrics, optimizer robustness, and ops Sep 2, 2026
@sherlockwu sherlockwu changed the title Infra improvement for dataloader, metrics, optimizer robustness, and ops [fix] small improvements for dataloader, metrics, optimizer robustness, and ops Sep 2, 2026
maocheng23 pushed a commit that referenced this pull request Sep 3, 2026
state.mark_materialized(refs)
if not put_interruptibly((batch, refs)):
future = pool.submit(self._timed_make_batch, refs)
state.track_fetch(future)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

track_fetch() removes the future as soon as it completes, while mark_materialized() happens only when the consumer calls future.result().

So it would trigger a race?

self._store_get_tensor(key, dst)
return dst
except KeyError as exc:
self._quarantined_buffers.append(dst)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This _quarantined_buffers will not be released?

logger.warning("%s; retry %d/3 in %.0fs", exc, attempt + 1, delay)
time.sleep(delay)
dst = _alloc_from_spec(spec)
self._store_get_tensor(key, dst)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there is no more try catch?

try:
self._store_get_tensor(key, dst)
return dst
except KeyError as exc:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this retry only the explicit transient TRANSFER_FAIL/-800 status?

Comment thread specforge/optimizer.py
for mp in self.fp32_params:
mp.grad = None
self.last_grad_norm = grad_norm.detach()
return self.last_grad_norm

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)Here we still silently return, so there will be a empty step.

"perf/data_wait_fetch_s": (
loader["wait_fetch_s"] / steps
),
"perf/fetch_seconds_per_sample": (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is per batch not per sample right?
As the denominator is batches

@maocheng23

Copy link
Copy Markdown
Collaborator

Measured this branch (merged onto main c49bab49 + #783/#831/#832) on B300 with Qwen3.8-27B NVFP4 target + DFlash2 draft, max_length 8192 (~277 MB of hidden states per sample), Mooncake TCP, dataloader_num_workers: 8, steady-state windows of 130 optimizer steps:

layout main loader (one fetch in flight) this PR (8 concurrent fetches)
8 capture servers on node B + DP8 trainer on node A (cross-node, 800 GbE) 10.97 samples/s, trainer bubble 35-40%, fetch p50 0.46 s/sample 13.97 samples/s (+27%), bubble 5.6%, exposed fetch wait 25% of rank time, fetch p50 1.10 s/sample
4 servers + DP4 trainer, single node 7.81 samples/s, bubble 37-42%, fetch p50 0.23 s/sample 7.63 samples/s, bubble 6%, exposed fetch wait 28%, fetch p50 1.23 s/sample

So the pool converts the wait into overlapped fetches, but on one node the same ~2 GB/s reaches the trainers either way; the gain shows up when the consumer pulls from another node.

Two operational notes from the same runs: with 8 fetches in flight the default 500 ms and a 3 s Mooncake lease both produced LEASE_EXPIRED (-707); the new retry kept the run alive, but the backoff sleeps distort timing, so I ran with --default_kv_lease_ttl=10000 (harmless once #832 makes removal not lease-bound). And the perf/data_wait_fetch_s vs perf/data_wait_producer_s split matched the per-segment records from an external tracer, which is a nice confirmation of the attribution.

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.

2 participants