Fix: Issue 349 open flag recognition#352
Open
idevasena wants to merge 2 commits intomlcommons:mainfrom
Open
Conversation
added 2 commits
April 27, 2026 02:21
The `--open` flag was silently ignored because of two intertwined bugs: 1. CLI parser: `--open` was defined with `action="store_false", dest="closed"`, which meant passing `--open` produced the exact same Namespace as passing nothing (`args.closed=False`, no `args.open` attribute). Callers had no way to tell the two cases apart. 2. verify_benchmark() in benchmarks/base.py: the guard `if not self.args.closed and not hasattr(self.args, "open"):` used hasattr on an attribute that was never created, so it was equivalent to `if not self.args.closed:`. Running with `--open` therefore routed through the 'Running the benchmark without verification for open or closed configurations' warning branch and skipped formal verification entirely. As a downstream consequence, overrides legitimate under OPEN (e.g. `dataset.num_files_train=425000` in mlcommons#349) were never accepted and the run crashed inside DLIO on a `num_subfolders == len(filenames)` assertion. Fix: * common_args.py: give `--open` its own `dest="open"` with `action="store_true"`. `--open` and `--closed` are now independent booleans, both default False, still mutually exclusive. * benchmarks/base.py: use `getattr(self.args, 'open'/'closed', False)`; the 'no verification' warning now fires only when *neither* flag is passed. The OPEN-allowed branch is now correctly gated on `open_mode` rather than the ambiguous `self.args.closed == False`. * benchmarks/kvcache.py: verification now triggers for `--open` too, not just `--closed`. * Adds mlpstorage_py/tests/test_open_closed_flag_recognition.py — 11 tests pinning (a) CLI parser produces distinguishable `open`/`closed`/neither namespaces, (b) verify_benchmark routes each case correctly, (c) kvcache guard fires for both submission flags.
…tics Follow-up to the mlcommons#349 fix. These tests currently cannot execute on main because they import `mlpstorage.benchmarks.base` instead of `mlpstorage_py.benchmarks.base` (broader package-rename migration that lives outside this PR). But once that import issue is resolved, the test logic should already be correct for the post-fix behavior: * The `benchmark` fixture now sets `open=False` alongside `closed=True` so the Namespace matches what the fixed argparse definition produces. * `test_exits_for_open_when_closed_required` — explicit `open=False`. * `test_allows_open_with_open_flag` — now sets `open=True` (rather than just `closed=False`, which the pre-fix code accepted only by accident). * `test_returns_true_with_closed_false_no_open_attr` renamed to `test_returns_true_with_neither_flag_set` and updated: both `closed` and `open` are explicitly `False`, matching the new parser defaults. * `test_full_workflow` fixture gets `open=False` for consistency. No test expectations are weakened; the only semantic change is `test_allows_open_with_open_flag`, which now actually exercises --open.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
FileSystemGuy
approved these changes
Apr 27, 2026
Contributor
FileSystemGuy
left a comment
FileSystemGuy
left a comment
There was a problem hiding this comment.
Please review: @russfellows
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Running
mlpstorage training run --open ...produces the warning "Running the benchmark without verification for open or closed configurations" and--open-allowed parameter overrides (e.g.dataset.num_files_train=425000) are rejected, which later causes anassert num_subfolders == len(filenames)crash inside DLIO.Root cause
Two linked bugs:
mlpstorage_py/cli/common_args.py(lines 211–224) —--openwas defined withaction="store_false", dest="closed", so passing--openproduced the exact sameNamespaceas passing nothing at all (args.closed=False, noargs.openattribute). There was no way for downstream code to distinguish the two cases.mlpstorage_py/benchmarks/base.pyline 789 — the guardif not self.args.closed and not hasattr(self.args, "open")usedhasattron an attribute that was never created, making it equivalent toif not self.args.closed:. That routed--openinto the "no verification" warning branch and skipped formal verification entirely. The DLIO crash is the downstream symptom of the override never being accepted.Fix
cli/common_args.py:--opennow has its owndest="open"withaction="store_true". Both flags are independent booleans, both defaultFalse, still mutually exclusive.benchmarks/base.py:verify_benchmark()usesgetattrfor both flags; "no verification" warning now fires only when neither flag is passed. The OPEN-allowed branch is correctly gated onopen_moderather than the ambiguousself.args.closed == False.benchmarks/kvcache.py: verification now triggers for--openas well as--closed.Tests
New file:
mlpstorage_py/tests/test_open_closed_flag_recognition.py— 11 regression tests pinning:open/closed/neither namespaces (4 tests)verify_benchmark()routes each case correctly, including the--openregression for issue Running training with --open doesn't seem to be recognized #349 (6 tests)kvcache.pyguard fires for both submission flags (1 test)Legacy
tests/unit/test_benchmarks_base.pyfixtures updated to match the new Namespace shape. Notably,test_allows_open_with_open_flagnow actually exercises--open(settingargs.open=True) rather than accidentally acceptingclosed=Falseas a proxy.Test results
pytest mlpstorage_py/tests/: 112 passed → 123 passed on cleanmain.Zero new regressions.
Fixes #349