Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamo benchmarks direct arg passing doesn't work #126543

Closed
NiklasZ opened this issue May 17, 2024 · 4 comments
Closed

Dynamo benchmarks direct arg passing doesn't work #126543

NiklasZ opened this issue May 17, 2024 · 4 comments
Labels
module: ci Related to continuous integration module: dynamo oncall: pt2 triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@NiklasZ
Copy link

NiklasZ commented May 17, 2024

馃悰 Describe the bug

I wanted to run the hugging face dynamo benchmark (benchmarks/dynamo/huggingface.py) multiple times with different arguments directly within Python and noticed that the function signature of the main running script def main(runner, original_dir=None, args=None) (in benchmarks/dynamo/common.py) supports passing in the commandline args as a list of strings. So I did as much using the following code:

from huggingface import HuggingfaceRunner, main
args = ["--performance", "--inference", "--backend", "inductor", "--progress", "--device", "cpu"]
main(HuggingfaceRunner(), args=args)

However, this doesn't actually work and yields an infinite subprocess loop where it keeps running the same model again and again:

Running model 1/46
Running model 1/46
Running model 1/46
Running model 1/46
etc.

The reason for this is that there are various calls in the benchmark code that use sys.argv directly instead of the args given to argparse. E.g from common.py:

                subprocess.check_call(
                    [sys.executable] + sys.argv + [f"--only={name}"],
                    timeout=timeout,
                    env=env,
                )

Notably, there is a workaround which is to keep the args and sys.argv in sync, but it is pretty ugly:

if len(sys.argv) == 1:
    sys.argv.extend(args)
else:
    args = sys.argv[1:]

I would suggest either replacing the sys.argv or updating the function signatures to not allow adding argument lists directly and removing related code.

Versions

Collecting environment information...
PyTorch version: 2.3.0+cu121
Is debug build: False
CUDA used to build PyTorch: 12.1
ROCM used to build PyTorch: N/A

OS: Ubuntu 20.04.5 LTS (x86_64)
GCC version: (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0
Clang version: Could not collect
CMake version: version 3.29.2
Libc version: glibc-2.31

Python version: 3.10.14 (main, Apr 8 2024, 18:03:12) [GCC 9.4.0] (64-bit runtime)
Python platform: Linux-5.15.0-102-generic-x86_64-with-glibc2.31
Is CUDA available: False
CUDA runtime version: No CUDA
CUDA_MODULE_LOADING set to: N/A
GPU models and configuration: No CUDA
Nvidia driver version: No CUDA
cuDNN version: No CUDA
HIP runtime version: N/A
MIOpen runtime version: N/A
Is XNNPACK available: True

cc @seemethere @malfet @pytorch/pytorch-dev-infra @ezyang @msaroufim @bdhirsh @anijain2305 @chauhang @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng

@NiklasZ NiklasZ changed the title Dynamo benchmarks use both argparser and sys.argv Dynamo benchmarks direct arg passing doesn't work May 17, 2024
@xmfan xmfan added module: ci Related to continuous integration triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels May 20, 2024
@xmfan
Copy link
Member

xmfan commented May 20, 2024

The benchmark suite only supports running it from CLI python benchmarks/dynamo/huggingface.py .... If you want to invoke that programmatically, one way could be to use subprocess to run the CLI command

@ezyang
Copy link
Contributor

ezyang commented May 20, 2024

If you want to send in some patches to stop snooping sys.argv I'm willing to help review them

@NiklasZ
Copy link
Author

NiklasZ commented May 21, 2024

Thanks, I will take a look at what changes are needed to replace the sys.argv usage. If it doesn't complicate the code much, I'll submit a PR in a few days. Otherwise I'll try the subprocess route.

@NiklasZ
Copy link
Author

NiklasZ commented May 26, 2024

Hi , so I took a look and replacing sys.argv with args will not fix the issue for me. I would still need this awkward if-else check. I instead went with @xmfan's suggestion to generate my various CLI parameter combinations in my python script and then call huggingface.py via subprocess.check_call(...) in a loop. Accordingly I don't think there's any point to submitting changes, so I will close this.

Thanks again!

@NiklasZ NiklasZ closed this as completed May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: ci Related to continuous integration module: dynamo oncall: pt2 triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
Archived in project
Development

No branches or pull requests

4 participants