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

3402 Add support to set other pickle related args #3412

Merged
merged 14 commits into from
Nov 29, 2021

Conversation

Nic-Ma
Copy link
Contributor

@Nic-Ma Nic-Ma commented Nov 29, 2021

Fixes #3402 .

Description

This PR added support to set other pickle related args for torch.save, especially for big cache content as user requested in #3402 .

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.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

I added unit tests to cover the args, and also verified with the below program to cache big content with pickle_protocol=4:

train_transforms = Compose(
    [
        LoadImaged(keys=["image", "label"]),
        AddChanneld(keys=["image", "label"]),
        Spacingd(keys=["image", "label"], pixdim=(0.5, 0.5, 0.5), mode=("bilinear", "nearest")),
        RepeatChanneld(keys="image", repeats=3),
        EnsureTyped(keys=["image", "label"]),
    ]
)

The cached content size:
image

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

/black

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

/black

monai-bot and others added 2 commits November 29, 2021 04:41
Signed-off-by: monai-bot <monai.miccai2019@gmail.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 3402-add-pickle-args branch from 48f8bb9 to 89c6bd8 Compare November 29, 2021 05:36
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma Nic-Ma force-pushed the 3402-add-pickle-args branch from c4a9ccc to 980ee78 Compare November 29, 2021 05:50
Signed-off-by: Nic Ma <nma@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

Some CI lib may be updated so the flake8 test failed in transform.py:

flake8
4.0.1 (flake8-bugbear: 21.11.28, flake8-comprehensions: 3.7.0,
flake8-executable: 2.1.1, flake8-pyi: 20.10.0, mccabe: 0.6.1, naming: 0.12.1,
pycodestyle: 2.8.0, pyflakes: 2.4.0) CPython 3.8.12 on Linux
/home/runner/work/MONAI/MONAI/monai/transforms/transform.py:207:5: B018 Found useless expression. Either assign it to a variable or remove it.
1     B018 Found useless expression. Either assign it to a variable or remove it.
1
Check failed!

I slightly adjusted the document of Transform class to avoid the flake8 issue.

Thanks.

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

/black

@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

/build

@Nic-Ma Nic-Ma requested review from rijobro, ericspod and wyli November 29, 2021 07:38
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good to me apart from the minor issue.

Signed-off-by: Nic Ma <nma@nvidia.com>
@Nic-Ma
Copy link
Contributor Author

Nic-Ma commented Nov 29, 2021

/build

@Nic-Ma Nic-Ma enabled auto-merge (squash) November 29, 2021 10:25
@wyli
Copy link
Contributor

wyli commented Nov 29, 2021

/build

@Nic-Ma Nic-Ma merged commit 12f74d3 into Project-MONAI:dev Nov 29, 2021
@wyli wyli mentioned this pull request Nov 29, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PersistentDataset Error
3 participants