2474 Deprecate SegmentationSaver and TransformInverter handlers #2475

Nic-Ma · 2021-06-29T03:12:09Z

Fixes #2474 .

Description

This PR added deprecated decorator to SegmentationSaver and TransformInverter handlers as we already have better options.

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

merge master

Signed-off-by: Nic Ma <nma@nvidia.com>

Nic-Ma · 2021-06-29T03:12:19Z

/black

Signed-off-by: monai-bot <monai.miccai2019@gmail.com>

Nic-Ma · 2021-06-29T03:22:56Z

Hi @ericspod ,

Thanks for developing the deprecated API, I tried to use it in this PR.
But I got a minor issue that I can't see the warning message when call the deprecated function in multi-processing case. Something wrong with my env settings?
Maybe you can help verify locally?
You can download this PR and just run below program in a PyTorch distributed program:

from monai.handlers.utils import evenly_divisible_all_gather
        print(evenly_divisible_all_gather(["tests"]))

It can print out the warning message if running in a regular single process program.

Thanks.

ericspod · 2021-06-29T11:39:17Z

I think you're using incorrect arguments unless the ones you provided were for testing. The version_val argument is for the current version of MONAI, you'd provide a value to this during testing so that comparisons can be made against a chosen version that doesn't change. Leaving that argument alone will use the current MONAI version which is what you want I think. The removed argument is for specifying which version the definition will be removed in, that is what I think you want to be "0.7.0".

Also for the msg_suffix value, this should be a full correct English sentence since it will be appended after a full stop. Unless the first word is a proper name/term that doesn't use normal capitalization rules, the sentence should begin with a capital letter.

For the multiprocessing this relates to the mechanism I added to ensure the warning is show only once. A global dictionary stores definitions that were called/instantiated and have printed the warning, on subsequent calls/instantiations the warning is skipped. This won't necessarily work across processes if this dictionary wasn't filled in before spawning subprocesses, so each process will start with an empty dictionary and each should issue a warning. This will always happen in Windows which lacks fork semantics. If the stdout/stderr for subprocesses is suppressed or routed elsewhere you will never see the warning.

However looking at the tests, for evenly_divisible_all_gather for example, I don't see how this would impact the lack of message. I think it might be a combination of factors between the value for the version_val, multiprocessing, and the fact that if you test a function multiple times you'd only get the warning on the first test so subsequent ones will fail if checking a warning was printed. The version_val argument can only be set when the decorator is applied which means when you try to test decorated functions the result you get will change as the version of MONAI increments. Perhaps these should changed to expect an exception raise when the version of MONAI is past the removed version (ie. anything at or above 0.7.0).

Nic-Ma · 2021-06-29T13:10:29Z

Hi @ericspod ,

Thanks for your detailed explanation! May I ask what the since version I should set in the dev branch now?
Actually, it will only be marked as deprecated since v0.6.0, but as we haven't released v0.6.0, if I use 0.6.0 now, it will raise an exception instead of a deprecated warning.

Thanks.

Signed-off-by: Nic Ma <nma@nvidia.com>

ericspod · 2021-06-29T13:34:31Z

If you use a since value in the future it should do nothing. I think I might have missed a test for this case however.

Nic-Ma · 2021-06-29T13:46:42Z

If you use a since value in the future it should do nothing. I think I might have missed a test for this case however.

I have updated the PR according to your comments to use the removed arg and capital word.
What do you think we should do next? You will update the since or something?

Thanks.

wyli · 2021-06-29T14:08:24Z

Just a reminder that I pushed the tag 0.6.0.rc1 to the repo. So if you fetch the tags to your local copy, the dev version will be 0.6.0.rc1+commits instead of the previous 0.5.3+commits, this might change the deprecation message results

Nic-Ma · 2021-06-29T14:16:17Z

Hi @wyli ,

Thanks for your help! I restarted the CI tests, let's see whether it can pass now as you pushed a new tag.
And I am wondering that maybe we need to change the deprecated decorator to avoid raising exception if the current version is lower than the since version, just don't print warning. @ericspod what do you think?

Thanks.

ericspod · 2021-06-29T14:18:01Z

And I am wondering that maybe we need to change the deprecated decorator to avoid raising exception if the current version is lower than the since version

This should already be the behaviour.

The since value is fine and is set to the version at which the definition became deprecated, but you should add removed set to the version it'll be removed at (0.7.0). As I said

Nic-Ma · 2021-06-29T14:25:18Z

Hi @ericspod ,

Thanks for your suggestions! Maybe I misunderstood your points. So is it correct in the PR now?

@deprecated(since="0.6.0", removed="0.7.0", msg_suffix="XXX")

Nic-Ma · 2021-06-29T14:29:04Z

Seems the CI test failed due to below error:

  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/monai/networks/nets/torchvision_fc.py", line 71, in <module>
    @deprecated(since="0.6.0", removed="0.7.0", msg_suffix="Please consider using `TorchVisionFCModel` instead.")
  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/monai/utils/deprecated.py", line 87, in deprecated
    is_deprecated = since is not None and version_leq(since, version_val)
  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/monai/utils/deprecated.py", line 61, in version_leq
    return l < r
TypeError: '<' not supported between instances of 'int' and 'str'

Thanks.

ericspod · 2021-06-29T14:32:12Z

Hi @ericspod ,

Thanks for your suggestions! Maybe I misunderstood your points. So is it correct in the PR now?
@deprecated(since="0.6.0", removed="0.7.0", msg_suffix="XXX")

Yes that should be right.

ericspod · 2021-06-29T14:37:06Z

Seems the CI test failed due to below error:

  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/monai/networks/nets/torchvision_fc.py", line 71, in <module>
    @deprecated(since="0.6.0", removed="0.7.0", msg_suffix="Please consider using `TorchVisionFCModel` instead.")
  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/monai/utils/deprecated.py", line 87, in deprecated
    is_deprecated = since is not None and version_leq(since, version_val)
  File "/opt/hostedtoolcache/Python/3.8.10/x64/lib/python3.8/site-packages/monai/utils/deprecated.py", line 61, in version_leq
    return l < r
TypeError: '<' not supported between instances of 'int' and 'str'

Thanks.

It doesn't like versions of the form "0.6.0rc0" because "0rc0" part won't parse. I had assumed that versions would always be three numbers separated by "." followed optionally by "." and other text that's ignored. A version of the form "0.6.0.rc0" would work otherwise we can figure out how to split the version string by "rc#" if it occurs.

Nic-Ma · 2021-06-29T14:40:08Z

Hi @ericspod ,

Maybe pkg_resource.parse_version can support it?
https://github.com/Project-MONAI/MONAI/blob/dev/tests/utils.py#L117

Thanks.

ericspod · 2021-06-29T14:52:08Z

Hi @ericspod ,

Maybe pkg_resource.parse_version can support it?
https://github.com/Project-MONAI/MONAI/blob/dev/tests/utils.py#L117

Thanks.

I hadn't want to rely on pkg_resource since it isn't a base dependency. I'm working on a fix right now though and I'l have a PR shortly.

Nic-Ma · 2021-06-29T14:53:14Z

Cool! Thanks for your quick update!
CC @wyli .

Thanks.

monai/networks/nets/torchvision_fc.py

ericspod · 2021-06-29T19:10:44Z

PR #2482 should sort the issue now.

Nic-Ma · 2021-06-29T22:30:15Z

Hi @ericspod ,

Cool! Thanks for your quick update.
I merged master, could you please help review this PR again?

Thanks.

ericspod

I'm good with it if @wyli is too.

wyli

thanks, perhaps you could also add some docstring for things that were not clear to you?

Signed-off-by: Nic Ma <nma@nvidia.com>

Nic-Ma · 2021-06-30T03:52:05Z

Sure, enhanced the doc-strings to make it more clear.

Thanks.

Nic-Ma · 2021-06-30T03:52:13Z

/black

Nic-Ma · 2021-06-30T05:38:23Z

Hi @ericspod @wyli ,

Do you know why the GPU tests failed? I tried to run several times.

Thanks in advance.

wyli · 2021-06-30T08:32:25Z

looks like it's related to version comparison, I'm looking into this

Nic-Ma · 2021-06-30T08:34:24Z

looks like it's related to version comparison, I'm looking into this

Can your PR #2485 fix it?
If yes, we can wait until merging your PR first.

Thanks.

wyli · 2021-06-30T10:19:04Z

looks like it's related to version comparison, I'm looking into this

Can your PR #2485 fix it?
If yes, we can wait until merging your PR first.

Thanks.

yes, tested #2485, it is good now. I didn't address #2485 (comment) but I think it's good enough for v0.6..

Nic-Ma and others added 4 commits February 1, 2021 19:15

Merge pull request #19 from Project-MONAI/master

42a45e0

merge master

Merge pull request #32 from Project-MONAI/master

cd16a13

merge master

Merge pull request #152 from Project-MONAI/dev

793ee6e

merge master

[DLMED] add deprecated decorator

e29a19e

Signed-off-by: Nic Ma <nma@nvidia.com>

Nic-Ma requested review from ericspod, rijobro and wyli June 29, 2021 03:12

[MONAI] python code formatting

54ada8c

Signed-off-by: monai-bot <monai.miccai2019@gmail.com>

Merge branch 'dev' into 2474-deprecate-handlers

4dbd62d

[DLMED] update according to comments

61a2a49

Signed-off-by: Nic Ma <nma@nvidia.com>

Merge branch 'dev' into 2474-deprecate-handlers

0ac7d9c

wyli reviewed Jun 29, 2021

View reviewed changes

monai/networks/nets/torchvision_fc.py Show resolved Hide resolved

Merge branch 'dev' into 2474-deprecate-handlers

eed9100

ericspod approved these changes Jun 29, 2021

View reviewed changes

wyli approved these changes Jun 29, 2021

View reviewed changes

[DLMED] enhance doc-string

67f9e52

Signed-off-by: Nic Ma <nma@nvidia.com>

Nic-Ma enabled auto-merge (squash) June 30, 2021 03:52

Merge branch 'dev' into 2474-deprecate-handlers

94dcf02

Nic-Ma merged commit 304a357 into Project-MONAI:dev Jun 30, 2021

Nic-Ma deleted the 2474-deprecate-handlers branch July 2, 2021 23:36

2474 Deprecate SegmentationSaver and TransformInverter handlers #2475

2474 Deprecate SegmentationSaver and TransformInverter handlers #2475

Uh oh!

Conversation

Nic-Ma commented Jun 29, 2021

Description

Status

Types of changes

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

wyli commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

Uh oh!

ericspod commented Jun 29, 2021

Uh oh!

Nic-Ma commented Jun 29, 2021

Uh oh!

ericspod left a comment

Choose a reason for hiding this comment

Uh oh!

wyli left a comment

Choose a reason for hiding this comment

Uh oh!

Nic-Ma commented Jun 30, 2021

Uh oh!

Nic-Ma commented Jun 30, 2021

Uh oh!

Nic-Ma commented Jun 30, 2021

Uh oh!

wyli commented Jun 30, 2021

Uh oh!

Nic-Ma commented Jun 30, 2021

Uh oh!

wyli commented Jun 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants