pytorch / ignite

High-level library to help with training and evaluating neural networks in PyTorch flexibly and transparently.
https://pytorch-ignite.ai
BSD 3-Clause "New" or "Revised" License
4.52k stars 617 forks source link

Add Engine.main() decorator #227

Closed elanmart closed 4 years ago

elanmart commented 6 years ago

I like to structure my code in the following manner, with all Engines and handlers first, followed by all the logic:

    trainer    = Engine()
    validator  = Engine()
    optimizer  = th.optim.Adam(params, lr=lr, amsgrad=True)
    timer      = Timer().attach(trainer)
    trn_loss   = Average()
    val_loss   = Average()

    @trainer.main()
    def step(engine: Engine, batch: Tuple[Tensor, Tensor]):
        ...

    @trainer.on(Events.ITERATION_COMPLETED)
    def print_loss(engine: Engine)
        ...

However currently one has to go with

    validator  = Engine()
    optimizer  = th.optim.Adam(params, lr=lr, amsgrad=True)
    timer      = Timer().attach(trainer)
    trn_loss   = Average()
    val_loss   = Average()

    def step(engine: Engine, batch: Tuple[Tensor, Tensor]):
        ...

    trainer = Engine(step)

    @trainer.on(Events.ITERATION_COMPLETED)
    def print_loss(engine: Engine)
        ...

Could we add the main() or process_function decorator?

vfdev-5 commented 6 years ago

@elanmart nice! 👍 for process_function decorator

vfdev-5 commented 6 years ago

@elanmart what you propose looks cool. And actually, the fact to be able to initialize Engine without process function let me think about the question on Engine design: stateful or stateless.

Today our Engine stores a lot of stuff and I'm not sure about its reusability (there are certainly some bugs with should_terminate etc that remains from previous runs). I think a good design can be something like callable classes (e.g nn.Module):

In this sense, I'm little hesitating on defining Engine without process function and abusing of the decorator to setup the process function multiple times...

I think a clever Engine refactor is needed, firstly to integrate engine checking/resuming and secondly to better define the API.

What do you think ?

alykhantejani commented 6 years ago

Not sure about this, mainly because then we remove/make optional the input to the constructor which could cause confusion as currently the Engine has one purpose (to run a loop and call your process/step function).

Not totally against it, but it makes things a bit more confusing I think

elanmart commented 6 years ago

I see your point. At the same time I feel that the code looks a bit cleaner with the propsed API. Perhaps refactoring the Engine would be a good idea, as @vfdev-5 suggests?

vfdev-5 commented 6 years ago

@elanmart I'm not sure nevertheless that refactoring will allow to have empty contructor and setup process function after

elanmart commented 6 years ago

@vfdev-5 I thought you were thinking about having a Engine that can be run mulitple times for multiple functions? In that case the process_function would be an argument to run, which would also be quite nice. Or I misunderstood your proposition?

vfdev-5 commented 6 years ago

@elanmart I was thinking about something like nn.Module:

model = nn.Sequential(
     # permanent stuff
)

# multiple execution on various input 
model(data1)
model(data2)

which is in parallel to a "proposed" vision of Engine

# permanent stuff
engine = Engine(process_function, checkpoint_options, ...)
engine.add_event_handler(ev1, h1)
engine.add_event_handler(ev2, h2)

# multiple execution on various input
engine.run(data1, 10)
engine.run(data2, 20)

# resume execution
engine.resume(data3, checkpoint_dir)
elanmart commented 6 years ago

Thanks for the snippet. Yes, that makes sense.

So, do you think we could still somehow separate the setup & logic? Maybe we could make the constructor arg mandatory, but allow explicitly passing None?

vfdev-5 commented 4 years ago

I close this issue as stale, but we'll keep it in mind for future if we opt to a more flexible design of the Engine.