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

Resolve remaining design questions for PR #6257 #6504

Open
12 of 32 tasks
atbenmurray opened this issue May 10, 2023 · 2 comments
Open
12 of 32 tasks

Resolve remaining design questions for PR #6257 #6504

atbenmurray opened this issue May 10, 2023 · 2 comments
Assignees
Labels
Design discussions related to the generic API designs

Comments

@atbenmurray
Copy link
Contributor

atbenmurray commented May 10, 2023

This issue is tracking the remaining concerns for #6257 so that they can be resolved and the PR merged.

How to work with this Issue

Please add points of concern as comments to the Issue via comments. I'll curate those into the "To discuss" section.
Each point will be discussed, and a resolution decided by consensus:

  • Needs doing for 1.2
  • Needs doing for post 1.2
  • Doesn't need doing

Those points that we have agreed upon will be detailed below in the TODO section and ticked off as they are done.
My suggestion is that, in order to move an item from discussion to TODO, we need three of the four stakeholders to agree on the item. Similarly, in order to mark an item as satisfactorily done, we need three of the four stakeholders to agree on the item.

To discuss

Do we need to modify reorder: lazy_last_nosync?

Do we want to make changes to this mode? Doing so would invalidate the benchmarks, so ideally we shouldn't, but are there any gotchas in it that we don't want to subsequently support?

  • 1.2
  • 1.3+
  • Not needed

Do we want to drop reorder: lazy_last?

This exists primarily as my first attempt to replicate the dev behaviour. It uses the ApplyPending[d] as a full barrier rather than doing so on a per-key basis, so is slightly easier to reason about. Do we keep it or drop it?

  • Drop for 1.2
  • Keep for 1.2
  • Keep, reconsider for 1.3+
  • Delay decisions until doc review

Compose.__call__ options parameter needs a more meaningful name

I'm still marginal on this one. Does anyone have a better name? execution_options is quite long-winded and I feel that the doc string can properly cover this.

  • options
  • exec_options
  • exec_kwargs
  • lazy_kwargs

Lazy Transform partially_lazy needs a more meaningful name

Design: raised by BM, NM

LazyTransforms such as CropForeground[d], RandCropByLabelClasses[d], RandCropByPosNegLabel[d] need to look at data as part of execution, even if they are executing lazily. As such, they require their inputs to be up to date at the point that they are executed, even if the result of that execution is added to pending_operations rather than applied_operations. There is a property, currently called partially_lazy that is False by default but must be overridden to True for such transforms. For lazy_last_nosync (ie out_of_order lazy execution mode, this flag is ignored, but for in_order lazy execution mode it is honoured.

It could do with a better name.

  • stay with partially_lazy
  • checks_data (BM's choice)
  • should_apply_pending
  • force_apply_pending
  • force_sync
  • None of the above (add suggestion to comments)

Lazy mode could do with more meaningful names

Lazy modes are set through options (which might also be renamed). The current modes are:

  • None - Standard lazy mode (doesn't change pipeline execution order, invertible)
  • "reorder": "lazy_last" - (changes pipeline execution order, invertible)
  • "reorder": "lazy_last_nosync" - (changes pipeline execution order, executes out of order, sometimes invertible)

Are there better names for these modes?

  • Stick with current
  • Change (make comments below)

Revaluate and remove overrides if possible

Design: raised by BM
overrides was added as a way to get around the issues caused by different mode and padding_mode namespaces that various transforms have. The problem is, they break a number of aspects of the design as they must be rewired through to
everywhere that needs them, and were never intended to be part of the design. Although I said OK to them at the time, I hadn't fully realised the issues that they bring about, and we have the opportunity to remove them now before they must be supported or removed going forward.

  • ApplyPending[d] needs to be passed overrides for them to work as originally intended in the design
  • LazyTransforms with "active inputs" (See "Lazy transform active inputs" above) would need to be passed overrides if the transforms themselves are responsible for their own "active inputs" to be up to date

I propose the following:

  1. We track equivalent flags across the different namespaces
  2. When applying pending transforms, the flags can be checked to determine whether they are equivalent
    a. If not, a resample must be carried out
    b. If so, keep applying the pending transforms
  3. At the point that the resample must occur, we convert the flag to the value required by the backend that must be executed.
  • 1.2
  • 1.3+
  • Keep overrides

Break Compose dependency issues in transforms.utils

Design: raised by BM
Compose is referred to in transforms.utils.py, which creates a high potential for cycles. I have tried to resolve this using typing.TYPE_CHECKING but this raises issues when the docs are being built. Ideally, there shouldn't be a dependency between
transform.utils.py to Compose, it moving them isn't the only way to break it:

  1. move the functions in question out of transforms.utils.py
  2. create a ComposeTrait interface that can be referred to instead in the types
  3. remove the type annotation for those functions or simplify it to a Callable
  4. if it is just a doc linting issue, then change the lint settings
  • do 1.
  • do 2.
  • do 3.
  • do 4. (if applicable)

TODO

Rename ComposeCompiler to ExecutionOptions

Design: raised by EK, NM
ExecutionOptions is the proposed name unless anyone can think of anything better

  • Done

ComposeCompiler should not be a poltergeist

Design: raised by EK
Make compose compiler persistent within the lifetime of the Compose object

  • Done

check_applied_ops should come out of is_tensor_invertible

Design: raised by BM
This function is useful in its own right and shouldn't be defined as an inner function in is_tensor_invertible

  • Done

Remove TraceKeys.LAZY from transform_info_keys and get_transform_info and append during track_transform_meta

Bug: raised by BM
Lazy is required as a parameter by track_transform_meta and a number of the functional methods for spatial and crop_pad transforms. Rather than adding it to get_transform_info and then pulling it out of there in all the places it is needed, we can
just append it to the info when we call track_transform_meta.

  • Done

Fix docstring issues

Action: raised by BM, EK, NM
Missing docstrings for some functions in lazy.executors.py

  • Done

Write lazy resampling topic page and link related classes to it

Action: raised by BM
Just as it says. There should be a central lazy resampling topic page, to avoid unnecessary duplication and to not overload Compose with info about lazy resampling

  • Done
@atbenmurray atbenmurray self-assigned this May 10, 2023
@atbenmurray
Copy link
Contributor Author

@ericspod @Nic-Ma @wyli
This issue should now be ready for us to start working on. Please add additional items in the comments below so that we can discuss them here or schedule them for meetings. Thanks!

@KumoLiu KumoLiu added the Design discussions related to the generic API designs label Dec 7, 2023
@atbenmurray
Copy link
Contributor Author

This discussion is effectively complete. #7151 to cover upcoming design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design discussions related to the generic API designs
Projects
None yet
Development

No branches or pull requests

2 participants