Skip to content

Fix support for torch Module type hints in LightningCLI#7807

Merged
justusschock merged 5 commits intoLightning-AI:masterfrom
mauvilsa:cli_torch_types
Jun 4, 2021
Merged

Fix support for torch Module type hints in LightningCLI#7807
justusschock merged 5 commits intoLightning-AI:masterfrom
mauvilsa:cli_torch_types

Conversation

@mauvilsa
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

carmocca commented Jun 2, 2021 •
edited
Loading

I can reproduce the failure locally on jsonargparse-3.13.0

@mauvilsa
Copy link
Copy Markdown
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
Comment thread CHANGELOG.md Outdated
awaelchli
awaelchli approved these changes Jun 3, 2021
View reviewed changes
Comment thread requirements/extra.txt
onnxruntime>=1.3.0
hydra-core>=1.0
jsonargparse[signatures]>=3.12.0
jsonargparse[signatures]>=3.13.1
Copy link
Copy Markdown
Contributor

@awaelchli awaelchli Jun 3, 2021

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
Copy Markdown
Contributor Author

@mauvilsa mauvilsa Jun 3, 2021

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í <carlossmocholi@gmail.com>
@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í <carlossmocholi@gmail.com>

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

(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í <carlossmocholi@gmail.com>

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

(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

@Borda Borda Awaiting requested review from Borda

@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

+2 more reviewers

@awaelchli awaelchli awaelchli approved these changes

@carmocca carmocca carmocca approved these changes

Reviewers whose approvals may not affect merge requirements

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