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.

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

[bugfix] Minor improvements to apply_to_collection and type signature of log_dict #7851

Merged
merged 3 commits into from
Jun 7, 2021

Conversation

kandluis
Copy link
Contributor

@kandluis kandluis commented Jun 7, 2021

What does this PR do?

Improve apply_to_collection to continue working with CfgNode from yacs

The existing implementation assumes that elem_type is constructible from Iterate[Tuple[K, V]]. This will fail for custom collections.

Specifically, this fails with CfgNode (from YAQS) (https://github.com/rbgirshick/yacs/blob/master/yacs/config.py#L74) which is used in Detectron2Go. We use the save_hyperameters function on this object, which eventually calls apply_to_collection. The code before PR #7769 worked fine, but the existing code raises an error since CfgNode cannot be constructed from a List of Tuples.

See for discussion: #7769 (comment)

Improved type signature for log_dict

When merging into our codebase, we noticed that the new type signature for log_dict is overly restrictive. PR #7771 improved the type of the dictionary parameter from dict to Dict[str, Union[Union[Metric, torch.Tensor, Number], Dict[str, Union[Metric, torch.Tensor, Number]]]]].

However, given Dict[K, V] implies mutability (eg, the function might mutate the dictionary), the type checker defines it as invariant (see python/mypy#2300 (comment)). As such, Dict[str, torch.Tensor] is no longer a valid type to pass to log_dict, which is unintended.

Given that dictionary parameter appears to be read-only in log_dict, the easiest fix is to change from Dict[K, V] to Mapping[K, V]. I believe this better matches the intention of the log_dict function, unless we truly intend for the parameter to be mutable.

See for discussion: #7771 (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 7, 2021

Codecov Report

Merging #7851 (89af805) into master (dff1047) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #7851    +/-   ##
=======================================
- Coverage      93%     88%    -5%     
=======================================
  Files         202     202            
  Lines       13050   13051     +1     
=======================================
- Hits        12085   11460   -625     
- Misses        965    1591   +626     

@justusschock justusschock enabled auto-merge (squash) June 7, 2021 07:12
@justusschock
Copy link
Member

justusschock commented Jun 7, 2021

@kandluis Mind updating changelog?

Edit: already done :)

@awaelchli awaelchli added the bug Something isn't working label Jun 7, 2021
@awaelchli awaelchli added this to the v1.3.x milestone Jun 7, 2021
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.

LGMT !

@justusschock justusschock merged commit 009e05d into Lightning-AI:master Jun 7, 2021
@kandluis
Copy link
Contributor Author

kandluis commented Jun 7, 2021

Thanks @justusschock for updating the changelog. Will keep this in mind for future PRs :)

@SeanNaren SeanNaren mentioned this pull request Jun 8, 2021
Borda pushed a commit that referenced this pull request Jun 8, 2021
…re of `log_dict` (#7851)

* minor fixeS

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

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

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
SeanNaren pushed a commit that referenced this pull request Jun 8, 2021
…re of `log_dict` (#7851)

* minor fixeS

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

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

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 009e05d)
lexierule pushed a commit that referenced this pull request Jun 9, 2021
…re of `log_dict` (#7851)

* minor fixeS

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

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

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 009e05d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants