Skip to content

[fix][train] Shut down DataLoader workers on exit - #1998

Open
bvolpato wants to merge 2 commits into
NovaSky-AI:mainfrom
bvolpato:bvolpato/shutdown-dataloader-workers
Open

[fix][train] Shut down DataLoader workers on exit#1998
bvolpato wants to merge 2 commits into
NovaSky-AI:mainfrom
bvolpato:bvolpato/shutdown-dataloader-workers

Conversation

@bvolpato

@bvolpato bvolpato commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

StatefulDataLoader worker processes normally stop when its iterator is exhausted or finalized. Training exceptions could bypass that cleanup because RL and SFT entrypoints did not shut down their train and evaluation dataloaders in finally blocks.

This change:

  • adds one compatibility helper around torchdata 0.11's iterator-owned worker shutdown
  • shuts down RL train and evaluation dataloaders after successful or failed runs
  • moves SFT shutdown into its entrypoint's finally path and closes every train/eval dataloader
  • logs cleanup failures without replacing the original training exception

This covers process exits that unwind Python normally. Abrupt parent termination such as SIGKILL remains outside this change's scope.

Testing

  • pre-commit run --all-files
  • uv run --isolated --extra skyrl-train --extra dev pytest tests/train/ tests/backends/skyrl_train/ --ignore=tests/backends/skyrl_train/gpu -m "not vllm" -q
    • 1467 passed, 5 skipped, 5 deselected
  • focused tests start real spawned train/eval workers, invoke shutdown twice, and verify every worker exits

CI dependency

Current main contains a Black formatting failure in an unrelated Tinker test. #2002 fixes that base-branch failure so this PR can rerun its gated CPU jobs.

@bvolpato
bvolpato marked this pull request as ready for review August 6, 2026 15:18

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request ensures that trainer resources, specifically DataLoader worker processes, are properly shut down when training finishes or encounters an error. It introduces a shutdown_dataloader utility function to stop worker processes owned by a StatefulDataLoader, and integrates this into the shutdown methods of both SFTTrainer and Trainer. Additionally, finally blocks are added to the entry points to guarantee that shutdown is always executed. Feedback suggests improving the readability of the dataloader shutdown logic in SFTTrainer by separating the train and evaluation dataloader shutdown blocks instead of using an intermediate list.

Comment thread skyrl/train/sft_trainer.py Outdated
@bvolpato
bvolpato force-pushed the bvolpato/shutdown-dataloader-workers branch from f32b273 to d74713a Compare August 6, 2026 17:44
@bvolpato
bvolpato force-pushed the bvolpato/shutdown-dataloader-workers branch from d74713a to 1a025cd Compare August 7, 2026 00:21
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