Generate dynamic arg parsing for closed versus open#360
Closed
FileSystemGuy wants to merge 2 commits intomainfrom
Closed
Generate dynamic arg parsing for closed versus open#360FileSystemGuy wants to merge 2 commits intomainfrom
FileSystemGuy wants to merge 2 commits intomainfrom
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 #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 #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 ✍️ ✅ |
Contributor
Author
|
Please review: @russ @idevasena This should be merged after PR-352. There will very likely be conflicts, so I'll fix them one PR352 is merged. |
Contributor
Author
|
The branch wasn't created correctly, so it had the wrong contents/diff. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
As we are parsing through the options on the command line, dynamically generate the argparsing controls for the remaining options and arguments on the command line.
The purpose of all this is so that the user cannot use, or even see in the help message, the options that are not permitted in CLOSED, but they can see them if they're running in OPEN. This makes it much less likely (impossible?) for them to make a mistake and give an option that isn't permitted in a CLOSED submission. It also makes the submission validation checker much more robust because if the run was for a CLOSED submission, we know that the user cannot use CLI options that are not permitted in CLOSED.