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.

Sign up for GitHub

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

Jump to bottom

Fix support for torch Module type hints in LightningCLI #7807

Merged
merged 5 commits into from
Jun 4, 2021
Merged

Fix support for torch Module type hints in LightningCLI #7807

merged 5 commits into from
Jun 4, 2021

Conversation

mauvilsa
Copy link
Contributor

@mauvilsa mauvilsa commented Jun 2, 2021 •
edited
Loading

What does this PR do?

Fixes a problem mentioned in #7774 (comment). It does not resolve the whole issue, only that single comment.

Before submitting

  • Was this discussed/approved 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?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Jun 2, 2021 •
edited
Loading

Codecov Report

Merging #7807 (c3db752) into master (0a72fd2) will decrease coverage by 5%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #7807    +/-   ##
=======================================
- Coverage      93%     88%    -5%     
=======================================
  Files         199     199            
  Lines       13008   13008            
=======================================
- Hits        12044   11425   -619     
- Misses        964    1583   +619

@mauvilsa mauvilsa marked this pull request as ready for review June 2, 2021 14:48
@carmocca carmocca added this to the v1.3.x milestone Jun 2, 2021
@carmocca carmocca added bug Something isn't working argparse (removed) Related to argument parsing (argparse, Hydra, ...) labels Jun 2, 2021
@mauvilsa
Copy link
Contributor Author

mauvilsa commented Jun 2, 2021

I did verify that the tests were successful locally. Not sure why it is failing.

@carmocca
Copy link
Contributor

carmocca commented Jun 2, 2021 •
edited
Loading

I can reproduce the failure locally on jsonargparse-3.13.0

@mauvilsa
Copy link
Contributor Author

mauvilsa commented Jun 2, 2021

I guess torchvision has changed a log between versions. In https://github.com/PyTorchLightning/pytorch-lightning/pull/7807/checks?check_run_id=2728907218 the error is "torchvision.transforms.Resize" is not a subclass of Module. I upgraded torchvision in my venv and now it does fail but the error is different ('cannot represent an object', <InterpolationMode.BILINEAR: 'bilinear'>). I don't know with which version it was successful for me.

I will look into this last error and probably also change the test to use something a bit more stable.

@mauvilsa mauvilsa marked this pull request as draft June 2, 2021 17:32
mauvilsa and others added 3 commits June 3, 2021 07:48
@mauvilsa
- Fix issue with serializing values when type hint is Any.
4bf8017 
- Run unit test only on newer torchvision versions in which the base class is Module.
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
5e2f919 
for more information, see https://pre-commit.ci
@mauvilsa
Minor change
fd844f1
@mauvilsa mauvilsa marked this pull request as ready for review June 3, 2021 06:44
carmocca
carmocca approved these changes Jun 3, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
awaelchli
awaelchli approved these changes Jun 3, 2021
View reviewed changes
requirements/extra.txt
@@ -7,4 +7,4 @@ torchtext>=0.5
# onnx>=1.7.0
onnxruntime>=1.3.0
hydra-core>=1.0
jsonargparse[signatures]>=3.12.0
jsonargparse[signatures]>=3.13.1
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the fix is there.

@mauvilsa @carmocca
Update CHANGELOG.md
c3db752 
Co-authored-by: Carlos Mocholí <[email protected]>
@justusschock justusschock merged commit f345840 into Lightning-AI:master Jun 4, 2021
@mauvilsa mauvilsa deleted the cli_torch_types branch June 4, 2021 05:47
@SeanNaren SeanNaren mentioned this pull request Jun 8, 2021
Closed
SeanNaren pushed a commit that referenced this pull request Jun 8, 2021
@mauvilsa
Fix support for torch Module type hints in LightningCLI (#7807)
c585913 
* Fixed support for torch Module type hints in LightningCLI

* - Fix issue with serializing values when type hint is Any.
- Run unit test only on newer torchvision versions in which the base class is Module.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Minor change

* Update CHANGELOG.md

Co-authored-by: Carlos Mocholí <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Carlos Mocholí <[email protected]>

(cherry picked from commit f345840)
lexierule pushed a commit that referenced this pull request Jun 9, 2021
@mauvilsa @lexierule
Fix support for torch Module type hints in LightningCLI (#7807)
057f05f 
* Fixed support for torch Module type hints in LightningCLI

* - Fix issue with serializing values when type hint is Any.
- Run unit test only on newer torchvision versions in which the base class is Module.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Minor change

* Update CHANGELOG.md

Co-authored-by: Carlos Mocholí <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Carlos Mocholí <[email protected]>

(cherry picked from commit f345840)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justusschock justusschock justusschock approved these changes

@carmocca carmocca carmocca approved these changes

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
argparse (removed) Related to argument parsing (argparse, Hydra, ...) bug Something isn't working
Projects
None yet
Milestone
pl:1.3.x
Development

Successfully merging this pull request may close these issues.
4 participants
@mauvilsa @carmocca @awaelchli @justusschock