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

[Feature] Add test of get_hooks_info() #672

Merged
merged 14 commits into from
Nov 22, 2022
Merged

[Feature] Add test of get_hooks_info() #672

merged 14 commits into from
Nov 22, 2022

Conversation

songyuc
Copy link
Contributor

@songyuc songyuc commented Nov 3, 2022 •
edited
Loading

Motivation

This PR is linked to #599

Modification

I added the test of runner.get_hook_info() in tests/test_runner/test_runner.py

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

Checklist

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  3. If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMCls.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

@songyuc songyuc changed the title Add test of get_hooks_info() [Feature] Add test of get_hooks_info() Nov 3, 2022
HAOCHENYE
HAOCHENYE previously approved these changes Nov 3, 2022
View reviewed changes
mmengine/hooks/hook.py Outdated Show resolved Hide resolved
mmengine/hooks/hook.py Outdated Show resolved Hide resolved
tests/test_runner/test_runner.py Outdated Show resolved Hide resolved
tests/test_runner/test_runner.py Outdated Show resolved Hide resolved
tests/test_runner/test_runner.py Outdated Show resolved Hide resolved
@HAOCHENYE
Copy link
Collaborator

Approve by mistake 🤣 ....

HAOCHENYE
HAOCHENYE reviewed Nov 3, 2022
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
HAOCHENYE
HAOCHENYE previously approved these changes Nov 4, 2022
View reviewed changes
mmengine/runner/runner.py Outdated Show resolved Hide resolved
tests/test_runner/test_runner.py Outdated Show resolved Hide resolved
@zhouzaida zhouzaida requested a review from mzr1996 November 4, 2022 09:41
@zhouzaida zhouzaida linked an issue Nov 9, 2022 that may be closed by this pull request
[Feature Request] Log hook info #599
Closed
@zhouzaida zhouzaida added this to the 0.4.0 milestone Nov 9, 2022
HAOCHENYE
HAOCHENYE reviewed Nov 17, 2022
View reviewed changes
mmengine/hooks/hook.py Outdated Show resolved Hide resolved
mmengine/hooks/hook.py Outdated Show resolved Hide resolved
mmengine/runner/runner.py Outdated Show resolved Hide resolved
@HAOCHENYE HAOCHENYE added the enhancement New feature or request label Nov 17, 2022
@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@d312bc4). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head d1b3614 differs from pull request most recent head 79c8d73. Consider uploading reports for the commit 79c8d73 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #672   +/-   ##
=======================================
  Coverage        ?   78.50%           
=======================================
  Files           ?      127           
  Lines           ?     9215           
  Branches        ?     1836           
=======================================
  Hits            ?     7234           
  Misses          ?     1670           
  Partials        ?      311
Flag Coverage Δ
unittests 78.50% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zhouzaida
Copy link
Collaborator

Hi @songyuc , thanks for your contribution. One thing to note is that MMEngine support adding additional custom hook methods so this logic for printing hook information is a little different from MMCV.

mmengine/mmengine/runner/runner.py

Lines 1735 to 1749 in 79c8d73

def call_hook(self, fn_name: str, **kwargs) -> None:
"""Call all hooks.
Args:
fn_name (str): The function name in each hook to be called, such as
"before_train_epoch".
**kwargs: Keyword arguments passed to hook.
"""
for hook in self._hooks:
# support adding additional custom hook methods
if hasattr(hook, fn_name):
try:
getattr(hook, fn_name)(self, **kwargs)
except TypeError as e:
raise TypeError(f'{e} in {hook}') from None

@HAOCHENYE HAOCHENYE modified the milestones: 0.4.0, 0.3.2 Nov 22, 2022
@zhouzaida zhouzaida merged commit 6636f07 into open-mmlab:main Nov 22, 2022
@songyuc songyuc deleted the dev_log branch November 22, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HAOCHENYE HAOCHENYE HAOCHENYE left review comments

@zhouzaida zhouzaida zhouzaida approved these changes

@RangiLyu RangiLyu RangiLyu approved these changes

@mzr1996 mzr1996 Awaiting requested review from mzr1996

Assignees

@ice-tong ice-tong

Labels
enhancement New feature or request
Projects
None yet
Milestone
0.3.2
Development

Successfully merging this pull request may close these issues.

[Feature Request] Log hook info
5 participants
@songyuc @HAOCHENYE @zhouzaida @RangiLyu @ice-tong