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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global gradcheck setting #49409

Open
albanD opened this issue Dec 15, 2020 · 8 comments
Open

Add global gradcheck setting #49409

albanD opened this issue Dec 15, 2020 · 8 comments
Assignees
Labels
better-engineering Relatively self-contained tasks for better engineering contributors module: autograd Related to torch.autograd, and the autograd engine in general triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@albanD
Copy link
Collaborator

albanD commented Dec 15, 2020

馃殌 Feature

There as two main use cases for gradcheck:

  • User facing API to help them check their custom autograd
  • Our own testing that use it to check that all our submodules work well with autograd

The problem that we encounter today while adding new features such as vmap and forward AD is that while we want the user facing API to remain stable to avoid breaking user code, we would like the internal testing to test these by default to make sure that functions we provide work with all these gradient-computing features.

Motivation

We want to avoid having to modify each call to gradcheck in our test suite when we add a new feature.
We also want to make sure new test writer get this by default and it is hard for them to forget to test these.

Pitch

Provide a global config for gradcheck that allows to change the default to the gradcheck and gradgradcheck functions.
It will consist in:

  • Add a config to gradcheck submodule with all the relevant settings.
  • Add API to set these settings easily for test. Also a decorator-based setting to disable them (to easily disable some settings for a single test).
  • Changing the API of gradcheck and gradgradcheck to take None as default that will translate to whatever is currently in the global config if no value is passed.
  • Update our default Test class to set the right setting on setUp and restore the current state in onExit.

Alternatives

Alternative would be to change all the calls we have in our test suite, but that wouldn't cover new tests being written.
Changing the default of all gradcheck is possible as well but would lead to BC-breaking behavior for users which is not great.

cc @ezyang @albanD @zou3519 @gqchen @pearu @nikitaved

@albanD albanD added better-engineering Relatively self-contained tasks for better engineering contributors module: autograd Related to torch.autograd, and the autograd engine in general triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Dec 15, 2020
@gchanan
Copy link
Contributor

gchanan commented Dec 15, 2020

How much is gradcheck actually used externally? I'm not sure I buy that breaking BC here regularly isn't the right choice (if it's not used much, if it adds additional checks with clear instructions on opting out).

@albanD
Copy link
Collaborator Author

albanD commented Dec 15, 2020

@gchanan a quick github search seems to indicate quite a lot: https://github.com/search?l=Python&q=torch.autograd.gradcheck&type=Code
There are few false positive in there but from checking some pages randomly almost all of them are actually using our version of gradcheck.

@albanD
Copy link
Collaborator Author

albanD commented Dec 16, 2020

From discussing with @gchanan offline, he thinks that having a custom function that should be used inside out tests is actually a feasible thing and that would be cleaner than a global config.
In particular, he argues that most new tests are written from copy/paste of other code and that we could also prevent use of autograd.gradcheck using a lint rule.
@zou3519 what do you think?

@zou3519
Copy link
Contributor

zou3519 commented Dec 16, 2020

A custom function and lint rule sounds reasonable to me, it should be less complex than maintaining a global config and the lint rule (if that is possible) will steer new test writers away from using autograd.gradcheck

@mruberry
Copy link
Collaborator

I agree that a new function sounds best. We could even put it under testing/_internal if we think the API will change frequently, or torch.testing if we think the API will be stable.

@zou3519
Copy link
Contributor

zou3519 commented Jan 19, 2021

We could even put it under testing/_internal if we think the API will change frequently, or torch.testing if we think the API will be stable.

The API will probably change frequently (e.g. we will add various different testing options), so testing/_internal sounds like a better place to be.

A simplified version of this would be helpful for cleaning up batched grad testing so I think I will toss something into testing/_internal, minus the lint rule, for now.

@zou3519
Copy link
Contributor

zou3519 commented Jan 29, 2021

@soulitzer should we keep this issue open until we have a lint rule (or should that be a separate issue?)

@soulitzer soulitzer reopened this Jan 30, 2021
@soulitzer
Copy link
Contributor

It probably makes sense for lint rule should be part of this same issue - reopening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
better-engineering Relatively self-contained tasks for better engineering contributors module: autograd Related to torch.autograd, and the autograd engine in general triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants