Fix #376: argparse conflicting option string: --file#377
Open
idevasena wants to merge 1 commit into
Open
Conversation
Remove duplicate --file/--object declaration from add_universal_arguments Add regression test mlpstorage_py/tests/test_issue_376_file_arg_conflict.py Update test_open_closed_flag_recognition.py _build_parser to call add_storage_type_arguments
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
FileSystemGuy
approved these changes
May 15, 2026
Contributor
|
Please review (and merge if appropriate): @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
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.
Fix #376: argparse "conflicting option string: --file" by removing duplicate declaration
Fixes #376.
Root cause
--file/--objectare declared in two places inmlpstorage_py/cli/common_args.py:add_universal_arguments()— line ~198add_storage_type_arguments()— line ~323Every benchmark subparser builder (
add_training_arguments,add_checkpointing_arguments,add_vectordb_arguments,add_kvcache_arguments) calls both functions on the same parser, so argparse raisesat parser-construction time — which fires on the very first
mlpstorage --helpinvocation, before any user arg is parsed. This is also why--debugappeared not to work in the report: the parser never finishes being built, so no flag gets evaluated.How the regression was introduced
Two commits, in sequence:
PR Bug fixes and performance enhancements: object storage, checkpointing, Parquet loading #359 (
39e657d, May 8 2026) — Correctly extracted--file/--objectout ofadd_universal_argumentsinto a newadd_storage_type_arguments(). After this commit--filelived in one place.PR Fix: Issue 349 open flag recognition #352 (
258483b, May 13 2026) — Fix for Running training with --open doesn't seem to be recognized #349--openrecognition. The substantive change was rewriting the--open/--closedmutex group insideadd_universal_arguments, but the diff also re-introduced the--file/--objectmutex block (withrequired=True) — most likely a merge-conflict resolution against a pre-Bug fixes and performance enhancements: object storage, checkpointing, Parquet loading #359 base. From that commit onward,--fileexists in both functions and every benchmark subparser builder crashes.Verifiable from
git show 258483b -- mlpstorage_py/cli/common_args.py: the diff adds back anaccess_proto.add_argument("--file", ...)block that PR #359 had explicitly removed eight days earlier.Fix
Remove the duplicate
--file/--objectblock fromadd_universal_arguments. Replace with a comment that pins the invariant so a future merge can't silently re-introduce it. Restores PR #359's intent: declare those flags once, inadd_storage_type_arguments, attached only to benchmark subparsers (training, checkpointing, vectordb, kvcache).Utility subparsers (
reports,history,lockfile) that only calladd_universal_argumentsno longer carry--file/--object, which is the correct behavior — they don't need storage-type selection.Tests
New file:
mlpstorage_py/tests/test_issue_376_file_arg_conflict.py— seven cases pinning the invariant:--file/--objectdeclared in exactly one adder (add_storage_type_arguments)add_training_arguments,add_checkpointing_arguments,add_vectordb_arguments,add_kvcache_arguments) construct cleanly when invoked the same waycli_parser.pyinvokes them--debugparses correctly post-fixUpdated:
mlpstorage_py/tests/test_open_closed_flag_recognition.py—_build_parsernow also callsadd_storage_type_arguments, mirroring how real benchmark subparsers are built, so the open/closed CLI tests keep working.Validation
mlpstorage --helpArgumentError: --file conflictSystemExit(0)mlpstorage training --helpSystemExit(0)mlpstorage checkpointing --helpSystemExit(0)mlpstorage vectordb --helpSystemExit(0)mlpstorage kvcache --helpSystemExit(0)mlpstorage --debug --helpSystemExit(0)tests/unit/test_cli.py::TestAddUniversalArguments(8 tests)mlpstorage_py/tests/test_open_closed_flag_recognition.py::TestOpenClosedCLIFlags(4 tests)mlpstorage_py/tests/test_issue_376_file_arg_conflict.py(7 tests, new)The 7 pre-existing unit-test failures in
TestAddUniversalArgumentswere caused by the same regression (therequired=Truere-introduction made everyparse_argscall without--fileexit with code 2) — they likely went unnoticed because CI wasn't running them on PR #352.Diff stats