Fix inspection of unspecified args for container hparams #9125

s-rog · 2021-08-26T05:57:37Z

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

The internal logic for qualifying containers has been changed from not isinstance(str) to specific types (Namespace, dict, omegaconf if available). May break undocumented edge case uses.

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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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 🙃

Todo:

omegaconf
tests

the tests only test for functionality, not sure how I would test for "non-inspection" besides passing in a type that breaks the parsing which is not ideal.

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

codecov · 2021-08-31T07:08:52Z

Codecov Report

Merging #9125 (87880c7) into master (cf1a589) will increase coverage by 0%.
The diff coverage is 89%.

@@          Coverage Diff           @@
##           master   #9125   +/-   ##
======================================
  Coverage      92%     92%           
======================================
  Files         178     178           
  Lines       14858   14875   +17     
======================================
+ Hits        13709   13726   +17     
  Misses       1149    1149

tchaton

Hey @s-rog. Mind putting more comments on the code. This part of the code might be complex for new contributors or reviewers.

pytorch_lightning/utilities/parsing.py

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

tchaton

LGTM !

pytorch_lightning/utilities/parsing.py

Co-authored-by: Jirka Borovec <[email protected]>

* Update parsing.py * add todo (for single arg) * unblock non container single arg * init test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update CHANGELOG.md * pep8 line length * Update pytorch_lightning/utilities/parsing.py * remove dict namespace conversion * add omegaconf support * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add dict test * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add omegaconf test * Update CHANGELOG.md * Update pytorch_lightning/utilities/parsing.py Co-authored-by: Jirka Borovec <[email protected]> * Update pytorch_lightning/utilities/parsing.py Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka Borovec <[email protected]>

awaelchli · 2021-09-21T22:44:45Z

@s-rog we may have missed a detail here regarding the _hparams_name, see #9631. Let's see if we can cover this edge case.

awaelchli · 2021-09-21T22:59:05Z

pytorch_lightning/utilities/parsing.py

+    # container
+    elif len(args) == 1 and isinstance(args[0], tuple(hparams_container_types)):
+        hp = args[0]
+        obj._hparams_name = "hparams"


here it is immediately assume that the name of the arg was "hparams". previously, the logic below was checking for matching argument names in the signature.

While this old way of passing in hyperparameters is not our recommended way, we have not officially dropped backward compatibility for this and therefore this was an unwanted breaking change imo.

See #9631 for an example of what was working previously.

…)" This reverts commit 904dde7.

…container (#9642)

Update parsing.py

ae617b7

s-rog added the bug Something isn't working label Aug 26, 2021

s-rog added this to the v1.4.x milestone Aug 26, 2021

s-rog self-assigned this Aug 26, 2021

s-rog requested a review from awaelchli August 26, 2021 06:05

s-rog and others added 7 commits August 26, 2021 14:11

add todo (for single arg)

8b0e5f1

unblock non container single arg

95f8914

init test

00f5b27

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

2911c70

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

Update CHANGELOG.md

b5eb070

Merge branch 'master' into bug/hparams-parsing

df9289e

pep8 line length

59dd364

s-rog marked this pull request as ready for review August 31, 2021 06:50

s-rog requested review from Borda, carmocca, justusschock, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners August 31, 2021 06:50

tchaton reviewed Sep 1, 2021

View reviewed changes

pytorch_lightning/utilities/parsing.py Outdated Show resolved Hide resolved

pytorch_lightning/utilities/parsing.py Outdated Show resolved Hide resolved

s-rog commented Sep 1, 2021

View reviewed changes

pytorch_lightning/utilities/parsing.py Outdated Show resolved Hide resolved

Update pytorch_lightning/utilities/parsing.py

19b3cf7

awaelchli reviewed Sep 1, 2021

View reviewed changes

pytorch_lightning/utilities/parsing.py Outdated Show resolved Hide resolved

s-rog and others added 5 commits September 2, 2021 09:00

remove dict namespace conversion

223527a

add omegaconf support

1ee5936

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

e4f4100

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

add dict test

d28cc40

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

f08205a

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

add omegaconf test

4a95f93

mergify bot added the has conflicts label Sep 2, 2021

awaelchli approved these changes Sep 3, 2021

View reviewed changes

Merge branch 'master' into bug/hparams-parsing

a1351e3

mergify bot removed the has conflicts label Sep 3, 2021

tchaton approved these changes Sep 3, 2021

View reviewed changes

mergify bot added ready PRs ready to be merged has conflicts labels Sep 3, 2021

Merge branch 'master' into bug/hparams-parsing

df39271

mergify bot removed the has conflicts label Sep 5, 2021

Update CHANGELOG.md

0c5654c

Borda reviewed Sep 6, 2021

View reviewed changes

pytorch_lightning/utilities/parsing.py Outdated Show resolved Hide resolved

pytorch_lightning/utilities/parsing.py Outdated Show resolved Hide resolved

s-rog and others added 2 commits September 6, 2021 16:19

Update pytorch_lightning/utilities/parsing.py

1ff4320

Co-authored-by: Jirka Borovec <[email protected]>

Update pytorch_lightning/utilities/parsing.py

87880c7

Co-authored-by: Jirka Borovec <[email protected]>

Borda approved these changes Sep 6, 2021

View reviewed changes

s-rog enabled auto-merge (squash) September 6, 2021 09:37

s-rog merged commit 904dde7 into master Sep 6, 2021

s-rog deleted the bug/hparams-parsing branch September 6, 2021 09:48

awaelchli mentioned this pull request Sep 21, 2021

incorrect "hparam_name" saved to checkpoint when saving from a params in a container #9631

Closed

awaelchli reviewed Sep 21, 2021

View reviewed changes

awaelchli added a commit that referenced this pull request Sep 22, 2021

Revert "Fix inspection of unspecified args for container hparams (#9125…

dda6f5f

…)" This reverts commit 904dde7.

awaelchli mentioned this pull request Sep 22, 2021

revert #9125 / fix back-compatibility with saving hparams as a whole container #9642

Merged

11 tasks

tchaton pushed a commit that referenced this pull request Sep 22, 2021

revert #9125 / fix back-compatibility with saving hparams as a whole …

b98ce0a

…container (#9642)

awaelchli added a commit that referenced this pull request Sep 22, 2021

revert #9125 / fix back-compatibility with saving hparams as a whole …

36e4616

…container (#9642)

lexierule pushed a commit that referenced this pull request Sep 22, 2021

revert #9125 / fix back-compatibility with saving hparams as a whole …

25bfd06

…container (#9642)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inspection of unspecified args for container hparams #9125

Fix inspection of unspecified args for container hparams #9125

s-rog commented Aug 26, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

tchaton left a comment

tchaton left a comment

awaelchli commented Sep 21, 2021

awaelchli Sep 21, 2021

awaelchli Sep 21, 2021

Fix inspection of unspecified args for container hparams #9125

Fix inspection of unspecified args for container hparams #9125

Conversation

s-rog commented Aug 26, 2021 • edited Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Todo:

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

tchaton left a comment

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

awaelchli commented Sep 21, 2021

awaelchli Sep 21, 2021

Choose a reason for hiding this comment

awaelchli Sep 21, 2021

Choose a reason for hiding this comment

s-rog commented Aug 26, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading