Skip to content

Allow GradScaler to be pickled#38296

Closed
mcarilli wants to merge 7 commits intopytorch:masterfrom
mcarilli:pickle_gradscaler
Closed

Allow GradScaler to be pickled#38296
mcarilli wants to merge 7 commits intopytorch:masterfrom
mcarilli:pickle_gradscaler

Conversation

@mcarilli
Copy link
Copy Markdown
Collaborator

@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 12, 2020

💊 CI failures summary and remediations

As of commit c870c83 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 14 times.

@williamFalcon
Copy link
Copy Markdown
Contributor

thank you!

Generally anything that can't be pickled is a problem for us as we automate the ddp init which uses spawn

@vincentqb
Copy link
Copy Markdown
Contributor

@albanD -- would you be willing to review this PR?

@vincentqb vincentqb added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 12, 2020
@albanD
Copy link
Copy Markdown
Collaborator

albanD commented May 12, 2020

Sure I'll take a look tomorrow morning

Copy link
Copy Markdown
Collaborator

@ngimel ngimel left a comment

Choose a reason for hiding this comment

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

Move the test please, otherwise lgtm.

Comment thread test/test_cuda.py Outdated
Comment thread test/test_torch.py Outdated
b = pickle.loads(serialized)
self.assertEqual(a, b)

def test_pickle_gradscaler(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you add it to some of the device-generic tests rather than _TorchMixin? We are trying to get rid of the latter.

Copy link
Copy Markdown
Collaborator Author

@mcarilli mcarilli May 13, 2020

Choose a reason for hiding this comment

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

Moved to TestTorchDeviceType and made it more devoutly device agnostic (see comments in code)

Copy link
Copy Markdown
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Looks good, same comment on tests as natalia.

Comment thread torch/cuda/amp/grad_scaler.py Outdated
# - Lambdas can't be pickled, so we don't want to supply a lambda as the factory.
# - Defining READY, UNSCALED, STEPPED and _refresh_per_optimizer_state within GradScaler
# causes a circular reference, which we'd rather avoid.
READY = 0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to make this an enum now that we don't support python 2 anymore? You removed the comment mentioning this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sure, done in c870c83.

Copy link
Copy Markdown
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.

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

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@albanD merged this pull request in 25f9185.

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
Summary:
Should unblock Lightning-AI/pytorch-lightning#1782.
Pull Request resolved: pytorch#38296

Differential Revision: D21553296

Pulled By: albanD

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

Labels

Merged module: amp (automated mixed precision) autocast open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants