-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add runner type #118
Add runner type #118
Conversation
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
==========================================
- Coverage 83.32% 83.27% -0.05%
==========================================
Files 83 83
Lines 3921 3947 +26
Branches 617 623 +6
==========================================
+ Hits 3267 3287 +20
- Misses 520 524 +4
- Partials 134 136 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
logger=logger, | ||
meta=meta) | ||
runner = build_runner( | ||
cfg.runner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the backward compatibility. It should be better to keep the original api and add a deprecation warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure about how to maintain the backward compatibility so I create a default runner section that reproduces the old behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, if the user is using an old config file, there will be error since max_iters
is not specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may keep the backward compatibility so that user may use old config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the block added above:
if cfg.get('runner') is None:
cfg.runner = {'type': 'IterBasedRunner', 'max_iters': cfg.total_iters}
warnings.warn(
'config is now expected to have a `runner` section, '
'please set `runner` in your config.', DeprecationWarning)
User should be able to use an old config file, right?
LGTM. @xvjiarui may take a pass. |
* Add runner_type option * pre-commit * Fix max_iters * Add by_epoch to EvalHook * Add test_eval_hook for epoch runner * Remove runner-type arg from tools/train * Add missing every_n_iters check for epoch mode * Bump mmcv min version * Use build_runner * Use interval in tests * Update test_eval_hook.py * Use every_n_epochs instead of every_n_iters. Update DistEvalHook * Add test_dist_eval_hook_epoch * Fix tests * Add DeprecationWarning * Update docs * Replace DeprecationWarning with UserWarning
final fixes before release
* init changelog * update * update * update * update * update * update
This P.R. uses mmcv's
build_runner
in order to allow to select between EpochBasedRunner and IterBasedRunner. Defaults to IterBasedRunner in order to maintain the original behaviour.