Summary
Ensure --data-dir for training cannot be silently defaulted. Today the argparse layer has no default, but enforcement has gaps and a hardcoded value living in DLIO workload YAMLs acts as an effective inherited default when no override is supplied. The benchmark should fail loudly any time the user has not explicitly provided --data-dir via the CLI or a YAML config file.
Current state
| Layer |
Behaviour |
Status |
argparse (mlpstorage_py/cli/training_args.py:171-178) |
No default= kwarg; default is None. |
✅ Already correct |
File-mode enforcement (mlpstorage_py/cli_parser.py:167-175) |
parser.error() fires before apply_yaml_config_overrides. A user supplying data_dir: via --config-file for file mode is rejected before YAML has a chance to populate it. |
❌ Bug |
Object-mode enforcement (mlpstorage_py/cli/training_args.py:269-276) |
sys.exit(EXIT_CODE.INVALID_ARGUMENTS) after YAML. |
✅ Works |
datasize command |
Excluded from the enforcement check at cli_parser.py:169. No fallback elsewhere. |
❌ Slips through |
| Env-var fallback |
No MLPS_DATA_DIR / MLPSTORAGE_DATA_DIR exists in the codebase. |
(Doesn't exist; intentionally out of scope) |
DLIO workload YAMLs — configs/dlio/workload/*.yaml |
Hardcode dataset.data_folder: data/<model>/ (e.g. data/unet3d/, data/resnet50, data/dlrm/, data/flux/). mlpstorage_py/benchmarks/dlio.py:410 only calls add_datadir_param() when self.args.data_dir is truthy, so when --data-dir is unset and enforcement is skipped (datasize, or any escape route), DLIO silently uses this relative-path default. |
❌ Silent inherited default |
Proposal
Single post-YAML check in validate_training_arguments (or the dispatcher) that fires for every training command (datasize, datagen, run, configview) regardless of data_access_protocol. Remove the early file-mode check in cli_parser.py so YAML config files can populate data_dir for either protocol. Error message should be actionable and point to both the CLI flag and the --config-file YAML key:
ERROR: --data-dir is required for training <command>.
Specify --data-dir <path> on the command line, or set 'data_dir:' in the file
passed via --config-file. There is no fallback default — the DLIO workload
YAML's data_folder will not be used.
Out of scope (deferred)
- Verifying that the effective
--data-dir actually lives on the system under test. Ideal — currently a user can point at a path that doesn't exist on the SUT and not notice until the benchmark stalls. The ideal check would stat() the path (or HEAD the object prefix) on each MPI rank's host and refuse to start otherwise. Decision: too much codebase disruption at this stage of the release cycle; record here so it isn't lost.
- Adding a
MLPS_DATA_DIR env-var fallback. Out of scope for this issue per the requesting user: the goal is to plug the silent-default leak, not expand the parameter-setting surface.
- Clearing
data_folder from the DLIO workload YAMLs. Higher blast radius (affects anyone running DLIO directly), and the proposed enforcement makes the YAML default unreachable from the mlpstorage CLI anyway, so not strictly necessary.
Test plan
- Unset
--data-dir, file mode, all four commands → fail with the new message (datasize currently slips through; this is the main regression class to cover).
- Unset
--data-dir, object mode, all four commands → fail with the new message.
--data-dir on CLI, all commands × both protocols → pass.
--data-dir only in --config-file YAML, all commands × both protocols → pass (currently broken for file mode).
- Confirm
dlio.py:410's add_datadir_param() is reached on every training command after the fix.
Files in scope
mlpstorage_py/cli_parser.py — remove the early file-mode block at L167–175.
mlpstorage_py/cli/training_args.py — extend the existing validate_training_arguments check to cover all commands and both protocols.
mlpstorage_py/cli/help_formatter.py — already lists --data-dir under "Required:" for the relevant commands; no change needed.
- Tests:
tests/unit/test_cli.py, tests/unit/test_cli_parser.py, tests/unit/test_parser_modes.py — add cases for the previously-uncovered datasize and --config-file-file-mode paths.
Summary
Ensure
--data-dirfor training cannot be silently defaulted. Today the argparse layer has no default, but enforcement has gaps and a hardcoded value living in DLIO workload YAMLs acts as an effective inherited default when no override is supplied. The benchmark should fail loudly any time the user has not explicitly provided--data-dirvia the CLI or a YAML config file.Current state
mlpstorage_py/cli/training_args.py:171-178)default=kwarg; default isNone.mlpstorage_py/cli_parser.py:167-175)parser.error()fires beforeapply_yaml_config_overrides. A user supplyingdata_dir:via--config-filefor file mode is rejected before YAML has a chance to populate it.mlpstorage_py/cli/training_args.py:269-276)sys.exit(EXIT_CODE.INVALID_ARGUMENTS)after YAML.datasizecommandcli_parser.py:169. No fallback elsewhere.MLPS_DATA_DIR/MLPSTORAGE_DATA_DIRexists in the codebase.configs/dlio/workload/*.yamldataset.data_folder: data/<model>/(e.g.data/unet3d/,data/resnet50,data/dlrm/,data/flux/).mlpstorage_py/benchmarks/dlio.py:410only callsadd_datadir_param()whenself.args.data_diris truthy, so when--data-diris unset and enforcement is skipped (datasize, or any escape route), DLIO silently uses this relative-path default.Proposal
Single post-YAML check in
validate_training_arguments(or the dispatcher) that fires for every training command (datasize,datagen,run,configview) regardless ofdata_access_protocol. Remove the early file-mode check incli_parser.pyso YAML config files can populatedata_dirfor either protocol. Error message should be actionable and point to both the CLI flag and the--config-fileYAML key:Out of scope (deferred)
--data-diractually lives on the system under test. Ideal — currently a user can point at a path that doesn't exist on the SUT and not notice until the benchmark stalls. The ideal check wouldstat()the path (or HEAD the object prefix) on each MPI rank's host and refuse to start otherwise. Decision: too much codebase disruption at this stage of the release cycle; record here so it isn't lost.MLPS_DATA_DIRenv-var fallback. Out of scope for this issue per the requesting user: the goal is to plug the silent-default leak, not expand the parameter-setting surface.data_folderfrom the DLIO workload YAMLs. Higher blast radius (affects anyone running DLIO directly), and the proposed enforcement makes the YAML default unreachable from the mlpstorage CLI anyway, so not strictly necessary.Test plan
--data-dir, file mode, all four commands → fail with the new message (datasize currently slips through; this is the main regression class to cover).--data-dir, object mode, all four commands → fail with the new message.--data-diron CLI, all commands × both protocols → pass.--data-dironly in--config-fileYAML, all commands × both protocols → pass (currently broken for file mode).dlio.py:410'sadd_datadir_param()is reached on every training command after the fix.Files in scope
mlpstorage_py/cli_parser.py— remove the early file-mode block at L167–175.mlpstorage_py/cli/training_args.py— extend the existingvalidate_training_argumentscheck to cover all commands and both protocols.mlpstorage_py/cli/help_formatter.py— already lists--data-dirunder "Required:" for the relevant commands; no change needed.tests/unit/test_cli.py,tests/unit/test_cli_parser.py,tests/unit/test_parser_modes.py— add cases for the previously-uncovered datasize and--config-file-file-mode paths.