-
Notifications
You must be signed in to change notification settings - Fork 78
[bugfix] correct dataloader checkpoint resume across forked workers and epochs #544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
b8fceac
6e269d8
c6bbfe5
5389e7f
51f6cf4
1e472f6
faa42a1
9a8dd14
1352019
587a992
e323234
4ca7dd6
87ee083
11850e2
d433720
5fafa37
4aea0fc
f9227ee
abf7498
595d0d8
ee33f5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| from torch import distributed as dist | ||
| from torch.utils.data import DataLoader | ||
|
|
||
| from tzrec.datasets.dataset import create_dataloader | ||
| from tzrec.datasets.parquet_dataset import ParquetDataset, ParquetReader, ParquetWriter | ||
| from tzrec.features.feature import create_features | ||
| from tzrec.protos import data_pb2, feature_pb2 | ||
|
|
@@ -235,6 +236,50 @@ def test_parquet_dataset_checkpoint_resume(self): | |
| # New offset should be less than or equal to acc checkpoint offset | ||
| self.assertLessEqual(new_offset, checkpoint_state_acc[key]) | ||
|
|
||
| def test_create_dataloader_checkpoint_state_reaches_workers(self): | ||
| """State passed to create_dataloader must reach forked workers. | ||
|
|
||
| create_dataloader eagerly starts persistent workers, so state applied | ||
| to the returned dataloader's dataset afterwards never reaches them; | ||
| the checkpoint_state argument applies it before the fork. | ||
| """ | ||
| feature_cfgs = self._create_feature_cfgs() | ||
| features = create_features(feature_cfgs) | ||
|
|
||
| with tempfile.TemporaryDirectory(prefix="tzrec_") as test_dir: | ||
| # 20000 rows at max_rows_per_file=5000 -> 4 files, so | ||
| # data_config.num_workers=2 survives the num_files clamp. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment justifies the fixture size with a constraint that doesn't apply:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 51f6cf4 — comment now justifies the fixture by rebalanced multi-worker intervals + a remaining tail; kept the 20000 rows since the reworked assertions iterate full passes. |
||
| self._create_test_parquet_data(test_dir, num_rows=20000) | ||
| input_path = f"{test_dir}/*.parquet" | ||
| data_config = data_pb2.DataConfig( | ||
| batch_size=128, | ||
| dataset_type=data_pb2.DatasetType.ParquetDataset, | ||
| fg_mode=data_pb2.FgMode.FG_NONE, | ||
| label_fields=["label"], | ||
| num_workers=2, | ||
| ) | ||
|
|
||
| dataloader1 = create_dataloader(data_config, features, input_path) | ||
| iterator1 = iter(dataloader1) | ||
| checkpoint_state = {} | ||
| for _ in range(4): | ||
| batch1 = next(iterator1) | ||
| update_dataloder_state(checkpoint_state, batch1.checkpoint_info) | ||
| self.assertGreater(len(checkpoint_state), 0) | ||
| del iterator1, dataloader1 | ||
|
|
||
| dataloader2 = create_dataloader( | ||
| data_config, features, input_path, checkpoint_state=checkpoint_state | ||
| ) | ||
| iterator2 = iter(dataloader2) | ||
| for _ in range(4): | ||
| batch2 = next(iterator2) | ||
| for key, new_offset in batch2.checkpoint_info.items(): | ||
| if key in checkpoint_state: | ||
| # without the pre-fork state, workers replay from row 0 | ||
| self.assertGreater(new_offset, checkpoint_state[key]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This guard makes the green path vacuous: when the fix works, resumed source keys are resumed_starts = set()
for _ in range(4):
batch2 = next(iterator2)
self.assertTrue(set(batch2.checkpoint_info).isdisjoint(checkpoint_state))
resumed_starts |= {int(k.rsplit(":", 1)[1]) for k in batch2.checkpoint_info}
self.assertEqual(resumed_starts, {v + 1 for v in checkpoint_state.values()})
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 51f6cf4 — phase 2 now iterates to exhaustion and asserts the affirmative invariants: key disjointness, presence of every |
||
| del iterator2, dataloader2 | ||
|
|
||
|
|
||
| class ParquetReaderTest(unittest.TestCase): | ||
| def setUp(self): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -588,22 +588,6 @@ def train_and_evaluate( | |
| # Build feature | ||
| features = _create_features(list(pipeline_config.feature_configs), data_config) | ||
|
|
||
| # Build dataloader | ||
| train_dataloader = create_dataloader( | ||
| data_config, features, pipeline_config.train_input_path, mode=Mode.TRAIN | ||
| ) | ||
| eval_dataloader = None | ||
| if pipeline_config.eval_input_path: | ||
| # pyre-ignore [16] | ||
| gl_cluster = train_dataloader.dataset.get_sampler_cluster() | ||
| eval_dataloader = create_dataloader( | ||
| data_config, | ||
| features, | ||
| pipeline_config.eval_input_path, | ||
| mode=Mode.EVAL, | ||
| gl_cluster=gl_cluster, | ||
| ) | ||
|
|
||
| ckpt_manager = checkpoint_util.CheckpointManager( | ||
| pipeline_config.model_dir, | ||
| keep_checkpoint_max=train_config.keep_checkpoint_max, | ||
|
|
@@ -637,12 +621,33 @@ def train_and_evaluate( | |
| "--continue_train)" | ||
| ) | ||
|
|
||
| # Restore dataloader checkpoint state | ||
| # Restore dataloader checkpoint state before building the dataloader: | ||
| # create_dataloader eagerly starts persistent workers, which keep a | ||
| # fork-time copy of the dataset, so state set afterwards is invisible | ||
| # to them. | ||
| dataloader_state: Optional[Dict[str, Any]] = None | ||
| if ckpt_path and continue_train: | ||
| dataloader_state = ckpt_manager.restore_dataloader_state(ckpt_path) | ||
| if dataloader_state: | ||
| train_dataloader.dataset.load_state_dict(dataloader_state) | ||
|
|
||
| # Build dataloader | ||
| train_dataloader = create_dataloader( | ||
| data_config, | ||
| features, | ||
| pipeline_config.train_input_path, | ||
| mode=Mode.TRAIN, | ||
| checkpoint_state=dataloader_state, | ||
| ) | ||
|
Comment on lines
+659
to
+666
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that the state actually reaches the workers, note it is re-applied on every epoch, not just the resumed one: nothing ever clears
Pre-PR this was latent (the state never reached forked workers at all); this PR activates it in the default multi-worker config. One subtlety if you fix it with consume-once semantics: clearing on entry to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5389e7f with the consume-once-on-exhaustion approach: |
||
| eval_dataloader = None | ||
| if pipeline_config.eval_input_path: | ||
| # pyre-ignore [16] | ||
| gl_cluster = train_dataloader.dataset.get_sampler_cluster() | ||
| eval_dataloader = create_dataloader( | ||
| data_config, | ||
| features, | ||
| pipeline_config.eval_input_path, | ||
| mode=Mode.EVAL, | ||
| gl_cluster=gl_cluster, | ||
| ) | ||
|
|
||
| sampler_type = _get_sampler_type(data_config) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseReader.load_state_dictstores this dict by reference (dataset.py:555), andtrain_and_evaluatepasses the very same object on to_train_and_evaluate, which mutates it in place on every step viaupdate_dataloder_state(main.py:462) and at save time. Withnum_workers >= 1the fork-time copy makes this accidentally safe, but withdata_config.num_workers < 1the DataLoader runs in-process, so the reader aliases the live, ever-growing dict — the nextiter(train_dataloader)(epoch 2) recomputes intervals from offsets accumulated during this run and skips that data.KafkaReader'son_assignrebalance callback also reads_checkpoint_stateat arbitrary later times, cross-thread. A defensive copy breaks the aliasing:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5389e7f —
create_dataloadernow passesdict(checkpoint_state)toload_state_dict, decoupling the reader from the training loop's accumulating dict.