-
Notifications
You must be signed in to change notification settings - Fork 863
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
Include SAM Fast in torch.compile nightly benchmark workflow #2856
Conversation
@@ -4,6 +4,7 @@ | |||
pip uninstall torchtext torchdata torch torchvision torchaudio -y | |||
|
|||
# Install nightly PyTorch and torchvision from the specified index URL | |||
pip install chardet |
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.
What is this for ?
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.
Included this dependency because one of the previous benchmark runs failed because of the absence of this module: https://github.com/pytorch/serve/actions/runs/7255062486/job/19764967013
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.
Was this happening locally? I didnt notice this. If not, can you please move this to the dependencies section of the benchmark run?
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.
I think I noticed it locally as well. I will include it in the benchmark_model_dependencies.sh
script which you suggested.
@@ -39,5 +39,8 @@ jobs: | |||
sudo apt-get update -y | |||
sudo apt-get install -y apache2-utils | |||
pip install -r benchmarks/requirements-ab.txt | |||
chmod +x examples/large_models/segment_anything_fast/install_segment_anything_fast.sh |
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.
Please make this a separate step
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.
Will update the PR.
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.
I'm thinking this should be a script under benchmarks. You can call it benchmark_model_dependencies.sh
. So, if there are more things in the future, we can add it there
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.
Sounds good. I will create this script.
uses: actions/setup-python@v4 | ||
with: | ||
python-version: 3.8 | ||
python-version: 3.10.9 |
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.
can we just have 3.10?
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.
One of the previous runs had failed when I specified just 3.10: https://github.com/pytorch/serve/actions/runs/7255048620/job/19764929373
Commit ID: bcda479
We can choose the versions according to https://raw.githubusercontent.com/actions/python-versions/main/versions-manifest.json.
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.
You need to use quotes, check this workflow https://github.com/pytorch/serve/blob/master/.github/workflows/regression_tests_cpu_binaries.yml#L20
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.
Oh I understand. I will update the workflow accordingly.
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.
Looks good overall. Some minor edits needed
Updated PR according to suggestions. |
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.
LGTM
Description
Please read our CONTRIBUTING.md prior to creating your first pull request.
The objective of this PR is to include the SAM Fast model with weights corresponding to vit_b and a process batch size of 4.
Testing:
Ran benchmark workflow in the commit .
Testing after 1st revision:
Ran benchmark workflow in the commit e89c1ba.