Skip to content

Conversation

@sublee
Copy link
Contributor

@sublee sublee commented May 28, 2019

I've reported inconsistency between checkpoint_sequential and nn.Sequential at #19260. Both should provide the same input signature but they don't. I think the consistency is important and I agree with @apaszke that nn.Sequential's semantics should be kept instead of checkpoint_sequential.

I hope checkpoint_sequential raises TypeError on variadic arguments since PyTorch 1.2.0. But for now, it's okay just to warn as DeprecationWarning. I've talked about this approach with @soumith.

Please review this pull request. Any comment will be my pleasure.

@pytorchbot pytorchbot added module: activation checkpointing Related to activation checkpointing module: tests Issues related to tests (not the torch.testing module) labels May 28, 2019
@sublee
Copy link
Contributor Author

sublee commented May 28, 2019

To test DeprecationWarning, I've used assertWarns in common_utils.py. But the test was failed (https://circleci.com/gh/pytorch/pytorch/1813423). The reason is that warnings.catch_warnings() misses already registered warnings in Python 2.7.

I fixed this problem by modifying assertWarns and assertWarnsRegex. I adopted the approach of reset_warning_registry.py reported in https://bugs.python.org/issue21724.

# This interface will be changed at PyTorch 1.2.0.
# See also: https://github.com/pytorch/pytorch/issues/19260
if not inputs:
warnings.warn('no input to checkpoint_sequential has been deprecated, '
Copy link
Contributor

Choose a reason for hiding this comment

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

"no input" -> "giving no inputs"
"will be raised since PyTorch 1.2.0" -> "will be raised since PyTorch 1.3.0" (we need atleast 1 stable version to deprecate things, right now it is 1.1.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great feedback and fix. I also capitalized the warning message for multiple inputs just like "Giving no inputs". Thank you so much!

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.

@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@soumith merged this pull request in ffdce79.

facebook-github-bot pushed a commit that referenced this pull request Dec 5, 2019
Summary:
To support variadic inputs of `checkpoint_sequential` was deprecated at #21006. This case should be warned with `DeprecationWarning` for PyTorch 1.2, but it should be simply failed with `TypeError` since PyTorch 1.3. This patch removes the `DeprecationWarning` for PyTorch 1.2.
Pull Request resolved: #25985

Differential Revision: D18809875

Pulled By: albanD

fbshipit-source-id: e84dd8629c04979c4b2dc63e8ada94292e8cedd0
wuhuikx pushed a commit to wuhuikx/pytorch that referenced this pull request Jan 30, 2020
Summary:
To support variadic inputs of `checkpoint_sequential` was deprecated at pytorch#21006. This case should be warned with `DeprecationWarning` for PyTorch 1.2, but it should be simply failed with `TypeError` since PyTorch 1.3. This patch removes the `DeprecationWarning` for PyTorch 1.2.
Pull Request resolved: pytorch#25985

Differential Revision: D18809875

Pulled By: albanD

fbshipit-source-id: e84dd8629c04979c4b2dc63e8ada94292e8cedd0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: activation checkpointing Related to activation checkpointing module: tests Issues related to tests (not the torch.testing module) open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants