Skip to content

fix(nvme): run main() from the perf script __main__ blocks - #8344

Open
Anai-Guo wants to merge 1 commit into
deepspeedai:masterfrom
Anai-Guo:fix-nvme-perf-script-entrypoints
Open

Anai-Guo wants to merge 1 commit into
deepspeedai:masterfrom
Anai-Guo:fix-nvme-perf-script-entrypoints

Conversation

@Anai-Guo

Copy link
Copy Markdown
Contributor

Problem

deepspeed/nvme/perf_generate_param.py and deepspeed/nvme/perf_run_sweep.py each define a main() that parses arguments and hands them to the worker function, and a __main__ block that skips main() and calls the worker directly with no arguments:

def generate_main(log_dir):
    ...

def main():
    args = parse_arguments()
    if not validate_args(args):
        quit()
    print(f'Generate DeepNVMe configuration from {args.log_dir} logs')
    generate_main(args.log_dir)      # <-- correct

if __name__ == "__main__":
    generate_main()                  # <-- no arguments
def sweep_main(args):
    ...

def main():
    args = parse_sweep_arguments()
    print(f"Running DeepNVMe performance sweep on {args.nvme_dir}")
    sweep_main(args)                 # <-- correct

if __name__ == "__main__":
    sweep_main()                     # <-- no arguments

So running either module directly fails immediately:

$ python -m deepspeed.nvme.perf_generate_param
TypeError: generate_main() missing 1 required positional argument: 'log_dir'

$ python -m deepspeed.nvme.perf_run_sweep
TypeError: sweep_main() missing 1 required positional argument: 'args'

and both main() functions — the ones that actually do the argument parsing and validation — are referenced nowhere else in the tree, so they are dead code today.

Sibling baseline

deepspeed/nvme/parse_nvme_stats.py, in this same package, already does it the right way:

if __name__ == "__main__":
    main()
module __main__ calls runs?
parse_nvme_stats.py main() ✅
perf_generate_param.py generate_main() ❌ — this PR
perf_run_sweep.py sweep_main() ❌ — this PR

Scope

bin/ds_nvme_tune is not affected and is not touched: it does its own parsing and already passes the arguments through (sweep_main(args) / generate_main(args.log_dir)). The public deepspeed.nvme exports (sweep_main, generate_main, parse_sweep_arguments, ds_io_main) are unchanged — this only fixes direct module invocation.

Verification

I replayed the argument binding straight out of the AST rather than hand-copying a repro, so it cannot drift from the source: each worker's parameter list and every call site's shape are read from the files themselves, a signature-identical stub is generated, and the binding is replayed.

perf_generate_param.py: generate_main(log_dir)
   line 97   1 arg(s) -> BINDS OK          # inside main()
   line 101  0 arg(s) -> TypeError: generate_main() missing 1 required
                         positional argument: 'log_dir'    # the __main__ block

perf_run_sweep.py: sweep_main(args)
   line 315  1 arg(s) -> BINDS OK          # inside main()
   line 319  0 arg(s) -> TypeError: sweep_main() missing 1 required
                         positional argument: 'args'       # the __main__ block

After the change both __main__ blocks route through main(), which is the call site that already binds correctly.


🤖 Generated with Claude Code

perf_generate_param.py and perf_run_sweep.py each define a main() that parses
arguments and then hands them to the worker -- generate_main(args.log_dir) and
sweep_main(args). Their __main__ blocks skip main() and call the worker with no
arguments, so running either module directly fails immediately:

    TypeError: generate_main() missing 1 required positional argument: 'log_dir'
    TypeError: sweep_main() missing 1 required positional argument: 'args'

Both main() functions are referenced nowhere else, so they are dead code today.
parse_nvme_stats.py, in this same package, already calls main() from its
__main__ block. The bin/ds_nvme_tune console script is unaffected -- it does its
own argument parsing and already passes the arguments through.

Signed-off-by: Anai-Guo <antai12232931@outlook.com>
@Anai-Guo
Anai-Guo requested a review from tjruwase as a code owner August 28, 2026 13:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f209e117f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".


if __name__ == "__main__":
generate_main()
main()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add the required sign-off trailer

This non-merge commit has no Signed-off-by trailer, violating the repository's mandatory commit policy and potentially failing DCO enforcement; recreate the commit with --signoff before merging.

AGENTS.md reference: AGENTS.md:L8-L8

Useful? React with 👍 / 👎.

@Anai-Guo
Anai-Guo force-pushed the fix-nvme-perf-script-entrypoints branch from 4f209e1 to 6d4e2a9 Compare August 28, 2026 16:40
@gpu-test-labeler gpu-test-labeler Bot added the skip-gpu-test Requires positive evidence that every changed behavior can be validated without a GPU. label Nov 14, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-gpu-test Requires positive evidence that every changed behavior can be validated without a GPU.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant