-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Finish PR #2432: Imagenet example updates + basic testing #2889
Conversation
Hello @awaelchli! Thanks for updating this PR.
Comment last updated at 2020-08-09 02:48:45 UTC |
Codecov Report
@@ Coverage Diff @@
## master #2889 +/- ##
======================================
Coverage 90% 90%
======================================
Files 79 79
Lines 7236 7236
======================================
Hits 6530 6530
Misses 706 706 |
You can still use this one... =) |
74a3182
to
ef2839c
Compare
…when ddp (#2432) * fix imagenet example: lr_scheduler, loader workers, batch size when ddp * Fix evaluation for imagenet example * add imagenet example test * cleanup * gpu * add imagenet example evluation test * fix test output * test is fixed in master, remove unecessary hack * CHANGE * Apply suggestions from code review Co-authored-by: Jirka <jirka@pytorchlightning.ai> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
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.
LGTM 🚀 Nice work as always, Adrian.
with mock.patch("argparse._sys.argv", ["any.py"] + cli_args): | ||
run_cli() | ||
|
||
|
||
# @pytest.mark.parametrize('cli_args', ['--max_epochs 1 --max_steps 3 --num_nodes 1 --gpus 2']) |
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.
Can we remove these comments if we don't need them?
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.
Yeah I was wondering myself. They are outdated and have to be rewritten anyway if we ever get multi-node ddp testing.
@@ -1,13 +1,27 @@ | |||
""" | |||
This example is largely adapted from https://github.com/pytorch/examples/blob/master/imagenet/main.py | |||
|
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 is great info, thanks.
It seems the current --evaluate won't load the checkpoint from --resume_from_checkpoint, right? I saw in the doc:
|
oh yes, I think you are right. Would you like to send a PR? |
I am not sure what is the most elegant solution:
But I think it's too ugly. |
we could simply pass the checkpoint path to .test |
ok, correction, then |
Doesn't that assume trainer already has the model? My understanding, if you want to use this, you have to run it after trainer.fit(model) otherwise trainer doesn't know what is the model. |
we could load the state at the beginning, like basically as you proposed, but with all additional params if args.evaluate:
model = ImageNetLightningModel.load_from_checkpoint(path, map_location=...) |
What does this PR do?
Finishes an old PR #2432 by @ruotianluo