Add Iteration base class

[Nic-Ma]

Description

Similar as the PrepareBatch signature, this PR defines the base class Iteration for every iteration of the precess_function of ignite engine.

Status

Ready

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
• Quick tests passed locally by running ./runtests.sh --quick --unittests.
• In-line docstrings updated.
• Documentation updated, tested make html command in the docs/ folder.

[Nic-Ma]

/black

[wyli]

ignite's definition is more flexible than this
https://github.com/pytorch/ignite/blob/4597bbcb17f546b0165d5573c94e8f2b952c93fd/ignite/engine/engine.py#L123, any reason that we want to enforce this definition?

[Nic-Ma]

Hi @wyli ,

Thanks for your review.
Sorry maybe I didn't describe the requirements clear. Actually, the process_function is stated as Callable function in ignite, but actually, it must accept engine and batch as the input args:
https://github.com/pytorch/ignite/blob/master/ignite/engine/engine.py#L850
Here we define the Iteration as basic signature of user-customized process_function, just like the PrepareBatch API we already have:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/engines/utils.py#L130
It can be useful for users to extend their own iteration logic in MONAI workflows, for example to config the customized Iteration in NVIDIA Clara MMAR.
Add @ericspod @vfdev-5 for discussion and review.

Thanks in advance.

[wyli]

Thanks, in my opinion we should allow for full flexibility and don't add the signature here.

[Nic-Ma]

Hi @ericspod @vfdev-5 , I think the process_function in ignite must accept engine and batch args, so we defined this signature to help users develop their own iteration in NVIDIA Clara (like the PrepareBatch signature), I tried to port this feature to MONAI in this PR. What do you guys think?

Thanks in advance.

[Nic-Ma]

And here is the proposal to support 4 levels of user customization for our workflow:

1. If custimize input data format, extend from the existing monai.engines.PrepareBatch signature.
2. If customize computation of every iteration, extend from monai.engines.Iteration signature, this PR.
3. If customize the overall trainer logic, extend from existing monai.engines.SupervisedTrainer signature.
4. If even don't want to follow ignite engine, extend from the existing monai.engines.BaseWorkflow signature.

Thanks.

[Nic-Ma]

Hi @wyli @vfdev-5 ,

Thanks for the interesting discussion here.
I don't quite understand "Otherwise the user can override Workflow._iteration", this signature is for the level 2 customization, if you have more complicated logic, you can extend the SupervisedTrainer in level 3.
Maybe I didn't make the use case clear enough, let me show you some example code:

from monai.engines import Iteration, SupervisedTrainer


class MyIteration(Iteration):
    def __call__(self, engine: Engine, batchdata: Dict[str, torch.Tensor]):
        XXX  # my computation logic for 1 iteration

my_iteration = MyIteration(...)
my_trainer = SupervisedTrainer(iteration_update=my_iteration, ...)

my_trainer.run()

And some Clara Train config example:

{
    "iteration": {
        "name": "MyIteration",
        "args": {
            ...
        }
    },
    "trainer": {
        "name": "SupervisedTrainer",
        "args": {
            "iteration_update": "@iteration",
            ...
        }
    }
}

@ericspod What do you think about this signature denination? I think it's same as the previous PrepareBatch.

Thanks in advance.

[wyli]

During the development of monai, we've been following the principle of minimising this kind of framework-like contraints and this makes it simple when integrating monai with other packages. Now with this PR, the users have to understand and inherit an Iteration base class in order to customise the ignite iteration, but it's unclear to me what the benefit is for monai.

[Nic-Ma]

Hi @wyli ,

Thanks for your good point.
But here we don't change the API type-hints for arg iteration_update of SupervisedTrainer and don't force it to be Iteration.
It's just a more clear and easier to understand definition of the signature, especially for beginner users, advanced users can pass any callable object or function to iteration_update.

Thanks.

[ericspod]

From what I see the purpose of this definition is to provide a concrete instance of the callable signature that is needed to be used as a process_function. @vfdev-5 is right in that the signature in the Engine definition is incomplete, however the full signature is rather large as seen with Workflow._iteration .

Pythonic coding tends away from defining large signatures like this and relying on plain-language documentation in docstrings to describe what's going on. I think we should adhere to that and not have "interface" definitions like this, ABC and other mechanisms make sense in Python when a class has a mix of dependent abstract and concrete methods that require full implementations. With PrepareBatch there are existing subclasses so it makes a bit more sense to have an abstract base class first.

If we want to stick to the Pythonic way I'd suggest we have in docstrings a description of what process_function/iteration_update accepts and returns, and point to Workflow._iteration as an example

[Nic-Ma]

Hi @wyli @vfdev-5 @ericspod ,

Thanks for the great discussion!
Seems all you guys are suggesting the same pythonic way of doing this, OK, I updated the PR to clarify the signature in the doc-string and type-hints.
Could you please help review it again?

Thanks in advance.

[Nic-Ma]

/build

[Nic-Ma]

/black

[Nic-Ma]

/build

[Nic-Ma]

/build

[vfdev-5]

@Nic-Ma I left few comments about the links on ignite code. Maybe, better to provide links on the docs instead of the code. We'll also update our docs to better reflect the examples of usage for these functions

[Nic-Ma]

/build

[Nic-Ma]

/black