Skip to content

revise cachedataset runtime_cache modes#5630

Merged
wyli merged 5 commits intoProject-MONAI:devfrom
wyli:runtime_cache_input
Dec 2, 2022
Merged

revise cachedataset runtime_cache modes#5630
wyli merged 5 commits intoProject-MONAI:devfrom
wyli:runtime_cache_input

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Dec 2, 2022

Fixes #5613

Description

  • runtime_cache=False: the default v1.0.1 behaviour
  • runtime_cache=True or "thread": single process, for caching cuda tensors
  • runtime_cache="process": single process workflow + multiprocess dataloader
  • runtime_cache= user-provided object, could be used to pass a container shared among processes

I feel in this way the user can determine what to use instead of guessing and providing an automated solution... let me know what you think @myron @Nic-Ma, I'm fine if this is eventually merged or not merged

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.

@wyli wyli requested review from Nic-Ma and myron December 2, 2022 11:20
Signed-off-by: Wenqi Li <[email protected]>
@wyli wyli force-pushed the runtime_cache_input branch from 7f19b26 to 8503dfe Compare December 2, 2022 12:03
@wyli wyli marked this pull request as ready for review December 2, 2022 14:12
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Dec 2, 2022

The overall solution looks good to me and it matches the option 2 from @myron in #5615 (comment)

Thanks.

@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Dec 2, 2022

/build

@wyli wyli enabled auto-merge (squash) December 2, 2022 16:52
@wyli wyli force-pushed the runtime_cache_input branch from 9cc7f1e to 15ccae8 Compare December 2, 2022 17:43
@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Dec 2, 2022

/build

@wyli wyli merged commit 025c107 into Project-MONAI:dev Dec 2, 2022
@myron myron mentioned this pull request Dec 2, 2022
7 tasks
@wyli wyli deleted the runtime_cache_input branch December 2, 2022 21:18
@myron
Copy link
Copy Markdown
Collaborator

myron commented Dec 4, 2022

This PR was created and merged without a discussion. Because it was done overnight, It started at 3am PST and merged at 10am PST, it did not give an opportunity for a review or a discussion.
We spent weeks working on an easy shared cache support, #5365, #5308. Most recently we spend over 10 days discussion this small PR #5565 which was heavily scrutinized, and as soon as it was finally merged, the current PR (#5630) removed all of that effort.

@wyli I don't believe it is a good project MONAI practice, you should have a meaningful discussion about it first.

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.

CacheDataset shared memory, during torchrun multi-gpu training

3 participants