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

Create a mechanism to trigger tests that only warn once #47624

Closed
mruberry opened this issue Nov 9, 2020 · 7 comments
Closed

Create a mechanism to trigger tests that only warn once #47624

mruberry opened this issue Nov 9, 2020 · 7 comments
Assignees
Labels
function request A request for a new function or the addition of new arguments/modes to an existing function. high priority module: tests Issues related to tests (not the torch.testing module) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@mruberry
Copy link
Collaborator

mruberry commented Nov 9, 2020

Today in PyTorch we have TORCH_WARN and TORCH_WARN_ONCE. TORCH_WARN_ONCE, as its name suggests, only throws a warning one time. This makes it tricky to test for. Our test suite, for example, has maybeWarnsRegex, but this just verifies that no other warnings are thrown.

This feature request is for a mechanism to consistently trigger warnings that use TORCH_WARN_ONCE. One mechanism for this would be to add a new context that, if set, causes these warnings to always warn. This would let us reliably test that these warnings are thrown correctly.

cc @ezyang @gchanan @zou3519 @bdhirsh @jbschlosser @mruberry @VitalyFedyunin @walterddr

@mruberry mruberry added function request A request for a new function or the addition of new arguments/modes to an existing function. module: testing Issues related to the torch.testing module (not tests) labels Nov 9, 2020
@malfet malfet added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 10, 2020
@mattip
Copy link
Collaborator

mattip commented Nov 17, 2020

If I understand the code correctly, it seems TORCH_WARN_ONCE and TORCH_WARN do not currently always use python. In a call like setDeterministic, the default warning_handler is used, which goes to LOG_AT_FILE_LINE which uses MessgeLogger to print to std::err. Most python wrappers use the HANDLE_TH_ERRORS macro to set a warning_handler by calling PyWarningHandler. For some reason the wrapper for setDeterministic does not use HANDLE_TH_ERRORS.

But that is only one part of the story. Overriding the behaviour of TORCH_WARN_ONCE to make it warn on each call is more complicated. As defined in c10/util/Exception.h, it instantiates a static class instance, making it a singleton, and uses that instance to call c10::Warning::warn only once ever. Refactoring that to call each time would make this equivalent to TORCH_WARN.

@mruberry
Copy link
Collaborator Author

I was hoping we could do something like set a global context, "setAlwaysWarn(true/false)", that TORCH_WARN_ONCE would query and, if the global flag was true, it would then warn like TORCH_WARN does.

@mattip mattip self-assigned this Nov 19, 2020
@mattip
Copy link
Collaborator

mattip commented Nov 19, 2020

Is this the direction you were thinking?

#define TORCH_WARN_MAYBE(...) \
  if (g_WARN_ONLY_ONCE) { \
    TORCH_WARN_ONCE(...) \
  } else { \
    TORCH_WARN(...) \
  }

Then

  • Some/All of the uses of TORCH_WARN_ONCE would be refactored to TORCH_WARN_MAYBE.
  • The new boolean global value g_WARN_ONLY_ONCE would be set false by the python warning handler, passing responsibility for filtering warnings to python.
  • Tests would need a warning filter, and could check that the warning is emitted.

Is there a warning context manager in the testing framework, or should we use the numpy.testing.suppress_warnings context manager to work around python issues with warning filters

@mruberry
Copy link
Collaborator Author

mruberry commented Nov 20, 2020

That's close to what I was thinking of.

I was thinking we'd keep TORCH_WARN_ONCE, and there would be a flag, like g_WARN_ONLY_ONCE, that had the behavior you're describing. It's true that this would make TORCH_WARN_ONCE a bit of a misnomer, but that's OK.

I was thinking the flag would be controlled by a PyTorch global context (like the deterministic flag).

We currently use a the Python warning filter in our test suite and that seems to be OK, I don't think we'd need to use numpy.testing.suppress_warnings.

Basically we want this flag to be just for debugging. We'll set it to true during the test suite so we can verify that these warnings are thrown.

Does that sound good?

@mruberry mruberry added module: tests Issues related to tests (not the torch.testing module) and removed module: testing Issues related to the torch.testing module (not tests) labels Jan 3, 2021
@mattip
Copy link
Collaborator

mattip commented Feb 3, 2021

Now that the assertWarnsOnceRegex is inplace, the next step is to replace maybeWarnsRegex with it.

facebook-github-bot pushed a commit that referenced this issue Feb 8, 2021
Summary:
Toward fixing #47624

~Step 1: add `TORCH_WARN_MAYBE` which can either warn once or every time in c++, and add a c++ function to toggle the value.
Step 2 will be to expose this to python for tests. Should I continue in this PR or should we take a different approach: add the python level exposure without changing any c++ code and then over a series of PRs change each call site to use the new macro and change the tests to make sure it is being checked?~

Step 1: add a python and c++ toggle to convert TORCH_WARN_ONCE into TORCH_WARN so the warnings can be caught in tests
Step 2: add a python-level decorator to use this toggle in tests
Step 3: (in future PRs): use the decorator to catch the warnings instead of `maybeWarnsRegex`

Pull Request resolved: #48560

Reviewed By: ngimel

Differential Revision: D26171175

Pulled By: mruberry

fbshipit-source-id: d83c18f131d282474a24c50f70a6eee82687158f
@mruberry
Copy link
Collaborator Author

I think this issue is closed. @mattip, would you file a separate issue to audit uses of maybeWarnsRegex in the test suite?

@mattip
Copy link
Collaborator

mattip commented Feb 10, 2021

See issue #52025

xsacha pushed a commit to xsacha/pytorch that referenced this issue Mar 31, 2021
Summary:
Toward fixing pytorch#47624

~Step 1: add `TORCH_WARN_MAYBE` which can either warn once or every time in c++, and add a c++ function to toggle the value.
Step 2 will be to expose this to python for tests. Should I continue in this PR or should we take a different approach: add the python level exposure without changing any c++ code and then over a series of PRs change each call site to use the new macro and change the tests to make sure it is being checked?~

Step 1: add a python and c++ toggle to convert TORCH_WARN_ONCE into TORCH_WARN so the warnings can be caught in tests
Step 2: add a python-level decorator to use this toggle in tests
Step 3: (in future PRs): use the decorator to catch the warnings instead of `maybeWarnsRegex`

Pull Request resolved: pytorch#48560

Reviewed By: ngimel

Differential Revision: D26171175

Pulled By: mruberry

fbshipit-source-id: d83c18f131d282474a24c50f70a6eee82687158f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
function request A request for a new function or the addition of new arguments/modes to an existing function. high priority module: tests Issues related to tests (not the torch.testing module) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

4 participants