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.

Sign up for GitHub

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

Jump to bottom

Fix PyTorch CI HUD dashboard missing perf numbers: hf_Whisper #1935

Closed
wants to merge 4 commits into from
Closed

Fix PyTorch CI HUD dashboard missing perf numbers: hf_Whisper #1935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
850559b
improve batch size error messaging
xmfan Sep 26, 2023
12a31f3
hf_Whisper set DEFAULT_TRAIN_BSIZE
xmfan Sep 26, 2023
ff92563
move up batch size check
xmfan Sep 26, 2023
4c96110
skip train unit test
xmfan Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions torchbenchmark/models/hf_Whisper/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

class Model(HuggingFaceModel):
task = SPEECH.RECOGNITION
DEFAULT_TRAIN_BSIZE = 8
DEFAULT_EVAL_BSIZE = 8
DEFAULT_EVAL_CUDA_PRECISION = "fp16"

Expand Down
1 change: 1 addition & 0 deletions torchbenchmark/models/hf_Whisper/metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ eval_deterministic: false
eval_nograd: true
not_implemented:
- device: cpu
- test: train
train_benchmark: false
train_deterministic: false
57 changes: 33 additions & 24 deletions torchbenchmark/util/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,36 +171,45 @@ def _determine_dynamic_num_batches(self, user_specified_num_batches: Optional[in
assert hasattr(self, 'DEFAULT_NUM_BATCH'), f"We expect all models with dynamic shapes specify field `DEFAULT_NUM_BATCHES`."
return self.DEFAULT_NUM_BATCH

def _determine_batch_size(self, batch_size=None):
def _get_batch_size_from_metadata(self) -> Optional[str]:
if self.device != "cuda":
current_device_name = str(self.device)
else:
current_device_name = torch.cuda.get_device_name()
assert current_device_name, f"torch.cuda.get_device_name() returns None when device is set to cuda, please double check."
if current_device_name in SPECIAL_DEVICE_MAPPING:
current_device_name = SPECIAL_DEVICE_MAPPING[current_device_name]

# use the device suggestion on CUDA inference tests, key should be either eval_batch_size or train_batch_size
device_batch_size_key = f"{self.test}_batch_size"
if self.metadata and "devices" in self.metadata and current_device_name in self.metadata["devices"] \
and device_batch_size_key in self.metadata["devices"][current_device_name]:
batch_size = self.metadata["devices"][current_device_name][device_batch_size_key]
return batch_size

def _determine_batch_size(self, user_specified_batch_size=None):
# batch size priority for eval tests: not ALLOW_CUSTOMIZE_BSIZE > user specified > device specified > default
# batch size priority for train tests: not ALLOW_CUSTOMIZE_BSIZE > user specified > default
self.batch_size = batch_size
if not batch_size:
self.batch_size = self.DEFAULT_TRAIN_BSIZE if self.test == "train" else self.DEFAULT_EVAL_BSIZE
if self.device == "cuda":
current_device_name = torch.cuda.get_device_name()
assert current_device_name, f"torch.cuda.get_device_name() returns None when device is set to cuda, please double check."
if current_device_name in SPECIAL_DEVICE_MAPPING:
current_device_name = SPECIAL_DEVICE_MAPPING[current_device_name]
else:
current_device_name = str(self.device)
# use the device suggestion on CUDA inference tests, key should be either eval_batch_size or train_batch_size
device_batch_size_key = f"{self.test}_batch_size"
if self.metadata and "devices" in self.metadata and current_device_name in self.metadata["devices"] \
and device_batch_size_key in self.metadata["devices"][current_device_name]:
self.batch_size = self.metadata["devices"][current_device_name][device_batch_size_key]
# If the model doesn't implement test or eval test
# its DEFAULT_TRAIN_BSIZE or DEFAULT_EVAL_BSIZE will still be None
if not self.batch_size:
raise NotImplementedError(f"Test {self.test} is not implemented.")
else:
self.batch_size = batch_size

self.batch_size = user_specified_batch_size

if not self.batch_size:
device_specified_batch_size = self._get_batch_size_from_metadata()
self.batch_size = device_specified_batch_size

if not self.batch_size:
default_batch_size = self.DEFAULT_TRAIN_BSIZE if self.test == "train" else self.DEFAULT_EVAL_BSIZE
self.batch_size = default_batch_size

if not self.batch_size:
raise NotImplementedError(f"Model's {'DEFAULT_TRAIN_BSIZE' if self.test == 'train' else 'DEFAULT_EVAL_BSIZE'} is not implemented.")

# Check if specified batch size is supported by the model
if hasattr(self, "ALLOW_CUSTOMIZE_BSIZE") and (not getattr(self, "ALLOW_CUSTOMIZE_BSIZE")):
if self.test == "train" and (not self.batch_size == self.DEFAULT_TRAIN_BSIZE):
raise NotImplementedError("Model doesn't support customizing batch size.")
raise NotImplementedError(f"Model doesn't support customizing batch size, but {self.test} test is providing a batch size other than DEFAULT_TRAIN_BSIZE")
elif self.test == "eval" and (not self.batch_size == self.DEFAULT_EVAL_BSIZE):
raise NotImplementedError("Model doesn't support customizing batch size.")
raise NotImplementedError(f"Model doesn't support customizing batch size, but {self.test} test is providing a batch size other than DEFAULT_EVAL_BSIZE")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why a model would have ALLOW_CUSTOMIZE_BSIZE but we would end up in this branch. For context, I'm looking into why we are not running stable_diffusion_unet in inference

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we just use the batch size of the model instead of failing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If ALLOW_CUSTOMIZE_BSIZE = False, the model will only accept the default batch size, not the batch size specified by the user.

We could silently use the default batch size instead of failing, but my concern this will cause misunderstanding on the user side (for example, they might think the model is running in batch size 100, but ALLOW_CUSTOMIZE_BSIZE = False and the default batch size is 1, so it will run silently with batch size 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

If batch_size is passed in as None, it seems okay to use the default specified on the model, instead of specified in self.metadata["devices"][current_device_name][device_batch_size_key]

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if we're worried about that case, we should also fix this upstream handling of it: https://github.com/pytorch/pytorch/blob/main/benchmarks/dynamo/torchbench.py#L369-L374

Copy link
Contributor

Choose a reason for hiding this comment

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

If batch_size is passed in as None, it seems okay to use the default specified on the model, instead of specified in self.metadata["devices"][current_device_name][device_batch_size_key]

Right, this is a bug. If ALLOW_CUSTOMIZE_BSIZE = False and batch_size passed in as None, we should use the default specified on the model instead of the device-specified batch size.

elif self.dargs.accuracy:
self.batch_size = 4 if self.batch_size > 4 else self.batch_size

Expand Down