[benchmarks] Run some models with smaller batch sizes. #6542
Conversation
ysiraichi
requested review from
cota,
vanbasten23,
frgossen,
golechwierowicz and
zpcore
|
This PR should only be merged after: #6518 |
| @@ -144,6 +144,48 @@ | |||
| "hf_T5_generate", | |||
| } | |||
|
|
|||
| # This list was extracted from PyTorch's repository: benchmarks/dynamo/torchbench.py | |||
| FORCE_AMP_FOR_FP16_BF16_MODELS = { | |||
There was a problem hiding this comment.
These lists feel prone to divergence. Is this how PyTorch does it, too?
There was a problem hiding this comment.
Yukio: can you extract these lists from where they are so that we can import them? That will eliminate any maintenance burden from us (i.e. I don't want us to have to manually keep these lists in sync with the ones in Pytorch.)
IIRC you did something similar with the deny list being extracted into a YAML file.
There was a problem hiding this comment.
Yes. There is a YAML file for skipped models. These models aren't included in that. I guess I could make the change to include these in the PyTorch repo.
What is the point of this? Is this to just get numbers for Inductor as close to the HUD as possible? |
|
That too. But another thing is that this may also prevent OOMs that we are getting even on inductor. |
ysiraichi
force-pushed
the
ysiraichi/fix-batch-size
branch
from
aa6dd72
to
d6101fa
Compare
|
@frgossen @golechwierowicz do you think we can merge this PR? |
benchmarks/torchbench_model.py
Outdated
| FORCE_FP16_FOR_BF16_MODELS = {"vision_maskrcnn"} | ||
|
|
||
| # Some models have large dataset that doesn't fit in memory. Lower the batch | ||
| # size to test the accuracy. | ||
| # This list was extracted from PyTorch's repository: benchmarks/dynamo/torchbench.py |
There was a problem hiding this comment.
Do you think we can factor these lists out there and reuse them? That way we would be more robust against divergence.
|
What worries me a little about PRs like this is that we will almost certainly diverge over time. Do you think we can import and reuse the lists from PyTorch? |
|
Right. I will try to move those lists to a YAML file and, then, update this PR. |
|
Waiting for pytorch/pytorch#120299 |
|
Sounds good. Ty! |
ysiraichi
force-pushed
the
ysiraichi/fix-batch-size
branch
from
d6101fa
to
10419ed
Compare
|
@frgossen @golechwierowicz I think this PR is ready for review. Could you review it when you have some time? Here's a summary of the changes:
|
| its lists of models into sets of models. | ||
| """ | ||
|
|
||
| benchmarks_dynamo_dir = find_near_file( |
There was a problem hiding this comment.
I think we can make the assumtion that the xla root is at pytorch/xla. Allowing this flexibility with find_near_file feels like it will be hard to debug eventually
There was a problem hiding this comment.
I think @zpcore had a setup where pytorch and xla were sibling folders.
There was a problem hiding this comment.
Would it make sense to agree on one setup so keep things simpler?
There was a problem hiding this comment.
Sure. I don't mind.
@zpcore Thoughts?
There was a problem hiding this comment.
If we decide to make the assumption, we should specify it in https://github.com/pytorch/xla/blob/master/benchmarks/README.md
There was a problem hiding this comment.
I will merge this PR, and open another one for this change.
There was a problem hiding this comment.
Maybe the better solution is to add the file location in setup.py in the future. We can use import pkg_resources to find the file location.
This PR adapts the code in PyTorch main repo, so that a few models are executed with smaller batch sizes. This is an effort towards making the benchmarking scripts behavior closer.
In summary, 3 new sets are introduced:
USE_SMALL_BATCH_SIZE: batch sizes for trainingINFERENCE_SMALL_BATCH_SIZE: batch sizes for inferenceDONT_CHANGE_BATCH_SIZE: models whose batch size can't be changed in the command linecc @miladm