-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Speed up Model tests by 20% #5574
Conversation
💊 CI failures summary and remediationsAs of commit 01be6a5 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
9214973
to
17593a5
Compare
db77373
to
1221e12
Compare
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.
Thanks @datumbox , I took a brief look only. Minor question but otherwise LGTM
eager_out = nn_module(*args) | ||
if eager_out is None: | ||
with torch.no_grad(), freeze_rng_state(): | ||
if unwrapper: |
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.
Is this line needed? It looks like it eager_out
wasn't unwrapped before
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.
It's not mandatory to have it for most of the existing models but this is only due to implementation details. Since this is a general purpose tool for checking JIT-scriptability, I opted for consistently unwrapping the output in all place.
FYI the reason many models don't have to get unwrapped is due to idioms like this:
vision/torchvision/models/googlenet.py
Lines 163 to 179 in 4ae20e5
@torch.jit.unused | |
def eager_outputs(self, x: Tensor, aux2: Tensor, aux1: Optional[Tensor]) -> GoogLeNetOutputs: | |
if self.training and self.aux_logits: | |
return _GoogLeNetOutputs(x, aux2, aux1) | |
else: | |
return x # type: ignore[return-value] | |
def forward(self, x: Tensor) -> GoogLeNetOutputs: | |
x = self._transform_input(x) | |
x, aux1, aux2 = self._forward(x) | |
aux_defined = self.training and self.aux_logits | |
if torch.jit.is_scripting(): | |
if not aux_defined: | |
warnings.warn("Scripted GoogleNet always returns GoogleNetOutputs Tuple") | |
return GoogLeNetOutputs(x, aux2, aux1) | |
else: | |
return self.eager_outputs(x, aux2, aux1) |
New non-detection models don't use this idiom any more (returning different output depending on jit/training flag), so I think it's safer to handle it explicitly.
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! Thanks for this change - great improvement!
Hey @datumbox! You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py |
Summary: * Measuring execution times of models. * Speed up models by avoiding re-estimation of eager output * Fixing linter * Reduce input size for big models * Speed up jit check method. * Add simple jitscript fallback check for flaky models. * Restore pytest filtering * Fixing linter Reviewed By: vmoens Differential Revision: D34878998 fbshipit-source-id: 37bfa05aac0d28d59d3320119147446006bff75c
We focus on the
test_classification_model
,test_detection_model
,test_quantized_classification_model
,test_segmentation_model
,test_video_model
tests and improve their execution times by 20%.This is achieved by:
Before: 629.21 sec
Run: https://app.circleci.com/pipelines/github/pytorch/vision/15497/workflows/51d7e30b-86ff-4c71-af68-7cb3438f25c9/jobs/1252313
After: 507.28 sec
Run: https://app.circleci.com/pipelines/github/pytorch/vision/15520/workflows/2cc25bdc-fe10-4705-b942-c206a9d12fad/jobs/1254266