Unify attribute finding logic, fix not using dataloader when hparams present #4559

rnett · 2020-11-06T22:27:51Z

Simple PR to fix #4558.

Unifies attribute finding logic into lightning_get_attr_holder, which does what it says and returns the object or dict with the target attribute, or none if it can't find one. The lightning get/set/has attribute methods then use it.

Changes the logic of attribute finding (in lightning_get_attr_holder) to check model, model.hparams, and dataload, and return the first one that has the attribute. Previously, if the model didn't have the attribute, but did have hparams, it would always return the result of checking hparams and never fall through to dataloader (unless hparams didn't exist, but it's a property). This change needs reviewing.

This successfully fixes the issue in my code.

I'm having issues getting the tests/utilities/parsing.py tests to pass because the test model objects don't have trainer attributes. As far as I can see, this would have caused test failures in the original code too, what's the solution here? It does not seem to be an actual issue.

pep8speaks · 2020-11-06T22:27:54Z

Hello @rnett! Thanks for updating this PR.

In the file pytorch_lightning/utilities/parsing.py:

Line 225:121: E501 line too long (145 > 120 characters)

Comment last updated at 2021-01-25 23:47:24 UTC

pytorch_lightning/utilities/parsing.py

Borda · 2020-11-30T17:47:37Z

@rnett how is it going, can we merge it next week?

rnett · 2020-11-30T20:48:40Z

This is finals week, but I should at a minimum be able to finish it over the weekend.

rnett · 2020-12-07T00:04:04Z

Should be good to go.

pytorch_lightning/utilities/parsing.py

carmocca · 2020-12-07T04:30:18Z

Can you merge/rebase master?

rnett · 2020-12-07T05:00:33Z

Rebase done.

codecov · 2020-12-07T07:47:06Z

Codecov Report

Merging #4559 (553251f) into master (c76cc23) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4559   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         135     135           
  Lines       10051   10048    -3     
======================================
- Hits         9383    9380    -3     
  Misses        668     668

rnett · 2020-12-07T07:51:30Z

Tests are passing again, the code check failures are 1) an error in PyRight and 2) not in my code.

pytorch_lightning/utilities/parsing.py

carmocca

Good job! This is shaping up nicely 👍

One more comment:

Can you add a test with the case that motivated this change?

Previously, if the model didn't have the attribute, but did have hparams, it would always return the result of checking hparams and never fall through to dataloader (unless hparams didn't exist, but it's a property).

pytorch_lightning/utilities/parsing.py

rnett · 2020-12-07T21:05:56Z

Something else I noticed: the old getattr updates attr each time it finds a matching attribute, and will return the last one. I.e. it will prefer dataloder.attr over model.attr. I'm fixing my function to match it, but can someone verify this is the intended behavior? It seems possibly backwards (although I could see the rational either way).

carmocca · 2020-12-07T23:53:51Z

Something else I noticed: the old getattr updates attr each time it finds a matching attribute, and will return the last one. I.e. it will prefer dataloder.attr over model.attr. I'm fixing my function to match it, but can someone verify this is the intended behavior? It seems possibly backwards (although I could see the rational either way).

I'm not sure if this is intended or just an implementation detail. You could add a comment to indicate that the last is taken for backwards compatibility

carmocca

LGTM

SeanNaren

Nice catch, took me some time to grok but the fix cleans a lot of the attribute handling. Thanks @rnett!

Borda · 2021-01-08T13:10:54Z

pytorch_lightning/utilities/parsing.py

-def lightning_hasattr(model, attribute):
-    """ Special hasattr for lightning. Checks for attribute in model namespace,
-        the old hparams namespace/dict, and the datamodule. """
+def lightning_get_all_attr_holders(model, attribute):


be careful, this is API change
we are free to change anything which is protected (starting with _) but the remaining is public API
so even when we are adding new features, make a decision about what is meant to the public and protected api
cc: @PyTorchLightning/core-contributors

lightning_hasattr is still there, the diff is just not rendering properly

Borda · 2021-01-08T13:12:40Z

pytorch_lightning/utilities/parsing.py

-            attr = attribute in model.hparams
-        else:
-            attr = hasattr(model.hparams, attribute)
+    if hasattr(model, 'hparams'):


hparams is property so what is the case it would be missing? some old model versions?

This function is just collectin attr candidates. If there is no hparams, one of the previous/next candidates will be used

Borda · 2021-01-08T13:13:56Z

pytorch_lightning/utilities/parsing.py

    # Check if the attribute in datamodule (datamodule gets registered in Trainer)
-    if not attr and trainer is not None:
-        attr = hasattr(trainer.datamodule, attribute)
+    if trainer is not None and trainer.datamodule is not None and hasattr(trainer.datamodule, attribute):


datamodule is also quite new

Suggested change

if trainer is not None and trainer.datamodule is not None and hasattr(trainer.datamodule, attribute):

if trainer is not None and hasattr(trainer, 'datamodule') is not None and hasattr(trainer.datamodule, attribute):

I'm not against being extra-safe but this should not fail. Unless a user is using this code with their own Trainer that does not inherit PL's

@Borda I think we don't need this, since we only expect this code to be used with the current version of the trainer and not some old.

Plus, if you want to add this, instead of hasattr(trainer, 'datamodule') you'd have to do getattr(trainer, 'datamodule', None) sinde hasattr` only return True or False.

Borda · 2021-01-08T13:16:02Z

pytorch_lightning/utilities/parsing.py

+    return holders
+
+
+def lightning_get_first_attr_holder(model, attribute):


do we really need this? is also not very consistent as the name says "first" but you take last as [-1]

As mentioned in line 229, the last is taken to preserve backwards compatibility. In case somebody is relying on the attribute candidate order

Borda · 2021-01-08T13:16:42Z

pytorch_lightning/utilities/parsing.py

+def lightning_hasattr(model, attribute):
+    """ Special hasattr for lightning. Checks for attribute in model namespace,
+        the old hparams namespace/dict, and the datamodule. """
+    return lightning_get_first_attr_holder(model, attribute) is not None


not sure of the usage

Can you elaborate?

Borda · 2021-01-08T13:18:16Z

tests/utilities/test_parsing.py

@@ -20,6 +20,9 @@ def _get_test_cases():
    class TestHparamsNamespace:
        learning_rate = 1

+        def __contains__(self, item):


mind add context for this state? (as a docstring)

s-rog · 2021-01-11T01:01:27Z

pytorch_lightning/utilities/parsing.py

+    if len(holders) == 0:
+        return None
+    # using the last holder to preserve backwards compatibility
+    return holders[-1]


So if an attribute key is in both pl_module and datamodule, the datamodule attr will be used? Is this intended?

I can't say for sure if that was the original intention but it's not a bad idea. Usually, if a datamodule is given it's because you want to use it.

The edge case is where the pl_module and datamodule contain the same key but different values. I guess this ties in with #3792

justusschock · 2021-01-11T12:13:47Z

pytorch_lightning/utilities/parsing.py

    # Check if the attribute in datamodule (datamodule gets registered in Trainer)
-    if not attr and trainer is not None:
-        attr = hasattr(trainer.datamodule, attribute)
+    if trainer is not None and trainer.datamodule is not None and hasattr(trainer.datamodule, attribute):


@Borda I think we don't need this, since we only expect this code to be used with the current version of the trainer and not some old.

Plus, if you want to add this, instead of hasattr(trainer, 'datamodule') you'd have to do getattr(trainer, 'datamodule', None) sinde hasattr` only return True or False.

…present (#4559) * Rebase onto master * indent fix * Remove duplicated logic * Use single return * Remove extra else * add `__contains__` to TestHparamsNamespace to fix tests * Fix lightning_setattr to set all valid attributes * update doc * better names * fix holder order preference * tests for new behavior * Comment about using the last holder Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> (cherry picked from commit eee3b1a)

rnett requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, tchaton, teddykoker and williamFalcon as code owners November 6, 2020 22:27

Borda reviewed Nov 8, 2020

View reviewed changes

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

Borda added the refactor label Nov 8, 2020

Borda added this to the 1.1 milestone Nov 8, 2020

akihironitta mentioned this pull request Nov 9, 2020

Add tests/utilities/test_parsing.py #4460

Merged

17 tasks

carmocca reviewed Nov 22, 2020

View reviewed changes

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

carmocca reviewed Nov 22, 2020

View reviewed changes

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

Borda modified the milestones: 1.1, 1.1.x Nov 30, 2020

carmocca reviewed Dec 7, 2020

View reviewed changes

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

rnett force-pushed the rn_fix_attr_finding branch from 9067801 to 7d73675 Compare December 7, 2020 04:59

Borda reviewed Dec 7, 2020

View reviewed changes

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

carmocca reviewed Dec 7, 2020

View reviewed changes

pytorch_lightning/utilities/parsing.py Show resolved Hide resolved

rnett added 4 commits December 7, 2020 13:13

update doc

fee57ba

better names

465d7e9

fix holder order preference

cdf2e95

tests for new behavior

ab55ff0

rnett force-pushed the rn_fix_attr_finding branch from 997a13f to ab55ff0 Compare December 7, 2020 21:13

carmocca approved these changes Dec 7, 2020

View reviewed changes

rnett and others added 2 commits December 7, 2020 16:07

Comment about using the last holder

50f94ac

Merge branch 'master' into rn_fix_attr_finding

18f2895

mergify bot requested a review from a team December 12, 2020 14:59

SeanNaren added bug Something isn't working priority: 1 Medium priority task labels Jan 7, 2021

Merge branch 'master' into rn_fix_attr_finding

c1c85cd

SeanNaren approved these changes Jan 7, 2021

View reviewed changes

SeanNaren enabled auto-merge (squash) January 7, 2021 16:45

Borda reviewed Jan 8, 2021

View reviewed changes

s-rog reviewed Jan 11, 2021

View reviewed changes

justusschock approved these changes Jan 11, 2021

View reviewed changes

mergify bot added 7 commits January 25, 2021 00:32

Merge branch 'master' into rn_fix_attr_finding

772880d

Merge branch 'master' into rn_fix_attr_finding

a3100bc

Merge branch 'master' into rn_fix_attr_finding

f3e7c0f

Merge branch 'master' into rn_fix_attr_finding

3dbc9fa

Merge branch 'master' into rn_fix_attr_finding

e0862b7

Merge branch 'master' into rn_fix_attr_finding

86bb020

Merge branch 'master' into rn_fix_attr_finding

553251f

SeanNaren merged commit eee3b1a into Lightning-AI:master Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify attribute finding logic, fix not using dataloader when hparams present #4559

Unify attribute finding logic, fix not using dataloader when hparams present #4559

rnett commented Nov 6, 2020

pep8speaks commented Nov 6, 2020 •

edited

Loading

Borda commented Nov 30, 2020

rnett commented Nov 30, 2020

rnett commented Dec 7, 2020

carmocca commented Dec 7, 2020

rnett commented Dec 7, 2020

codecov bot commented Dec 7, 2020 •

edited

Loading

rnett commented Dec 7, 2020

carmocca left a comment •

edited

Loading

rnett commented Dec 7, 2020

carmocca commented Dec 7, 2020

carmocca left a comment

SeanNaren left a comment

Borda Jan 8, 2021

carmocca Jan 8, 2021

Borda Jan 8, 2021

carmocca Jan 8, 2021

Borda Jan 8, 2021

carmocca Jan 8, 2021

justusschock Jan 11, 2021

Borda Jan 8, 2021

carmocca Jan 8, 2021

Borda Jan 8, 2021

carmocca Jan 8, 2021

Borda Jan 8, 2021

s-rog Jan 11, 2021

carmocca Jan 11, 2021

s-rog Jan 12, 2021

justusschock Jan 11, 2021

	if trainer is not None and trainer.datamodule is not None and hasattr(trainer.datamodule, attribute):
	if trainer is not None and hasattr(trainer, 'datamodule') is not None and hasattr(trainer.datamodule, attribute):

		return holders


		def lightning_get_first_attr_holder(model, attribute):

Unify attribute finding logic, fix not using dataloader when hparams present #4559

Unify attribute finding logic, fix not using dataloader when hparams present #4559

Conversation

rnett commented Nov 6, 2020

pep8speaks commented Nov 6, 2020 • edited Loading

Comment last updated at 2021-01-25 23:47:24 UTC

Borda commented Nov 30, 2020

rnett commented Nov 30, 2020

rnett commented Dec 7, 2020

carmocca commented Dec 7, 2020

rnett commented Dec 7, 2020

codecov bot commented Dec 7, 2020 • edited Loading

Codecov Report

rnett commented Dec 7, 2020

carmocca left a comment • edited Loading

Choose a reason for hiding this comment

rnett commented Dec 7, 2020

carmocca commented Dec 7, 2020

carmocca left a comment

Choose a reason for hiding this comment

SeanNaren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Nov 6, 2020 •

edited

Loading

codecov bot commented Dec 7, 2020 •

edited

Loading

carmocca left a comment •

edited

Loading