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

apply_to_collection improvements and add apply_to_collections #7769

Merged
merged 10 commits into from
Jun 1, 2021
Merged

apply_to_collection improvements and add apply_to_collections #7769

merged 10 commits into from
Jun 1, 2021

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented May 30, 2021 •
edited
Loading

What does this PR do?

Part of #7631

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?
  • [n/a] 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

  • 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

@carmocca carmocca added the feature Is an improvement or enhancement label May 30, 2021
@carmocca carmocca added this to the v1.4 milestone May 30, 2021
@carmocca carmocca self-assigned this May 30, 2021
@codecov
Copy link

codecov bot commented May 30, 2021 •
edited
Loading

Codecov Report

Merging #7769 (b0ac231) into master (a69beab) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7769   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files         199     199           
  Lines       12986   13015   +29     
======================================
+ Hits        11994   12010   +16     
- Misses        992    1005   +13

justusschock
justusschock approved these changes May 31, 2021
View reviewed changes
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/apply_func.py Show resolved Hide resolved
pytorch_lightning/utilities/apply_func.py Show resolved Hide resolved
carmocca
carmocca commented May 31, 2021
View reviewed changes
pytorch_lightning/utilities/apply_func.py
Comment on lines +152 to +156
zipped = {k: (data1[k], data2[k]) for k in data1.keys() | data2.keys()}
return elem_type({
k: apply_to_collections(*v, dtype, function, *args, wrong_dtype=wrong_dtype, **kwargs)
for k, v in zipped.items()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@awaelchli this implementation is not OrderedDict safe as data1.keys() | data2.keys() is a set.

But I don't think we need to implement it in this PR. Could add a warning if the instance is an ordereddict

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized the previous implementation also did not take care of OrderedDict.

awaelchli
awaelchli approved these changes May 31, 2021
View reviewed changes
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
@awaelchli awaelchli added the ready PRs ready to be merged label May 31, 2021
tchaton
tchaton approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

pytorch_lightning/utilities/apply_func.py Show resolved Hide resolved
@tchaton tchaton enabled auto-merge (squash) June 1, 2021 11:53
@tchaton tchaton merged commit 195b24b into master Jun 1, 2021
@tchaton tchaton deleted the enh/apply-fn-improvements branch June 1, 2021 12:09
kandluis
kandluis reviewed Jun 4, 2021
View reviewed changes
pytorch_lightning/utilities/apply_func.py
v = apply_to_collection(v, dtype, function, *args, wrong_dtype=wrong_dtype, **kwargs)
if include_none or v is not None:
out.append((k, v))
return elem_type(out)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no guarantee that an arbitrary Mapping will support construction from Iterate[Tuple[K, T]]. This will fail for custom collections.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, this will fail when data is CfgNode (from YAQS) (https://github.com/rbgirshick/yacs/blob/master/yacs/config.py#L74).

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying this was supported before and now this change broke? Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we use Lightning in Detectron2Go (based on Detectron). The configuration there is specified by YACS (a CfgNode object).

We use the save_hyperameters function on this object, which eventually calls the code here. The previous code (which calls elem_type === CfgDict with a dict) worked fine. With this new change, an error is raised since elem_type === CfgDict cannot be constructed from a List of Tuples.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the modification:

from collections import OrderedDict
...
elem_type(OrderedDict(out))

will fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kandluis are you interested in sending a patch with your proposal?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure -- sent out #7851 :)

@kandluis kandluis mentioned this pull request Jun 7, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli approved these changes

@kandluis kandluis kandluis left review comments

@justusschock justusschock justusschock approved these changes

@tchaton tchaton tchaton 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

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees

@carmocca carmocca

Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.4
Development

Successfully merging this pull request may close these issues.
5 participants
@carmocca @kandluis @awaelchli @tchaton @justusschock