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

Lazy eval refactor #6257

Closed
wants to merge 141 commits into from
Closed

Conversation

atbenmurray
Copy link
Contributor

@atbenmurray atbenmurray commented Mar 30, 2023

Lazy evaluation refactor

This PR contains a reworking of the lazy evaluation mechanism on dev to provide the following:

  1. There are now three lazy execution options:
    1. the options=None (default) mode guarantees that all pending operations are evaluated before evaluating non-lazy _apply_transforms
      or transforms with lazy set to False. This means that all pipelines should be able to execute lazily without breaking (the first thing that users will try)
    2. there is an options={'reorder': 'lazy_last'} mode that moves all lazy operations past non-lazy ones, (bounded by use of
      ApplyPending/ApplyPendingd) and guarantees invertibility
    3. there is an options={'reorder': 'lazy_last_nosync'} mode that relies on ApplyPending/ApplyPendingd to cause application of
      pending transforms, and precisely mimics the behaviour of the lazy execution on dev.
  2. Lazy now has three states on Compose:
    1. lazy=True overrides the lazy flag on lazy transforms, making them all execute lazily
    2. lazy=None uses the lazy flags set on transforms, and can execute them lazily or non-lazily depending
    3. lazy=False (default) causes lazy processing to be disabled and all lazy transforms will execute non-lazily
  3. Lazy transforms have a lazy override that they can optionally set during __call__. This means that lazy behaviour can be overridden without mutating the transform instance
  4. Logging has been fixed so that the first transform is included in the logging messages. Logging activities are clearer and contain addition important information about lazy modes
  5. ApplyPending and ApplyPendingd replace the use of Identity and Identityd for causing pending ops to be evaluated when using the lazy_last[_nosync] options
  6. Compose API changes:
    1. overrides and override_keys have been consolidated into overrides
      1. on dev: override_keys=('a', 'b'), overrides={'mode': ('bilinear', None), 'padding_mode: (None, 'zeros')'}
      2. on PR: overrides={'a': {'mode': 'bilinear'}, 'b': {'padding_mode': 'zeros'}}
    2. log_stats and verbose have been replaced with logger_name, consistent with other parts of MONAI
  7. additional lazy resampling documentation has / is being added
  8. additional lazy tests have been added

Types of changes

  • Breaking changes:
    • lazy=True/None and options=None is new functionality relative to dev
    • lazy=True/None and options={"reorder": "lazy_last"} is new functionality relative to dev
    • lazy=None and setting lazy=True on a transform is new functionality relative to dev
    • Identity[d] doesn't cause pending operations to be evaluated; they must be replaced with ApplyPending[d]
  • Non-breaking changes:
    • lazy=True and options={"reorder": "lazy_last_nosync"} (with Identity[d]being replaced byApplyPending[d]) should be identical in behaviour to dev`
  • [wip] 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.
  • [wip] Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
…arameter, refactoring spatial arrays to support lazy_evaluation call time parameter, refactoring _apply_transform to pass call time lazy_evaluation flag

Signed-off-by: Ben Murray <ben.murray@gmail.com>
… transforms

Signed-off-by: Ben Murray <ben.murray@gmail.com>
lazy_evaluation initing / calls

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
croppad/array

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
meaningful
. Adding ApplyPending and ApplyPendingd transforms

Signed-off-by: Ben Murray <ben.murray@gmail.com>
respectively

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Filter for metatensors in ApplyPending / ApplyPendingd classes

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
@wyli
Copy link
Contributor

wyli commented Apr 6, 2023

hi @atbenmurray sorry, earlier I didn't notice this PR is pending review. please address the merging conflicts I'll test and merge it. ApplyPending is better than implicit resampling calls in compose, and allows for flexible overriding.

Signed-off-by: Ben Murray <ben.murray@gmail.com>
@atbenmurray
Copy link
Contributor Author

Gone through the merge from dev. I still need to test things before we merge!

pre-commit-ci bot and others added 13 commits April 14, 2023 16:56
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
…pply_pending. Fixing various tests by moving to use of 'overrides' on apply_pending

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
return data


class ComposeCompiler:
Copy link
Member

Choose a reason for hiding this comment

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

As we've discussed today at the meeting I'd suggest something like LazyPolicy given the current state of this class. If the intent is to evolve this to do more complicated work to "compile" in some manner we might want to put that into a new class anyway and keep something to do policy like this separate.

Comment on lines +605 to +610
if invertible is False:
if reasons is not None:
reason_text = "\n".join(reasons)
raise RuntimeError(f"Unable to run inverse on 'data' for the following reasons:\n{reason_text}")
else:
raise RuntimeError("Unable to run inverse on 'data'; no reason logged in trace data")
Copy link
Member

@ericspod ericspod May 5, 2023

Choose a reason for hiding this comment

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

Suggested change
if invertible is False:
if reasons is not None:
reason_text = "\n".join(reasons)
raise RuntimeError(f"Unable to run inverse on 'data' for the following reasons:\n{reason_text}")
else:
raise RuntimeError("Unable to run inverse on 'data'; no reason logged in trace data")
if invertible is False:
reason_text = "No reason logged in trace data" if reasons is None else "\n".join([""]+list(reasons))
raise RuntimeError(f"Unable to run inverse on 'data': {reason_text}")

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 5, 2023

/build

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
atbenmurray and others added 9 commits May 10, 2023 12:12
Cleaner tensor ops for test_compose

Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
…vertible. Removed LAZY from default transform_info(). It is now added during 'track_transform_meta'

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: Ben Murray <ben.murray@gmail.com>
…ing out of order execution for brevity

Signed-off-by: Ben Murray <ben.murray@gmail.com>

def __init__(self):
# construct the list of options
options = {"reorder": {"lazy_last": self.reorder_lazy_last, "lazy_last_nosync": self.reorder_lazy_last_nosync}}
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid having nested structures like this, instead:

Suggested change
options = {"reorder": {"lazy_last": self.reorder_lazy_last, "lazy_last_nosync": self.reorder_lazy_last_nosync}}
options = {("reorder","lazy_last"): self.reorder_lazy_last, ("reorder","lazy_last_nosync"): self.reorder_lazy_last_nosync}

Also the string keywords here should be in enums and the structure that's expected should be documented in the docstring.

options = {"reorder": {"lazy_last": self.reorder_lazy_last, "lazy_last_nosync": self.reorder_lazy_last_nosync}}
self.options = options

def __call__(self, transforms, lazy: bool | None, options: dict | None = None):
Copy link
Member

Choose a reason for hiding this comment

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

options is only ever used with one key-value pair so maybe just have two variables here. Otherwise eval_options or something, with more docstring on what it means.

if len(options.keys()) > 1:
raise ValueError("Only one option can currently be set")

for k, v in options.items():
Copy link
Member

Choose a reason for hiding this comment

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

k, v = monai.utils.first(options.items())?

t.lazy_evaluation = self.lazy_evaluation
self.options = options
self.logger_name = logger_name
self.execution_options = ExecutionOptions()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.execution_options = ExecutionOptions()
self.exec_options = ExecutionOptions() if exec_options is None else exec_options

monai/transforms/compose.py Show resolved Hide resolved
Args:
enabled: True if the transform should operate in a lazy fashion, False if not.
"""
raise NotImplementedError()

@property
def partially_lazy(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def partially_lazy(self):
def partially_lazy(self) -> bool:

@atbenmurray atbenmurray mentioned this pull request May 22, 2023
7 tasks
Nic-Ma added a commit that referenced this pull request Jun 1, 2023
Reduced lazy resampling functionality for MONAI 1.2

### Description

This PR is a subset of #6257 intended for MONAI 1.2. It contains the
basic resampling strategy that has been approved for the 1.2 release
during MONAI core dev meeting of 19th May, 2023.

Draft status:
 * still to do
   * doc strings
   * topic page
   * resolve compose reference doc issue

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

---------

Signed-off-by: Ben Murray <ben.murray@gmail.com>
Signed-off-by: monai-bot <monai.miccai2019@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Co-authored-by: Nic Ma <nma@nvidia.com>
Co-authored-by: monai-bot <monai.miccai2019@gmail.com>
@atbenmurray
Copy link
Contributor Author

This is out of date. We'll revisit further lazy resampling functionality in new PRs. See #7151 for details

@atbenmurray atbenmurray closed this Feb 9, 2024
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.

6 participants