394 enhance engines to provide unified workflow

[Nic-Ma]

Fixes #394 .

Description

This PR enhanced our engines module with trainers and evaluators.

They can provide unified and easier training and evaluation workflows, it will be useful to set advanced strategies, like: JSON config, AutoML and Federated Learning, etc.

For next steps, I will also do these items in other PRs:

1. Update and enhance our handlers, add more useful handlers.
2. Develop workflow integration test.

Status

Ready

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or new feature that would cause existing functionality to change)
• New tests added to cover the changes
• Docstrings/Documentation updated

[Nic-Ma]

/black

[Nic-Ma]

/black

[ericspod]

There is some discussion with Ignite on high level API types that has overlap with this PR: pytorch/ignite#912

[Nic-Ma]

There is some discussion with Ignite on high level API types that has overlap with this PR: pytorch/ignite#912

Hi @ericspod ,

Thanks very much for your review and sharing!
I will totally adjust the workflows tomorrow based on your previous investigation and comments.
Maybe we can use this PR as the baseline and then enhance & add more workflows later.
I think the unified training workflow will be very useful, especially for JSON/YAML config, AutoML and Federated Learning, etc.
Thanks.

[wyli]

thanks, I'm taking more time to think about this PR. since this is related to the ignite engine, perhaps @vfdev-5 @justusschock have some comments as well...

[vfdev-5]

Thanks for mentioning @wyli ! Yes, definitely, classes like SupervisedTrainer make a lot of sense and it is something we aim to provide in Ignite too while keeping the flexibility of Engine.

PS: I also cc @sdesrozis as @justusschock is not working anymore on Ignite.

[Nic-Ma]

/black

[Nic-Ma]

Hi @ericspod and @wyli ,

Thanks very much for your review and feedback!
I updated the PR according to your comments.
Could you please help review it again?
Thanks.

[sdesrozis]

Hi ! I reviewed (quite late) this PR because closely related to ignite API. I really appreciate the smart usage of ignite done here. it makes me think that ignite is exactly in the right position for projects like MONAI : providing building blocks to compose complex applications.

My feedback about the PR is maybe we need in ignite a way to define smart functions in Engine, a bit more than a Callable to help about amp for instance. Moreover, I think that the work we are currently doing about agnostic distributed computing (for tpu, cuda, etc.) should be helpful for MONAI. We will support on that way if you need 👍🏻

[Nic-Ma]

Hi @sdesrozis ,

Thanks very much for your review and feedback from ignite side, glad to co-work with you guys.
I updated the PR according to your comments.
And we will add AMP support when PyTorch v1.6 released(it's just reserved for API now).
We may leverage the AMP feature in ignite directly.

Thanks.

[Nic-Ma]

Hi @ericspod and @wyli ,

I updated this PR with all the latest comments, could you please help review it again?
Thanks.

[Nic-Ma]

/black

[Nic-Ma]

Thanks for @wyli 's review, I updated according to his offline comments:

1. change _run() to run().
2. delete the _iteration() abstract function in Trainer and Evaluator.
3. add doc-string for the global_epoch parameter of Evaluator.
4. change RegularInferer to SimpleInferer.
5. fix several small typos in doc-strings and documentation.
6. refine all the doc-strings in this PR.

I also changed all the assert to raise Exception according to @ericspod 's comments.

@ericspod , @wyli , Could you please help review it again?
Thanks in advance.

[wyli]

looks good to me, need another iteration for the unit tests. thanks for the comments @vfdev-5 @sdesrozis @ericspod

[Nic-Ma]

/black

[Nic-Ma]

Thanks @wyli for your review!

Hi @ericspod ,

Do you have any other comments or suggestions?
Thanks.

[ericspod]

It looks good in general. The approach I had taken with a higher level Engine type was to have a base type inheriting Engine which served the role of an inferer as well as being subtyped as a Trainer and Evaluator classes to suite those roles. The idea was that this provided a simple inference capability regardless of which subtype was used, so one could have inference routines that could work with either.

Another idea was to have a method implementing the network forward pass (pass arguments to the network, packaging results into a tuple) which both subtypes would use, this would reduce code and allow adapting the network to data parallel operations without creating a wrapper type. Similarly there was a method for wrapping the forward pass for the loss function. The idea with this was to allow a subtype to modify how the forward passes worked without having to change any of the other code.

I'm fine with merging this now though, these are ideas for evolution into the future perhaps.

[Nic-Ma]

Hi @ericspod ,

Thanks for your review and for sharing cool ideas!
I think your 2 proposals are very interesting, they are mainly focusing on the optimization for the operations of inferer and networkor loss, which are not enhanced in this PR. This PR is mainly to enhance the operations for iteration, handlers, and metrics in the ignite engines.
So maybe we can work together later to combine your proposals with current Trainer and Evaluator. I will merge this PR first and develop other higher level modules in later PRs.

Thanks.