Skip to content
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

Add gradient accumulation logic to SupervisedTrainer #6101

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

jak0bw
Copy link
Contributor

@jak0bw jak0bw commented Mar 3, 2023

#6100

Description

A few sentences describing the changes proposed in this pull request.

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 --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@jak0bw
Copy link
Contributor Author

jak0bw commented Mar 3, 2023

(Source code is strongly (and shamelessly) influenced by https://pytorch.org/ignite/generated/ignite.engine.supervised_training_step.html

@jak0bw jak0bw force-pushed the add-gradient-accumulation-to-supervised-trainer branch from 6bc4652 to de2884a Compare March 3, 2023 19:14
@jak0bw
Copy link
Contributor Author

jak0bw commented Mar 3, 2023

Needs Feedback if this feature is desired and if yes probably test(s) for the new parameter.

@jak0bw jak0bw force-pushed the add-gradient-accumulation-to-supervised-trainer branch from 3c25c7c to 68177ac Compare March 3, 2023 19:36
@jak0bw jak0bw marked this pull request as draft March 4, 2023 12:37
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Mar 6, 2023

Hi @jak0bw ,

Thanks for your idea and contribution here.
The design of SupervisedTrainer follows the ignite engine logic, it just defines the standard (or default) computation logic for every iteration. If you have any customized logic, please pass it as a callback function:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/engines/trainer.py#L101

Thanks.

@jak0bw
Copy link
Contributor Author

jak0bw commented Mar 6, 2023

Hi @Nic-Ma,

thank you for your answer.

I am sorry if I misunderstand something critical here but doesn't the standard ignite logic include gradient accumulation similarly to my proposed changes (since ignite 0.4.7)? (My code is more or less copy pasted from the ignite source code) Therefore a change in the way outlined as in this pr would just restore feature parity between ignite and monai and not be considered customized logic.

Link to ignite create_supervised_trainer: https://github.com/pytorch/ignite/blob/c7c0df0fbfdff2a86415476cf0e68f36a089c1d2/ignite/engine/__init__.py#L404

Link to the used step function(s):
https://github.com/pytorch/ignite/blob/c7c0df0fbfdff2a86415476cf0e68f36a089c1d2/ignite/engine/__init__.py#L44

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Mar 6, 2023

Hi @jak0bw ,

Oh, I didn't notice this new option in ignite.
@vfdev-5 @wyli Do you think it's necessary to add it in MONAI trainer?

Thanks in advance.

@wyli
Copy link
Contributor

wyli commented Mar 6, 2023

I think if ignite's supervised_training_step is not directly usable, we should create a util function in monai.engine.utils:

def grad_accumulation_iteration(steps=...):
    def iteration(engine, ...):
        ...
        return engine.output
    return iteration

and the usage would be

monai.engine.SupervisedTrainer(..., iteration_update=monai.engine.utils.grad_accumulation_iteration(steps), ...)

@jak0bw
Copy link
Contributor Author

jak0bw commented Mar 6, 2023

As mentioned in #6100 it is possible to directly use Ignite's supervised_training_step but as it does not emit the same Events as the monai step function some monai handlers using these Events are not triggered correctly.

@wyli
Copy link
Contributor

wyli commented Mar 6, 2023

sure, please consider creating a function in monai.engines.utils
and the engine will prefer this iteration_update if it's provided:

if iteration_update is not None:
super().__init__(iteration_update)
else:
super().__init__(self._iteration)

this is how we create various iteration_update functions, for example: https://github.com/Project-MONAI/MONAI/blob/dev/monai/apps/deepedit/interaction.py#LL26C7-L26C18

usage:
https://github.com/Project-MONAI/tutorials/blob/aa4ca78d3e7f08c6d8f5a5a009d5da508acdb6ad/deepedit/ignite/train.py#L250C37-L259

@jak0bw jak0bw force-pushed the add-gradient-accumulation-to-supervised-trainer branch from 5538ec7 to da515e3 Compare March 6, 2023 19:05
@jak0bw jak0bw force-pushed the add-gradient-accumulation-to-supervised-trainer branch 3 times, most recently from d48d18d to 3301889 Compare March 20, 2023 14:25
@jak0bw jak0bw force-pushed the add-gradient-accumulation-to-supervised-trainer branch 6 times, most recently from 303a1a8 to 1d8415e Compare March 28, 2023 14:01
@jak0bw jak0bw force-pushed the add-gradient-accumulation-to-supervised-trainer branch from dfe24ef to fda5cb6 Compare March 28, 2023 14:09
@jak0bw
Copy link
Contributor Author

jak0bw commented Mar 28, 2023

@wyli I think I added a custom iteration update function as requested. The code still has a circular import error (the circular import of SupervisedTrainer) which I don't know how to resolve really as it kinda depends on how the monai project is structured (or deals with these problems in code).

Additionally tests are still missing.

@Nic-Ma Unfortunately, I don't really have time to further work on this pull request in the near future. Therefore this pull request (and/or the corresponding issue) can be marked for contribution wanted, closed or however the monai team wants to deal with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants