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.

Sign up for GitHub

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

Jump to bottom

4148 Enhance engine iteration logic and the typehints #4150

Merged
merged 14 commits into from
Apr 20, 2022
Merged

4148 Enhance engine iteration logic and the typehints #4150

merged 14 commits into from
Apr 20, 2022

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Apr 20, 2022 •
edited
Loading

Fixes #4148 .

Description

This PR enhanced the engine iteration logic and the typehints.

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.

Nic-Ma and others added 12 commits February 1, 2021 19:15
@Nic-Ma
Merge pull request #19 from Project-MONAI/master
42a45e0 
merge master
@Nic-Ma
Merge pull request #32 from Project-MONAI/master
cd16a13 
merge master
@Nic-Ma
Merge pull request #180 from Project-MONAI/dev
6f87afd 
merge master
@Nic-Ma
Merge pull request #214 from Project-MONAI/dev
f398298 
merge master
@Nic-Ma
Merge pull request #397 from Project-MONAI/dev
ec463d6 
merge master
@Nic-Ma
Merge pull request #404 from Project-MONAI/dev
c3ae03a 
merge master
@Nic-Ma
[DLMED] update Workflow.py
7e253e9 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
[DLMED] update all the engines
33bde29 
Signed-off-by: Nic Ma <[email protected]>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
a13117c 
for more information, see https://pre-commit.ci
@Nic-Ma
[DLMED] fix flake8
5a95621 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Merge branch '4148-update-engines' of https://github.com/Nic-Ma/MONAI …
379939e 
…into 4148-update-engines
@Nic-Ma
Merge branch 'dev' into 4148-update-engines
af7c297
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

/build

@Nic-Ma
[DLMED] fix flake8
3721648 
Signed-off-by: Nic Ma <[email protected]>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

/build

@Nic-Ma Nic-Ma changed the title [WIP] 4148 Enhance engine iteration logic and the typehints 4148 Enhance engine iteration logic and the typehints Apr 20, 2022
@Nic-Ma Nic-Ma marked this pull request as ready for review April 20, 2022 16:03
@Nic-Ma Nic-Ma requested review from ericspod and rijobro April 20, 2022 16:04
@rijobro
Copy link
Contributor

rijobro commented Apr 20, 2022

Is the future annotations necessary or purely aesthetic? I would leave it if it's the latter. Even though I find a | None more intuitive than Optional[a], we probably don't want to be doing this piecemeal in the codebase.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

Hi @ericspod ,

I updated the engines in this PR, and I checked all the handlers don't have dependency on Workflow, so keep them with ignite Engine.
Seems pre-commit-ci also automatically changed several typehints, I saw @rijobro also used similar thing in MetaTensor:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/meta_tensor.py
Maybe this is the right way to go? We may need to automatically update all the typehints in the codebase with the pre-commit-ci tool later (I don't know how to trigger it..).
Could you please help the PR?

Thanks in advance.

@rijobro
Copy link
Contributor

rijobro commented Apr 20, 2022 •
edited
Loading

So you imported from __future__ import annotations so that you could use SupervisedEvaluator as type hint for engine in _iteration?

And then from there, the pre-commit-ci made the other type hint changes (a | None)? If that's the case, this is all OK with me.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

So you imported from __future__ import annotations so that you could use SupervisedEvaluator as type hint for engine in _iteration?

And then from there, the pre-commit-ci made the other type hint changes (a | None)? If that's the case, this is all OK with me.

Hi @rijobro ,

Yes, I imported the annotations to make current class is available for typehints.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

/build

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

Hi @rijobro ,

The initial discussion about this PR is here: #4132 (comment).

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Apr 20, 2022

I executed the integration test test_integration_workflows.py locally and it passed OK.

Thanks.

@Nic-Ma Nic-Ma enabled auto-merge (squash) April 20, 2022 16:51
@ericspod
Copy link
Member

I'm good with the changes, this way of defining optional values should be compatible with the versions of Python we're supporting now.

@Nic-Ma Nic-Ma merged commit ed1c281 into Project-MONAI:dev Apr 20, 2022
Can-Zhao pushed a commit to Can-Zhao/MONAI that referenced this pull request May 10, 2022
@Nic-Ma @pre-commit-ci @Can-Zhao
4148 Enhance engine iteration logic and the typehints (Project-MONAI#…
ce98f47 
…4150)

* [DLMED] update Workflow.py

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] update all the engines

Signed-off-by: Nic Ma <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* [DLMED] fix flake8

Signed-off-by: Nic Ma <[email protected]>

* [DLMED] fix flake8

Signed-off-by: Nic Ma <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@Shadow-Devil Shadow-Devil mentioned this pull request Jan 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericspod ericspod ericspod approved these changes

@rijobro rijobro rijobro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update engines according to discussion
3 participants
@Nic-Ma @rijobro @ericspod