Skip to content

Use Ignite's interrupt api in MonaiAlgo#5071

Merged
wyli merged 12 commits intoProject-MONAI:devfrom
holgerroth:4554-use-interrupt-api
Sep 6, 2022
Merged

Use Ignite's interrupt api in MonaiAlgo#5071
wyli merged 12 commits intoProject-MONAI:devfrom
holgerroth:4554-use-interrupt-api

Conversation

@holgerroth
Copy link
Copy Markdown
Collaborator

@holgerroth holgerroth commented Sep 1, 2022 •
edited
Loading

Fixes #4554.

Requires latest Ignite RC.

Also improves ClientAlgo docstrings formatting.

Description

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

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

@holgerroth holgerroth requested review from Nic-Ma and wyli September 1, 2022 23:42
@Nic-Ma Nic-Ma requested a review from vfdev-5 September 2, 2022 07:45
Nic-Ma
Nic-Ma reviewed Sep 2, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 2, 2022 •
edited
Loading

Hi @holgerroth @wyli ,

I think we should update the min_version of ignite in the monai_algo.py file to 0.4.10:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/fl/client/monai_algo.py#L41
Other places keep the original min version.
Then when ignite released 0.4.10 next Monday, let's update the ignite version in all the docker and CI configs.
Now the min required version is 0.4.4 for all the places:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/config/deviceconfig.py#L261
And the docker and CI version is 0.4.9:
https://github.com/Project-MONAI/MONAI/blob/dev/requirements-dev.txt#L3

What do you think?

Thanks.

@holgerroth
remove ignite import
939decb 
Signed-off-by: Holger Roth <[email protected]>
@holgerroth
Copy link
Copy Markdown
Collaborator Author

holgerroth commented Sep 2, 2022

Hi @holgerroth @wyli ,

I think we should update the min_version of ignite in the monai_algo.py file to 0.4.10: https://github.com/Project-MONAI/MONAI/blob/dev/monai/fl/client/monai_algo.py#L41 Other places keep the original min version. Then when ignite released 0.4.10 next Monday, let's update the ignite version in all the docker and CI configs. Now the min required version is 0.4.4 for all the places: https://github.com/Project-MONAI/MONAI/blob/dev/monai/config/deviceconfig.py#L261 And the docker and CI version is 0.4.9: https://github.com/Project-MONAI/MONAI/blob/dev/requirements-dev.txt#L3

What do you think?

Thanks.

Actually, we don't need to import Ignite anymore in this class. I removed it.

@holgerroth
add typing
bee31e3 
Signed-off-by: Holger Roth <[email protected]>
@vfdev-5
Copy link
Copy Markdown
Member

vfdev-5 commented Sep 5, 2022

FYI, we just released 0.4.10, so you can check and also update MONAI deps.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 5, 2022

FYI, we just released 0.4.10, so you can check and also update MONAI deps.

Cool!
@holgerroth Can I update the ignite related logic in your PR directly?

Thanks.

Nic-Ma
Nic-Ma approved these changes Sep 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the ignite dependency in the PR.

Thanks.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 5, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 5, 2022

/build

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 5, 2022

Hi @wyli ,

Could you please help double confirm it?

Thanks in advance.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 5, 2022

And @YanxuanLiu , may need your help to update the ignite version in the Blossom CI config to 0.4.10.

Thanks in advance.

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 6, 2022

/build

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Sep 6, 2022

Hi @wyli ,

Could you please help double confirm it?

Thanks in advance.

I see there are docs-test errors: https://github.com/Project-MONAI/MONAI/runs/8197608122?check_suite_focus=true

vfdev-5
vfdev-5 reviewed Sep 6, 2022
View reviewed changes
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 6, 2022

/black

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Sep 6, 2022

/build

@wyli wyli enabled auto-merge (squash) September 6, 2022 09:07
@wyli wyli merged commit b6016a9 into Project-MONAI:dev Sep 6, 2022
wyli pushed a commit to yashika-git/MONAI that referenced this pull request Sep 6, 2022
@holgerroth @Nic-Ma
@pre-commit-ci @monai-bot @wyli
Use Ignite's interrupt api in MonaiAlgo (Project-MONAI#5071)
2dc9a67 
Fixes Project-MONAI#4554. 

Requires latest Ignite RC.

Also improves ClientAlgo docstrings formatting.

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

### Status
**Ready**

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not applicable items -->
- [x] 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.

Signed-off-by: Holger Roth <[email protected]>
Signed-off-by: Nic Ma <[email protected]>
Signed-off-by: monai-bot <[email protected]>
Co-authored-by: Nic Ma <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: monai-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vfdev-5 vfdev-5 vfdev-5 left review comments

@Nic-Ma Nic-Ma Nic-Ma approved these changes

@wyli wyli Awaiting requested review from wyli

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Add MONAI FL client components [P0 v1.0]

Development

Successfully merging this pull request may close these issues.

Add interrupt/terminate option to Trainers & Evaluators

5 participants

@holgerroth @Nic-Ma @vfdev-5 @wyli @monai-bot