fix(storage#795): default run num_files_train to the datasize minimum (#804)

The reporter's run wanted 1,170,301 files while `datasize` recommended
257,173, and aborted because the objects did not exist. Root cause: the
run reads `dataset.num_files_train` from the workload YAML reference
default (retinanet_b200: 1,170,301) whenever the user does not override
it. `datasize` computes a smaller, memory-derived minimum for the
system. Because `skip_listing` is forced on, each rank reconstructs
filenames as `{prefix}_{idx}_of_{num_files_train}.{ext}` instead of
listing the directory, so a run defaulting to 1,170,301 reconstructs
`_of_1170301` names that miss every `_of_257173` object the user
generated per the datasize/datagen hint.

Add `_resolve_num_files_train()`: on the run/configview path, when the
user has not passed `--params dataset.num_files_train`, default it to the
datasize-computed minimum (same cluster_information + declared-vs-measured
reconciliation datasize uses), so the interval calc, the run, and
check_num_files_train all agree and the reconstructed names match the
generated dataset.

Deliberately scoped:
  - Never touches datagen. The generation host set (and its memory)
    typically differs from the run host set, and submitters routinely
    over-generate (up to ~10x) to reuse one dataset; generation size is
    the submitter's choice, threaded explicitly via the datasize->datagen
    hint. datagen has no cluster_information, so the method no-ops.
  - Respects an explicit --params dataset.num_files_train override — a
    submitter running against a deliberately larger generated set sets it
    and that value wins.

dataset.num_files_train is already CLOSED_ALLOWED, so the auto-resolved
value passes the run-checker whether user- or tool-originated; updated the
TOOL_INJECTED_PARAMS comment in training.py to note the new tool origin.

Adds tests/unit/test_resolve_num_files_train.py (5 cases: resolve,
explicit-override respected, datagen no-op, computed==default no-op,
graceful compute-failure).

Author: FileSystemGuy

mlpstorage_py/benchmarks/dlio.py

@@ -336,6 +336,106 @@ def _apply_skip_listing_params(self, num_files_override: Optional[int] = None):
                 f'(~{checks:,} HEAD checks at startup)'
             )
 
+    def _resolve_num_files_train(self):
+        """storage#795: default ``dataset.num_files_train`` to the datasize
+        minimum for THIS run instead of the workload YAML reference value.
+
+        The ``num_files_train`` in the workload YAML is a reference placeholder
+        (retinanet_b200: 1,170,301) sized for a large system — it is *not* the
+        count a given submitter should read. ``datasize`` computes the real
+        per-system minimum from the 5x-memory and 500-step rules
+        (rules/utils.calculate_training_data_size). When the user has not
+        explicitly passed ``--params dataset.num_files_train``, resolve it to
+        that minimum so the run reads exactly the dataset ``datasize``
+        recommended (and that the ``datasize``-emitted ``datagen`` hint was
+        told to generate).
+
+        Why this matters now: ``skip_listing`` (forced on by
+        ``_apply_skip_listing_params``) makes every rank reconstruct filenames
+        as ``{prefix}_{idx}_of_{num_files_train}.{ext}`` rather than listing the
+        directory. If the run's ``num_files_train`` differs from the value the
+        dataset was generated with, the reconstructed ``_of_{total}`` names
+        match no object and the startup HEAD checks all miss — exactly the
+        storage#795 abort, where the run defaulted to the YAML 1,170,301 while
+        the reporter had generated the (smaller) datasize-recommended set.
+
+        Deliberately scoped:
+          * Never touches datagen. The generation host set (and its memory)
+            typically differs from the run host set, and submitters routinely
+            over-generate (up to ~10x) to reuse one dataset across
+            experiments; generation size is the submitter's choice, threaded
+            explicitly via the datasize->datagen hint. datagen has no
+            ``cluster_information`` here (dlio.py __init__ skips host-info
+            collection for datagen), so the guard below no-ops for it.
+          * Respects an explicit ``--params dataset.num_files_train`` — a
+            submitter running against a deliberately larger generated set (or
+            any specific count) sets it themselves and that value wins.
+
+        Computes against the same ``cluster_information`` (and the same
+        declared-vs-measured reconciliation) that ``datasize`` used, so the
+        resolved value matches the generated dataset's ``_of_{total}`` naming
+        and satisfies ``check_num_files_train``'s ``configured >= required``
+        gate by construction.
+        """
+        # An explicit user override always wins — never second-guess it.
+        if 'dataset.num_files_train' in self.params_dict:
+            return
+
+        cluster_info = getattr(self, 'cluster_information', None)
+        if not cluster_info:
+            # No memory basis to size against (datagen path). Leave the YAML
+            # default in place; generation size is the submitter's call.
+            return
+
+        # Recompute silently: check_num_files_train (during verify_benchmark)
+        # logs the authoritative "Minimum file count dictated by..." RESULT a
+        # moment later, so a second copy here — plus the storage#785
+        # reconciliation warning, which runs never emit today — would just be
+        # noise. Our own INFO below carries the "why".
+        import logging as _logging
+        quiet = _logging.getLogger("mlpstorage_py.num_files_train.resolve")
+        if not quiet.handlers:
+            quiet.addHandler(_logging.NullHandler())
+        quiet.setLevel(_logging.CRITICAL + 1)
+        quiet.propagate = False
+
+        try:
+            computed, _, _ = calculate_training_data_size(
+                self.args,
+                cluster_info,
+                self.combined_params['dataset'],
+                self.combined_params['reader'],
+                quiet,
+            )
+        except (ValueError, KeyError) as exc:
+            self.logger.debug(f'num_files_train auto-resolution skipped: {exc}')
+            return
+
+        dataset_params = self.combined_params.setdefault('dataset', {})
+        try:
+            yaml_default = int(dataset_params.get('num_files_train'))
+        except (TypeError, ValueError):
+            yaml_default = None
+
+        if computed == yaml_default:
+            # YAML default already equals the minimum — nothing to change.
+            return
+
+        self.params_dict['dataset.num_files_train'] = computed
+        dataset_params['num_files_train'] = computed
+
+        default_note = (
+            f' (workload YAML default was {yaml_default:,})'
+            if yaml_default is not None else ''
+        )
+        self.logger.info(
+            f'num_files_train not set; defaulting to the datasize minimum for '
+            f'this system: {computed:,}{default_note}. The run reads this many '
+            f'files from --data-dir; if you generated a different count (e.g. '
+            f'an over-generated dataset), pass --params '
+            f'dataset.num_files_train=<N>. See storage#795.'
+        )
+
     @staticmethod
     def _strip_uri_scheme(value):
         # DLIO obj_store_lib treats storage_root as a bare bucket/prefix and
@@ -697,6 +797,11 @@ def __init__(self, args, **kwargs):
         # computed until ``datasize()`` runs. The datasize path calls this
         # method itself once the recommendation is known.
         if self.args.command != "datasize":
+            # storage#795: default num_files_train to the datasize-computed
+            # minimum (not the YAML reference) BEFORE skip_listing derives its
+            # validation interval, so the interval, the INFO log, the DLIO run,
+            # and check_num_files_train all agree on the count the run reads.
+            self._resolve_num_files_train()
             self._apply_skip_listing_params()
 
         if self.args.command not in ("datagen", "datasize"):

mlpstorage_py/rules/run_checkers/training.py

@@ -76,8 +76,12 @@ class TrainingRunRulesChecker(RunRulesChecker):
         # entry, so without this line reportgen loading a datasize run's
         # metadata marks the run INVALID for a value the tool itself wrote.
         # (num_files_train / num_subfolders_train are also tool-written on
-        # datasize runs but appear in CLOSED_ALLOWED_PARAMS for run commands
-        # where they are genuine user overrides — leave those to that path.)
+        # datasize runs, and num_files_train is additionally auto-resolved to
+        # the datasize minimum on run/configview when the user does not pass it
+        # explicitly — see _resolve_num_files_train, storage#795. Both cases
+        # are covered by its CLOSED_ALLOWED_PARAMS entry, which accepts the
+        # value whether it originated from the user or the tool, so no
+        # TOOL_INJECTED entry is needed here.)
         'dataset.total_disk_bytes',
     })
 

tests/unit/test_resolve_num_files_train.py

@@ -0,0 +1,125 @@
+"""
+Regression test for storage#795 (run-path half).
+
+The workload YAML ``num_files_train`` (retinanet_b200: 1,170,301) is a reference
+placeholder sized for a large system — not the count a given submitter should
+read. ``datasize`` computes the real per-system minimum from the 5x-memory /
+500-step rules. Because ``skip_listing`` is forced on, every rank reconstructs
+filenames as ``{prefix}_{idx}_of_{num_files_train}.{ext}`` instead of listing the
+directory, so if the run's ``num_files_train`` differs from the value the dataset
+was generated with, the reconstructed ``_of_{total}`` names match no object and
+the startup HEAD checks all miss. That is the storage#795 abort: the run defaulted
+to the YAML 1,170,301 while the reporter had generated the (smaller) datasize set.
+
+``_resolve_num_files_train`` fixes this by defaulting the run/configview
+``num_files_train`` to the datasize-computed minimum when the user did not pass
+``--params dataset.num_files_train`` — while leaving datagen (different host set,
+deliberate over-generation) and explicit user overrides untouched. These tests
+exercise the method directly against the same computation ``check_num_files_train``
+uses, patched to a known value so the wiring — not the arithmetic — is asserted.
+"""
+
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+from mlpstorage_py.benchmarks.dlio import TrainingBenchmark
+
+_MODULE = "mlpstorage_py.benchmarks.dlio.calculate_training_data_size"
+
+
+def _make_stub(*, params_dict=None, cluster_information=object(),
+               combined_num_files_train=1_170_301):
+    """Minimal shape ``_resolve_num_files_train`` reads off ``self``."""
+    return SimpleNamespace(
+        args=SimpleNamespace(),
+        params_dict=dict(params_dict or {}),
+        cluster_information=cluster_information,
+        combined_params={
+            'dataset': {'num_files_train': combined_num_files_train},
+            'reader': {'batch_size': 1},
+        },
+        logger=MagicMock(),
+    )
+
+
+def test_resolves_to_datasize_minimum_when_not_overridden():
+    """The retinanet_b200 reporter scenario: user did not pass num_files_train,
+    so the run must default to the computed minimum (257,173), not the YAML
+    reference (1,170,301) — otherwise skip_listing reconstructs ``_of_1170301``
+    names that miss every generated ``_of_257173`` object."""
+    stub = _make_stub(combined_num_files_train=1_170_301)
+
+    with patch(_MODULE, return_value=(257_173, 0, 83_055_820_561)):
+        TrainingBenchmark._resolve_num_files_train(stub)
+
+    # Both the DLIO override surface and the effective combined config are
+    # updated so the interval calc, the run, and the run-checker all agree.
+    assert stub.params_dict['dataset.num_files_train'] == 257_173
+    assert stub.combined_params['dataset']['num_files_train'] == 257_173
+
+    logged = ' '.join(str(c.args[0]) for c in stub.logger.info.call_args_list)
+    assert '257,173' in logged
+    assert '1,170,301' in logged  # names the YAML default it superseded
+    assert 'storage#795' in logged
+
+
+def test_explicit_user_override_is_respected():
+    """A submitter running against a deliberately over-generated dataset passes
+    ``--params dataset.num_files_train=N``; that value must win and the method
+    must not recompute or log."""
+    stub = _make_stub(
+        params_dict={'dataset.num_files_train': 2_000_000},
+        combined_num_files_train=2_000_000,
+    )
+
+    with patch(_MODULE) as calc:
+        TrainingBenchmark._resolve_num_files_train(stub)
+        calc.assert_not_called()
+
+    assert stub.params_dict['dataset.num_files_train'] == 2_000_000
+    assert stub.logger.info.call_count == 0
+
+
+def test_no_cluster_information_is_a_noop():
+    """The datagen path has no cluster_information (host-info collection is
+    skipped for datagen, and its host set / memory typically differ from the
+    run's anyway). With no memory basis to size against, leave the YAML default
+    in place and do not touch params_dict."""
+    stub = _make_stub(cluster_information=None, combined_num_files_train=1_170_301)
+
+    with patch(_MODULE) as calc:
+        TrainingBenchmark._resolve_num_files_train(stub)
+        calc.assert_not_called()
+
+    assert 'dataset.num_files_train' not in stub.params_dict
+    assert stub.combined_params['dataset']['num_files_train'] == 1_170_301
+    assert stub.logger.info.call_count == 0
+
+
+def test_computed_equal_to_yaml_default_does_not_inject_override():
+    """When the computed minimum already equals the YAML default, there is
+    nothing to change — and, crucially, the tool must not manufacture a
+    redundant override that would show up in the audit trail as a user tune."""
+    stub = _make_stub(combined_num_files_train=1_170_301)
+
+    with patch(_MODULE, return_value=(1_170_301, 0, 1)):
+        TrainingBenchmark._resolve_num_files_train(stub)
+
+    assert 'dataset.num_files_train' not in stub.params_dict
+    assert stub.combined_params['dataset']['num_files_train'] == 1_170_301
+    assert stub.logger.info.call_count == 0
+
+
+def test_compute_failure_degrades_gracefully():
+    """If the sizing computation cannot run (missing inputs on some path),
+    swallow it at debug level and leave the YAML default — never crash the run
+    over an auto-defaulting convenience."""
+    stub = _make_stub(combined_num_files_train=1_170_301)
+
+    with patch(_MODULE, side_effect=ValueError("no memory basis")):
+        TrainingBenchmark._resolve_num_files_train(stub)
+
+    assert 'dataset.num_files_train' not in stub.params_dict
+    assert stub.combined_params['dataset']['num_files_train'] == 1_170_301
+    assert stub.logger.info.call_count == 0
+    stub.logger.debug.assert_called_once()