-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Implement first draft of autograd benchmark. #40586
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
Conversation
💊 CI failures summary and remediationsAs of commit 6a685ff (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 fixed upstream failure:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
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.
We is a new MultiheadAttention container in torchtext. Are we going to benchmark it?
https://github.com/pytorch/text/blob/bcb9104680eb9dc978a6bbcc2b9ca46cf2bdbed9/torchtext/modules/multiheadattention.py#L5
|
@zhangguanheng66 We can definitely add it! |
Here is an example. You will have to add the dependency of torchtext. |
|
@zhangguanheng66 I added the multi head attention as a standalone thing (without downstream net or loss). |
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.
Not done reading yet. Some initial feedback I had was:
- it might be good to add a test that the benchmark script works to prevent bitrot
- Since we've dropped Python 2 support, some type annotations would enhance readability of the code
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.
Because PyTorch is Python 3+, I'd love to see some type annotations here (and in the other non-model Python files). Type annotations improve the readability of functions, especially with the helper functions in utils.py
e.g.,
def get_wav2letter(device: torch.device) -> Tuple[Callable, Tuple]
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
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.
okay I think I read through everything minus torchvision_models and torch_audiomodels
The benchmarking code looks correct; most of my comments are suggestions for readability and structuring
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
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, some minor comments
benchmarks/functional_autograd_benchmark/functional_autograd_benchmark.py
Outdated
Show resolved
Hide resolved
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.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
It is quite a lot of code because I pulled some code from torchaudio and torchvision to remove issues I had to get latest version with pytorch built from source while I can't build there libs from source (dependency missing for torchaudio).
The compare script generates table as follows: