Add `dict` properties to Engine and State

[Nic-Ma]

🚀 Feature

Hi @vfdev-5 ,

In the current implementation of MONAI handlers, they are deeply coupled with ignite engine.
Some users that use different workflows can't leverage our handlers, so we are trying to make handlers decouple from ignite.
We discussed the problem and got some basic ideas:

1. Add dict properties to the engine, then handlers can treat engine arg as a regular dict with same structure as ignite Engine object.
2. Pass the event name when trigger handler, so other workflows also can call this handler with the same event name.

For example:

class TestHandler:
    def attach(self):
        # ignite specific logic
       ... ...

    def __call__(self, data: Dict, event: str):
        # data map to the engine dict of ignite
        if event == "ITERATION_COMPLETED":
            self._iteration(data)
        if event == "EPOCH_COMPLETED":
            self._epoch(data)

What do you think?

[sdesrozis]

@Nic-Ma Thank you for suggesting such features !

IMO the two points should be discussed separately, shouldn't they ?

In the first point, the dict is the current data of the engine ? Or more generally the state ?

In the second point, I suppose you want to have one attach (and one handler) and some specific behaviours at the runtime, right ? What is the limitation having multiple attach methods ?

Thank in advance !

EDIT

It missed the engine argument in the attach method in your handler, doesn't it?

[vfdev-5]

@Nic-Ma thanks for the proposing this feature. I'm not quite sure about what kind of final goal you would like to achieve.

In ignite there is no strong requirements on the handler type, so once a callable is attached to the engine it is called.
If your users do something else and has other "engines" with their own "events" when to trigger the callbacks it means that MONAI handler's should adhere this logic as well...
Could you please give a concrete example of where is the issue and we can try to figure out appropriate solution ?

Thanks

[Nic-Ma]

Hi @vfdev-5 and @sdesrozis ,

Thanks for your support here, sorry I didn't make my idea clear enough.
Here is a demo program for my 2 ideas:

1. Add __getitem__() and __setitem__() to the State and Engine of ignite:

from ignite.engine import Engine, Events, State


class DictState(State):
    def __getitem__(self, item):
        return getattr(self, item)

    def __setitem__(self, key, value):
        setattr(self, key, value)


class DictEngine(Engine):
    def __init__(self, process_function: Callable):
        super().__init__(process_function)
        self.state = DictState()

    def __getitem__(self, item):
        return getattr(self, item)

    def __setitem__(self, key, value):
        setattr(self, key, value)

2. Then define a util function to attach handler to the engine with predefined EVENTS of handler.
   This util function is the only API related to ignite here:

def attach_engine(engine: DictEngine, handler: Callable):
    for event in handler.get_events():
        # pass the event as kwarg to handler callback
        engine.add_event_handler(Events[event], handler, event=event)

3. Define the Handler interface and a TestHandler, they don't depend on ignite anymore.
   Just expect the data dictionary has same structure as ignite Engine.

class EVENTS:
    STARTED = "STARTED"
    ITERATION_COMPLETED = "ITERATION_COMPLETED"
    EPOCH_COMPLETED = "EPOCH_COMPLETED"
    COMPLETED = "COMPLETED"
    EXCEPTION_RAISED = "EXCEPTION_RAISED"


class Handler(ABC):
    """
    Base class of all handlers

    """
    def __init__(self, events: Sequence[str]) -> None:
        self.events = events

    def get_events(self):
        return self.events

    @abstractmethod
    def __call__(self, data: Dict, event: str):
        # data should have the same structure as ignite.Engine,
        # which need to support dict properties
        raise NotImplementedError(f"Subclass {self.__class__.__name__} must implement this method.")


class TestHandler(Handler):
    def __init__(self) -> None:
        super().__init__(events=[EVENTS.STARTED, EVENTS.ITERATION_COMPLETED, EVENTS.EPOCH_COMPLETED])

    def __call__(self, data: Dict, event: str):
        # data should have the same structure as ignite.Engine,
        # which need to support dict properties
        if event == EVENTS.STARTED:
            self._started(data)
        if event == EVENTS.ITERATION_COMPLETED:
            self._iteration(data)
        if event == EVENTS.EPOCH_COMPLETED:
            self._epoch(data)

    def _started(self, data: Dict):
        print(f"total epochs: {data['state']['max_epochs']}.")

    def _iteration(self, data: Dict):
        print(f"current iteration: {data['state']['iteration']}.")

    def _epoch(self, data: Dict):
        print(f"should terminated: {data['should_terminate']}.")

4. Here is the test program:

def _train_func(engine, batch):
    return torch.tensor(0.0)

engine = DictEngine(_train_func)
testhandler = TestHandler()
attach_engine(engine, testhandler)
engine.run(range(3), max_epochs=2)

So in summary, the feature request is to add dict properties to ignite Engine and State, then we can make the handlers independent from ignite and users can apply them in their own workflow which uses dict as dataflow.
I think this is a nonbreaking enhancement for ignite.
What do you think?

Thanks.

[Nic-Ma]

Hi @vfdev-5 and @sdesrozis ,

Could you please help share some comments? We are trying to refactor all the handlers in MONAI v0.6, it would be a big change and enhancement for us.
I think maybe it's also helpful and non-breaking to provide the dict properties in ignite Engine and State?

Thanks in advance.

[sdesrozis]

@Nic-Ma Thanks for the snippet of code.

Consider the engine as a dict looks weird to me, I have to think about it, even if it does not break anything.

We will discuss it as soon as possible !

[vfdev-5]

Hi @Nic-Ma , sorry for a late reply !

Normally, we do not force users to have handlers to be dependent on ignite. If he/she is using Engine to schedule events than any function or functor can be used as a handler: https://github.com/pytorch/ignite#power-of-events--handlers

I checked your current handlers structure and I agree that it could refactored and avoid using attach to the engine.
However, I would suggest to wrap the engine and state as dicts from your side. For example, reusing your TestHander and attach_engine functions :

from collections import UserDict
from ignite.engine import Engine, Events, State

engine = Engine(lambda e, b: None)
handler = TestHandler()

class DictState(State):

    def __getitem__(self, item):
        return getattr(self, item)

    def __setitem__(self, key, value):
        setattr(self, key, value)    

    @staticmethod
    def from_state(state):
        dstate = DictState()
        dstate.__dict__ = state.__dict__
        return dstate


class EngineAsDict(UserDict):
    
    def __init__(self, engine):
        super().__init__()
        engine.state = DictState.from_state(engine.state)
        self.data = engine.__dict__


def attach_engine(engine, handler):
    for event in handler.get_events():
        # pass the event as kwarg to handler callback
        engine.add_event_handler(Events[event], handler, data=EngineAsDict(engine), event=event)
        

attach_engine(engine, handler)
engine.run(range(3), max_epochs=2)

> total epochs: 2.
current iteration: 1.
current iteration: 2.
current iteration: 3.
should terminated: False.
current iteration: 4.
current iteration: 5.
current iteration: 6.
should terminated: False.

What do you think ?

PS: It is a bit sad to hear about such decoupling from ignite...

[Nic-Ma]

Hi @vfdev-5 ,

Thanks so much for your detailed suggestions and sample code! I think maybe we can try at own side first.
And I have a small question about:

engine.add_event_handler(Events[event], handler, data=EngineAsDict(engine), event=event)

In this way, I think the handler API should still contain the engine as the first arg?

class Handler:
    def __call__(self, engine: Engine, data: Dict, event: str):

I hope it could be:

class Handler:
    def __call__(self, data: Dict, event: str):

BTW, please don't feel sad about the decoupling handlers, on the opposite, it might be good for ignite.

1. We are trying to expand MONAI users who are using other workflows or their own self-contained workflows now, when they started to use our decoupled handlers, we will recommend our MONAI workflows which are totally based on ignite.
2. And we still have many advanced handlers that are based on ignite handlers, like Checkpoint Loader / Saver, Metrics, etc.
3. If we can attract users by some flexible handlers, finally they may change from other workflows to ignite, otherwise, they don't even want to have a try on MONAI workflows, just using our transforms, networks, losses, etc.
4. I think we need to work closely on the development of the next version MONAI engines, we are exploring the trainers for NLP, semi-supervised training, GAN, etc. for MONAI v0.6.

Thanks.

[vfdev-5]

Hi @Nic-Ma ,

I think the handler API should still contain the engine as the first arg?

This was the case for v0.3.0 and we dropped that as an option since v0.4.0 I think, see here: https://pytorch.org/ignite/engine.html#ignite.engine.engine.Engine.add_event_handler

Actually, my above code works with v0.4.4. So, a handler like you propose

class Handler:
    def __call__(self, data: Dict, event: str):

will work.

Thanks a lot for the clarification on the decoupling decision ! It makes perfect sense and hope we will work closely on those SSL, NLP and GAN trainers.

[Nic-Ma]

Cool, sounds good!
Let me try to refactor handlers for the inference workflows first based on this proposal.
For the first step, we are trying to co-work with some GPU optimized inference projects of NVIDIA, which are currently based on other workflows, like DALI, etc.

Thanks so much for your help!

[Nic-Ma]

Hi @ericspod and @wyli,

Do you guys have any other opinions about this proposal?
If you all agree with it, I think we can try to adjust several existing handlers first, like TransformsInverter, SegmentationSaver, ClassificationSaver, etc. Then co-work with other NVIDIA teams to integrate our handlers into their GPU optimized inference products.

Thanks in advance.

[wyli]

thanks, I feel the utilities such as EngineAsDict could help onboard users for both ignite and monai. (also one of monai's principles is "to be compatible with existing efforts and ease of 3rd party integration for various components.")

the details need more discussions but they'll be ignite usage discussions instead of a feature request here... I'd suggest converting this ticket into a discussion...