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

Refactor some loops code and hook tests #7682

Merged
merged 9 commits into from
May 25, 2021

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented May 24, 2021

What does this PR do?

Assortment of minor changes

  • Joined hook order tests
  • Added hook order test for fit when no validation is run
  • Tiny bit of typing
  • Simplified the "should run validation" logic
  • Simplified the update lr logic
  • A test refactor

Reducing the diff for #7357

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)
  • [n/a] Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • [n/a] 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

@mergify mergify bot removed the has conflicts label May 24, 2021
@codecov
Copy link

codecov bot commented May 24, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@fe1c4ca). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #7682   +/-   ##
========================================
  Coverage          ?     92%           
========================================
  Files             ?     199           
  Lines             ?   12965           
  Branches          ?       0           
========================================
  Hits              ?   11957           
  Misses            ?    1008           
  Partials          ?       0           

@carmocca carmocca mentioned this pull request May 24, 2021
8 tasks
@mergify mergify bot removed the has conflicts label May 24, 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.

LGTM !

'on_validation_batch_end',
]

def training_step(self, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

wahy not use Mock? @awaelchli

Copy link
Contributor Author

@carmocca carmocca May 25, 2021

Choose a reason for hiding this comment

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

It's hard to use as there are several checks throughout our codebase for isinstance(LightningModule)

I've tried it in the past without success. You can give it a try if you feel confident :D

Copy link
Member

Choose a reason for hiding this comment

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

ok, so lest check if we have a ticket for mocking this kind of tests and if note create one...

Copy link
Contributor Author

@carmocca carmocca May 25, 2021

Choose a reason for hiding this comment

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

I don't think it's possible, maybe if we could mock the inherited classes defining all these hooks...

No explicit issue for it. Has been brought up a few times across PRs though

tests/models/test_hooks.py Show resolved Hide resolved
@carmocca carmocca merged commit e2ead9a into master May 25, 2021
@carmocca carmocca deleted the refactor/loop-code-and-hook-tests branch May 25, 2021 11:27
awaelchli added a commit that referenced this pull request May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants