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

Add functools.wraps support for is_overridden #8296

Merged
merged 4 commits into from
Jul 6, 2021
Merged

Add functools.wraps support for is_overridden #8296

merged 4 commits into from
Jul 6, 2021

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Jul 5, 2021 •
edited
Loading

What does this PR do?

Fixes #8273

Some very minor doc fixes are included

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 bug Something isn't working label Jul 5, 2021
@carmocca carmocca self-assigned this Jul 5, 2021
@carmocca
Copy link
Contributor Author

carmocca commented Jul 5, 2021

@awaelchli Do we need this in the bugfix branch? is_overridden was refactored and the changes aren't there.

@codecov
Copy link

codecov bot commented Jul 5, 2021 •
edited
Loading

Codecov Report

Merging #8296 (419838e) into master (7ddcdb2) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #8296   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files         213     213           
  Lines       13804   13806    +2     
======================================
- Hits        12749   12738   -11     
- Misses       1055    1068   +13

@carmocca carmocca requested a review from ananthsub as a code owner July 5, 2021 16:11
awaelchli
awaelchli approved these changes Jul 5, 2021
View reviewed changes
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

oh this is very nice! I would have never figured this out <3

@awaelchli
Copy link
Contributor

awaelchli commented Jul 5, 2021 •
edited
Loading

@awaelchli Do we need this in the bugfix branch? is_overridden was refactored and the changes aren't there.

Don't think so, because calling the methods when they are not implemented will do nothing. So it's not noticable and apart from the has_x_called there are no side effects afaik.

@awaelchli awaelchli enabled auto-merge (squash) July 5, 2021 17:07
@carmocca carmocca added this to the v1.4 milestone Jul 5, 2021
@carmocca carmocca added the ready PRs ready to be merged label Jul 5, 2021
Borda
Borda reviewed Jul 5, 2021
View reviewed changes
tests/utilities/test_model_helpers.py
return 2

# `functools.wraps()` support
assert not is_overridden("foo", WrappedModel(), parent=TestModel)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this shall mark as overwritten as a wrapped can significantly change its functionality...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation assumes that the wrapper will be the same both for the instance and the parent

Not sure whether we could check if the wrappers are different

Copy link
Member

Choose a reason for hiding this comment

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

can you detect what wrapper was used and then compare the wrapper itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can detect that a wrapper was used (current PR changes) but no idea about comparing the wrappers

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check if the wrapper is from Lightning or users ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth for another PR. I believe this is a pretty rare use-case.

@awaelchli awaelchli merged commit 8fead58 into master Jul 6, 2021
@awaelchli awaelchli deleted the bugfix/is_overridden-wraps-support branch July 6, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaton tchaton tchaton approved these changes

@Borda Borda Borda left review comments

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@awaelchli awaelchli awaelchli approved these changes

@justusschock justusschock Awaiting requested review from justusschock justusschock 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

@ananthsub ananthsub Awaiting requested review from ananthsub

Assignees

@carmocca carmocca

Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.4
Development

Successfully merging this pull request may close these issues.

false positives with is_overridden on DataModule hooks
5 participants
@carmocca @awaelchli @Borda @tchaton @SkafteNicki