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

Allow frozen data classes in optimizer state dict #16656

Merged
merged 8 commits into from Mar 10, 2023

Conversation

janEbert
Copy link
Contributor

@janEbert janEbert commented Feb 6, 2023

What does this PR do?

We use the (now merged) PR in lightning-utilities that allows frozen dataclasses to be ignored in apply_to_collection. With this, we can now allow saving optimizers for which the state includes frozen dataclasses that we just want to pass through.
Any optimizer that includes a frozen dataclass in its state, is not supported in Lightning without this, instead causing errors.

This should be backward-compatible, since previously, frozen dataclasses in optimizers simply caused an error. Other than that, nothing changed.

Before submitting
  • Was this discussed/agreed via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
    I tried, tests were broken for me even without the changes. Manually running tests/tests_fabric/utilities/test_optimizer.py does not complain.
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)
    Did not seem necessary.

The frozen data class should be explicitly allowed to exist in the
optimizer's `state`.
@github-actions github-actions bot added the fabric lightning.fabric.Fabric label Feb 6, 2023
@janEbert janEbert marked this pull request as draft February 6, 2023 18:36
@janEbert
Copy link
Contributor Author

janEbert commented Feb 6, 2023

Converted to a draft for safety regarding the other merge happening first. This PR itself is ready, though.

@janEbert janEbert marked this pull request as ready for review February 7, 2023 07:57
@janEbert
Copy link
Contributor Author

janEbert commented Feb 7, 2023

Other merge happened, this is good to go from my side.

@awaelchli awaelchli added this to the 2.0 milestone Feb 7, 2023
@awaelchli awaelchli added feature Is an improvement or enhancement community This PR is from the community labels Feb 7, 2023
@awaelchli awaelchli removed this from the 2.0 milestone Feb 7, 2023
@janEbert
Copy link
Contributor Author

janEbert commented Feb 7, 2023

Tests fail because it expects the lightning-utiliities master, I guess. Would it make sense to tag a new version in there? Seems a bit overkill for the small change, but it would be a solution.

@awaelchli awaelchli added this to the 2.0 milestone Feb 9, 2023
@awaelchli awaelchli self-assigned this Mar 10, 2023
@mergify mergify bot added the ready PRs ready to be merged label Mar 10, 2023
@github-actions github-actions bot added the app Generic label for Lightning App package label Mar 10, 2023
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Mar 10, 2023
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #16656 (25eae2a) into master (7c80fe6) will decrease coverage by 0%.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #16656    +/-   ##
========================================
- Coverage      82%      82%    -0%     
========================================
  Files         438      418    -20     
  Lines       31744    31534   -210     
========================================
- Hits        25991    25786   -205     
+ Misses       5753     5748     -5     

@awaelchli awaelchli enabled auto-merge (squash) March 10, 2023 14:22
@awaelchli awaelchli merged commit dd02397 into Lightning-AI:master Mar 10, 2023
145 of 146 checks passed
@janEbert
Copy link
Contributor Author

Thanks for your work on fixing everything up!

@awaelchli
Copy link
Member

Thanks, and this was a great addition. 2.0 here we come!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Generic label for Lightning App package community This PR is from the community fabric lightning.fabric.Fabric feature Is an improvement or enhancement pl Generic label for PyTorch Lightning package ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants