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.

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

add no_grad for inference runs #181

Merged
merged 1 commit into from
Feb 5, 2021
Merged

add no_grad for inference runs #181

merged 1 commit into from
Feb 5, 2021

Conversation

Krovatkin
Copy link
Contributor

add no_grad for inference runs

Copy link
Contributor

@wconstab wconstab left a comment

Choose a reason for hiding this comment

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

I think this is a good idea, but is it potentially limiting and presumptive to put it here for all benchmarks? I can imagine one or two benchmarks might use grad in their forward, although I'm not aware of having one that does that now.

Also, i'd be wary of benchmarking the system in a way that is not how users use it. Do users generally set no_grad when doing inference?

Still I'm in favor of changing this either here or possibly inside the benchmark code themselves.

@asuhan
Copy link

asuhan commented Dec 29, 2020

@wconstab Why would our benchmarks use gradient in forward, are you thinking that might be the case because of the test failures we're seeing?

Would be interesting if we could run benchmarks both with and without no_grad set, it'd give us a sense of some pure overheads (since we wouldn't actually compute the gradients in either case) through comparison.

@wconstab
Copy link
Contributor

@asuhan I don't think any of our current benchmarks do, but I think there are some types of less common models that compute a grad as part of their inference step. I need to look for an example.

@Krovatkin Krovatkin merged commit f4ee431 into master Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants