Skip to content

Add warning for legacy autograd function #22922

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.

Sign up for GitHub

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

Jump to bottom
Closed
yf225 wants to merge 10 commits into from
Closed

Add warning for legacy autograd function #22922

yf225 wants to merge 10 commits into from

Conversation

@yf225
Copy link
Contributor

@yf225 yf225 commented Jul 16, 2019 •
edited
Loading

When working on #22762, we discovered that we haven't actually deprecated legacy autograd function. This PR puts up the deprecation warning for 1.2, with the goal to remove legacy function support completely in the near future.

@pytorchbot pytorchbot added the module: autograd Related to torch.autograd, and the autograd engine in general label Jul 16, 2019
@yf225 yf225 requested a review from gchanan July 16, 2019 20:05
This was referenced Jul 16, 2019
Closed
Closed
zou3519
zou3519 reviewed Jul 16, 2019
View reviewed changes
zou3519
zou3519 reviewed Jul 16, 2019
View reviewed changes
torch/autograd/function.py Outdated

# old-style functions
if not has_static_forward:
warnings.warn('Legacy autograd function with non-static forward method is deprecated. '
Copy link
Contributor

@zou3519 zou3519 Jul 16, 2019

Choose a reason for hiding this comment

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

I'm wondering if test_autograd.py will warning spew. At some point we should convert the legacy functions there that are not explicitly testing legacy behavior to non-legacy ones (probably doesn't have to happen in this PR)

Copy link
Contributor Author

@yf225 yf225 Jul 16, 2019

Choose a reason for hiding this comment

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

That's a good idea, I opened another PR for it: #22925.

@yf225 yf225 mentioned this pull request Jul 16, 2019
Closed
@zou3519
Copy link
Contributor

zou3519 commented Jul 17, 2019

Test failure seems like it might be related?

Jul 16 21:05:30 ======================================================================
Jul 16 21:05:30 ERROR: test_legacy_function_deprecation_warning (__main__.TestAutograd)
Jul 16 21:05:30 ----------------------------------------------------------------------
Jul 16 21:05:30 Traceback (most recent call last):
Jul 16 21:05:30   File "test_autograd.py", line 180, in test_legacy_function_deprecation_warning
Jul 16 21:05:30     str(w[0].message))
Jul 16 21:05:30 IndexError: list index out of range
Jul 16 21:05:30

@yf225
Copy link
Contributor Author

yf225 commented Jul 17, 2019

@zou3519 I couldn't make the test work in Python 2.7, but I tested it manually and can confirm that it prints warning in Python 2.7.

@gchanan
Copy link
Contributor

gchanan commented Jul 17, 2019

we have tests for warnings in python2 and this is a pretty major deprecation; we should have tests for it.

@pytorchbot pytorchbot added the module: pybind Related to our Python bindings / interactions with other Python libraries label Jul 18, 2019
@yf225 yf225 force-pushed the legacy_function_warning branch from f8a9b8a to f56e01d Compare July 18, 2019 00:45
@yf225
Copy link
Contributor Author

yf225 commented Jul 18, 2019

I moved the warning to the legacy function's forward method so that it can be reliably shown in both Python 2.7 and Python 3.

pull bot pushed a commit to Pandinosaurus/pytorch that referenced this pull request Jul 18, 2019
@facebook-github-bot
Remove usage of legacy autograd function (pytorch#22925)
52bf380 
Summary:
We are planning to put up a deprecation warning for legacy autograd function in 1.2: pytorch#22922. This PR removes all usage of legacy function in PyTorch core and test suite, to prepare for the eventual removal of legacy function.
Pull Request resolved: pytorch#22925

Differential Revision: D16344834

Pulled By: yf225

fbshipit-source-id: 8bf4cca740398835a08b7a290f3058c3e46781ba
zou3519
zou3519 reviewed Jul 18, 2019
View reviewed changes
test/test_autograd.py
def test_legacy_function_deprecation_warning(self):
with warnings.catch_warnings(record=True) as w:
# Ensure warnings are being shown
warnings.simplefilter("always")
Copy link
Contributor

@zou3519 zou3519 Jul 18, 2019

Choose a reason for hiding this comment

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

Is this necessary for this warning to be shown? I'm not sure what the default warnings behavior is

Copy link
Contributor

@zou3519 zou3519 Jul 18, 2019

Choose a reason for hiding this comment

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

Oh, I see why you had to do this:

print the first occurrence of matching warnings for each location (module + line number) where the warning is issued

zou3519
zou3519 reviewed Jul 18, 2019
View reviewed changes
torch/csrc/autograd/python_function.cpp Outdated
std::vector<c10::IValue>(),
autograd::Function::peek_at_next_sequence_nr());

TORCH_WARN("Legacy autograd function with non-static forward method is deprecated. ",
Copy link
Contributor

@zou3519 zou3519 Jul 18, 2019

Choose a reason for hiding this comment

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

Maybe give a deadline if you want to remove it asap? "Will be removed in 1.3".

zou3519
zou3519 approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

lgtm, I can't think of a use case where someone constructs an autograd function without calling it so putting a deprecation warning in forward should be fine.

facebook-github-bot
facebook-github-bot reviewed Jul 18, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 18, 2019

@yf225 merged this pull request in c1c4014.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zou3519 zou3519 zou3519 approved these changes

@gchanan gchanan Awaiting requested review from gchanan

Assignees

No one assigned

Labels

Merged module: autograd Related to torch.autograd, and the autograd engine in general module: pybind Related to our Python bindings / interactions with other Python libraries

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@yf225 @zou3519 @gchanan @facebook-github-bot @pytorchbot @mruberry