-
-
Notifications
You must be signed in to change notification settings - Fork 12.9k
[Misc] Add run one batch script that supports profiling #32968
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[Misc] Add run one batch script that supports profiling #32968
Conversation
Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
|
Documentation preview: https://vllm--32968.org.readthedocs.build/en/32968/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new example script for running a single batch inference with optional profiling capabilities. The script is well-structured and serves its purpose as a clear example. I've identified one high-severity issue related to input validation for the profiling directory path, which could lead to a crash. My suggestion addresses this by adding a check to ensure the path is a valid directory.
| if not profile_dir: | ||
| raise ValueError("--profile-dir must be set when profiling is enabled.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script crashes if --profile-dir points to an existing file instead of a directory. This is because the underlying PyTorch profiler expects a directory path and will fail when trying to create it if a file with the same name exists. Adding a check to validate the path will make the script more robust and prevent unexpected crashes.
| if not profile_dir: | |
| raise ValueError("--profile-dir must be set when profiling is enabled.") | |
| if not profile_dir: | |
| raise ValueError("--profile-dir must be set when profiling is enabled.") | |
| import os | |
| if os.path.exists(profile_dir) and not os.path.isdir(profile_dir): | |
| raise ValueError( | |
| f"--profile-dir '{profile_dir}' exists and is not a directory.") |
|
Hi @LucasWilkinson, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
add a script that runs one batch and can optionally profile it; e.g.