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

Enable PyTorch 1.7 in conda CI #3541

Merged
merged 9 commits into from
Sep 25, 2020
Merged

Enable PyTorch 1.7 in conda CI #3541

merged 9 commits into from
Sep 25, 2020

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Sep 18, 2020

What does this PR do?

Fixes #3531

Python property support for PyTorch JIT PR (v1.7): pytorch/pytorch#42390

Thanks to ptrblck

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • 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? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@ydcjeff ydcjeff marked this pull request as draft September 18, 2020 03:27
@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #3541 into master will increase coverage by 1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #3541   +/-   ##
======================================
+ Coverage      90%     91%   +1%     
======================================
  Files         110     110           
  Lines        8129    8069   -60     
======================================
- Hits         7331    7323    -8     
+ Misses        798     746   -52     

@Borda Borda added the feature Is an improvement or enhancement label Sep 18, 2020
@Borda
Copy link
Member

Borda commented Sep 18, 2020

lets first finis the tests fixing here and later follow up in #3543

@Borda
Copy link
Member

Borda commented Sep 18, 2020

it seems that most of the TorchScript are failing

FAILED tests/models/test_cpu.py::test_lbfgs_cpu_model - AssertionError: This ...
FAILED tests/models/test_torchscript.py::test_torchscript_input_output[EvalModelTemplate]
FAILED tests/models/test_torchscript.py::test_torchscript_input_output[ParityModuleRNN]
FAILED tests/models/test_torchscript.py::test_torchscript_input_output[BasicGAN]
FAILED tests/models/test_torchscript.py::test_torchscript_retain_training_state
FAILED tests/models/test_torchscript.py::test_torchscript_properties[EvalModelTemplate]
FAILED tests/models/test_torchscript.py::test_torchscript_properties[ParityModuleRNN]
FAILED tests/models/test_torchscript.py::test_torchscript_properties[BasicGAN]
FAILED tests/models/test_torchscript.py::test_torchscript_save_load[EvalModelTemplate]
FAILED tests/models/test_torchscript.py::test_torchscript_save_load[ParityModuleRNN]
FAILED tests/models/test_torchscript.py::test_torchscript_save_load[BasicGAN]

and it seems there is issue with DataModule:

>       concrete_type._create_methods_and_properties(property_defs, property_rcbs, method_defs, method_rcbs, method_defaults)
E       RuntimeError: Can't redefine method: __datamodule_getter on class: __torch__.tests.base.models.ParityModuleRNN

@nateraw mind have look, pls

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 20, 2020

Seems like there is something update to the JIT.

But not sure the cause is from Lightning or PyTorch.

@Borda
Copy link
Member

Borda commented Sep 20, 2020

Maybe move this testing to some alternative ci/badge? @williamFalcon

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 24, 2020

to_torchscript now works with PyTorch 1.7 nightly. This is done via __ignored_properties__, from https://github.com/pytorch/pytorch/pull/42390/files#diff-ff4f8670281cd1eb4e09329cc1dcb43b

So far I have ignored all the properties in LightningModule (some are from DeviceDtypeMixin) that would cause torchscript tests failing.

Question:

  • What properties should we ignore and not ignore? (since I am not sure), but we are only converting LightningModule to TorchScript, right?

TorchScript doesn't support Union type.

There is a related PR #3295 doing similarly for 1.7 JIT support.

Finally for these kind of breaking changes, would it be better to have separate ci/badge?

@williamFalcon @Borda @nateraw @awaelchli

@ydcjeff ydcjeff marked this pull request as ready for review September 24, 2020 16:10
@mergify mergify bot requested a review from a team September 24, 2020 16:10
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team September 24, 2020 17:29
@Borda Borda added the ci Continuous Integration label Sep 24, 2020
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team September 24, 2020 20:32
@ydcjeff ydcjeff marked this pull request as draft September 25, 2020 05:34
@ydcjeff ydcjeff marked this pull request as ready for review September 25, 2020 05:45
@Borda Borda added the ready PRs ready to be merged label Sep 25, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 25, 2020

This pull request is now in conflict... :(

@Borda Borda merged commit 05e5f03 into Lightning-AI:master Sep 25, 2020
@ydcjeff ydcjeff deleted the dockers/pt-1.7 branch September 25, 2020 14:20
@Borda Borda added this to the 0.9.x milestone Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for 1.7 nightly
6 participants