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

Remove save_state_warning in LambdaLR #46813

Closed

Conversation

jsrozner
Copy link
Contributor

@jsrozner jsrozner commented Oct 24, 2020

Fixes #46405, #43352

I updated the docstring in the local file (function level comments). Do I also need to edit somewhere else or recompile docstrings?

Also, though I didn't change any types here, how is typing (for IDE type checking) documentation generated / used)?

@dr-ci
Copy link

dr-ci bot commented Oct 24, 2020

💊 CI failures summary and remediations

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


  • 3/3 failures introduced in this PR

🕵️ 3 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc5_4_build (1/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.
CONFLICT (add/add): Merge conflict in .circleci/docker/centos-rocm/Dockerfile 
Auto-merging .circleci/docker/centos-rocm/Dockerfile 
CONFLICT (add/add): Merge conflict in .circleci/config.yml 
Auto-merging .circleci/config.yml 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py 
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_data.py 
Auto-merging .circleci/cimodel/data/pytorch_build_data.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/dimensions.py 
Auto-merging .circleci/cimodel/data/dimensions.py 
Automatic merge failed; fix conflicts and then commit the result. 

See CircleCI build pytorch_xla_linux_bionic_py3_6_clang9_build (2/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.
CONFLICT (add/add): Merge conflict in .circleci/docker/centos-rocm/Dockerfile 
Auto-merging .circleci/docker/centos-rocm/Dockerfile 
CONFLICT (add/add): Merge conflict in .circleci/config.yml 
Auto-merging .circleci/config.yml 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py 
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_data.py 
Auto-merging .circleci/cimodel/data/pytorch_build_data.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/dimensions.py 
Auto-merging .circleci/cimodel/data/dimensions.py 
Automatic merge failed; fix conflicts and then commit the result. 

See CircleCI build pytorch_linux_xenial_py3_6_gcc5_4_build (3/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.
CONFLICT (add/add): Merge conflict in .circleci/docker/centos-rocm/Dockerfile 
Auto-merging .circleci/docker/centos-rocm/Dockerfile 
CONFLICT (add/add): Merge conflict in .circleci/config.yml 
Auto-merging .circleci/config.yml 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py 
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_data.py 
Auto-merging .circleci/cimodel/data/pytorch_build_data.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/dimensions.py 
Auto-merging .circleci/cimodel/data/dimensions.py 
Automatic merge failed; fix conflicts and then commit the result. 

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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 4 times.

@izdeby izdeby requested a review from vincentqb October 26, 2020 17:13
@izdeby izdeby added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 26, 2020
@facebook-github-bot
Copy link
Contributor

Hi @jsrozner!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@vincentqb
Copy link
Contributor

I updated the docstring in the local file (function level comments). Do I also need to edit somewhere else or recompile docstrings?

As mentioned in comment, the docstring will automatically regenerate the doc on master. :)

Also, though I didn't change any types here, how is typing (for IDE type checking) documentation generated / used)?

Since this is just updating the warning and the documentation, there is no need update the typing.

@jsrozner
Copy link
Contributor Author

Also, though I didn't change any types here, how is typing (for IDE type checking) documentation generated / used)?

Since this is just updating the warning and the documentation, there is no need update the typing.

Yes, agreed. But I was curious how the pytorch library handles typing since it is not inlined in, e.g., these function definitions

Is anything else needed by me to finish this pull request?

@vincentqb
Copy link
Contributor

Also, though I didn't change any types here, how is typing (for IDE type checking) documentation generated / used)?

Since this is just updating the warning and the documentation, there is no need update the typing.

Yes, agreed. But I was curious how the pytorch library handles typing since it is not inlined in, e.g., these function definitions

We are migrating to inline, but the types are in *.pyi files adjacent :)

Is anything else needed by me to finish this pull request?

Thanks for signing and writing the pull request, this is good to go :)

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

LGTM :)

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.

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

@jsrozner
Copy link
Contributor Author

jsrozner commented Dec 1, 2020

@vincentqb
Can we merge?

@facebook-github-bot
Copy link
Contributor

@vincentqb merged this pull request in 42e6951.

@stas00
Copy link
Contributor

stas00 commented Dec 8, 2020

That broke transformers where it's used:
https://github.com/huggingface/transformers/blob/aa0969c2f3cf5cf7ce5690e7b77f95b1622988d6/src/transformers/trainer_pt_utils.py#L37

But reading why it was done (goodness), transformers will just need to recode for this change.

Not sure how one could deprecate a constant...

stas00 added a commit to huggingface/transformers that referenced this pull request Dec 8, 2020
FYI `SAVE_STATE_WARNING` has been removed 3 days ago: pytorch/pytorch#46813

Fixes: #8232

@sgugger
stas00 added a commit to huggingface/transformers that referenced this pull request Dec 8, 2020
* [training] SAVE_STATE_WARNING was removed in pytorch

FYI `SAVE_STATE_WARNING` has been removed 3 days ago: pytorch/pytorch#46813

Fixes: #8232

@sgugger

* style, but add () to prevent autoformatters from botching it

* switch to try/except

* cleanup
binshengliu pushed a commit to binshengliu/transformers that referenced this pull request Mar 2, 2021
* [training] SAVE_STATE_WARNING was removed in pytorch

FYI `SAVE_STATE_WARNING` has been removed 3 days ago: pytorch/pytorch#46813

Fixes: huggingface#8232

@sgugger

* style, but add () to prevent autoformatters from botching it

* switch to try/except

* cleanup
binshengliu pushed a commit to binshengliu/transformers that referenced this pull request Mar 2, 2021
* [training] SAVE_STATE_WARNING was removed in pytorch

FYI `SAVE_STATE_WARNING` has been removed 3 days ago: pytorch/pytorch#46813

Fixes: huggingface#8232

@sgugger

* style, but add () to prevent autoformatters from botching it

* switch to try/except

* cleanup
binshengliu pushed a commit to binshengliu/transformers that referenced this pull request Mar 5, 2021
* [training] SAVE_STATE_WARNING was removed in pytorch

FYI `SAVE_STATE_WARNING` has been removed 3 days ago: pytorch/pytorch#46813

Fixes: huggingface#8232

@sgugger

* style, but add () to prevent autoformatters from botching it

* switch to try/except

* cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged 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.

Allow downstream users to suppress Save Optimizer warnings
6 participants