Propose ADR 0002: Remove special handling of Optional and Union #154
Conversation
|
Very nice! |
| Sciline builds a data dependency graph based on type hints of callables. | ||
| Some callables may have optional inputs, which are commonly represented by `Optional[T]` in the type hint, for some type `T`. | ||
| Therefore, in [#50](https://github.com/scipp/sciline/pull/50) we have added special handling for `Optional` and [#89](https://github.com/scipp/sciline/pull/89) extended this for `Union`. | ||
| In the case of `Optional`, they way this works is that `sciline.Pipeline` prunes branches at the node where the optional input used, if any ancestor node has unsatisfied dependencies. |
There was a problem hiding this comment.
| In the case of `Optional`, they way this works is that `sciline.Pipeline` prunes branches at the node where the optional input used, if any ancestor node has unsatisfied dependencies. | |
| In the case of `Optional`, the way this works is that `sciline.Pipeline` prunes branches at the node where the optional input is used if any ancestor node has unsatisfied dependencies. |
| There are a couple of possible workarounds: | ||
|
|
||
| - Add an explicit `Optional` provider that wraps (or depends on) the non-optional provider. | ||
| - Modify the graph structure (which we plan to support in the redesign of Sciline) using something like `pipeline[Optional[MyParam]] = pipeline[MyParam]`. |
There was a problem hiding this comment.
3rd option:
Since with the proposed approach, certain T basically always occur as Optional[T], we could adapt the domain types to include Optional directly, e.g.
MyParam = NewType('MyParam', Optional[float])in the case above.
There was a problem hiding this comment.
Isn't that a sub case of the first option (just syntactic sugar)?
There was a problem hiding this comment.
Sort of. It forces the use of Optional[T] everywhere. With point 1, you could still mix Optional[T] and T.
| It makes it tricky to represent the internals of `sciline.Pipeline` as a simple data dependency graph. | ||
| The special handling of `Optional` and `Union` seems to require pervasive changes to the code, which is a sign that it is not a good fit for the design. | ||
|
|
||
| ### Counter arguments |
There was a problem hiding this comment.
Another counter argument: See ess.reduce.nexus.load_sample. The file_path arg supports multiple different types. They cannot be handled via duck typing as they require special handling within the provider. So #153 would not be a solution. So if we didn't support Union, we would need to restrict providers to accept either a single concrete type or an implementation of a protocol as argument.
There was a problem hiding this comment.
Not sure I follow. load_sample would not need to change with the proposal?
There was a problem hiding this comment.
Did I misunderstand it? Are you saying you want to keep support for Union but remove the branch-pruning behaviour of Optional?
There was a problem hiding this comment.
The user can still use Union and Optional, but Sciline would not longer handle them in any special manner. A provider could then return Union[int, float], and another provider could depend on this.
What would be removed is the logic to hook up int or float to Union[int, float].
There was a problem hiding this comment.
And that is exactly why I said it is bad for the example. It would require specifying the parameter as
params = {
Union[FilePath, NeXusFile, NeXusGroup]: Path("...")
}which is unwieldy. The best way to improve this I could think of is
FileSpec = NewType("FileSpec", Union[FilePath, NeXusFile, NeXusGroup])
params = {
FileSpec: Path("...")
}but this is odd because it requires two explicit types (key and value) but those are not the same.
docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md
Outdated
Show resolved
Hide resolved
…d-union.md Co-authored-by: Sunyoung Yoo <luysunyoung9@gmail.com>
Please comment!