From 03bb5064136118ecbb842e6aa0f47c95ed41a758 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Thu, 11 Apr 2024 08:58:49 +0200 Subject: [PATCH 01/13] Add ADR 0001: Remove isinstance checks when setting parameters --- ...instance-checks-when-setting-parameters.md | 43 +++++++++++++++++++ .../architecture-decision-records.md | 10 +++++ docs/developer/index.md | 1 + 3 files changed, 54 insertions(+) create mode 100644 docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md create mode 100644 docs/developer/architecture-decision-records.md diff --git a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md new file mode 100644 index 00000000..d3bad07c --- /dev/null +++ b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md @@ -0,0 +1,43 @@ +# ADR 0001: Remove isinstance checks when setting parameters + +- Status: proposed +- Deciders: +- Date: 2024-04-11 + +## Context + +Sciline builds a data dependency graph based on type hints of callables. +Dependencies can be fulfilled by setting values (instances of classes) as so called *parameters*. +In an attempt to extend the correctness guarantees of the dependency graph, Sciline's `__setitem__` checks if the value is instance of the key (a type) when setting a parameter. + +This has led to a number of problems. +For example, supporting different file handles types is too difficult [#140](https://github.com/scipp/sciline/issues/140), +parameter type handling is too inflexible in general [#144](https://github.com/scipp/sciline/issues/144), +and the mechanism is broken with Python 3.12 type aliases [#145](https://github.com/scipp/sciline/issues/145). +In short, the mechanism gets in the way of the user, since it causes false positives. + +Considering the bigger picture, we can think of this mechanism as a poor man's form of *validation*. +Validation of input parameters is very important when running workflows, but it should be done in a more explicit way. +Validating the type is only a fraction of what we want to do when validating parameters. +Therefore, we should remove this mechanism and replace it with a more general validation mechanism. +The more general validation mechanism can be considered out of scope for Sciline, and should be implemented in the user code or using other common libraries such as `pydantic`. + +Finally, we can think of this mechanism as a form of runtime type checking. +This is not the intended scope of Sciline. + +## Decision + +- Remove the mechanism that checks if a value is an instance of the key when setting it as a parameter. +- Encourage users to validate inputs in providers, which can also be tested in unit tests without setting up the full workflow. +- Encourage users to use a more general parameter validation mechanism using other libraries. + +## Consequences + +### Positive + +- The mechanism will no longer get in the way of the user. +- The code will be simplified slightly. + +### Negative + +- The correctness guarantees will be slightly diminished. \ No newline at end of file diff --git a/docs/developer/architecture-decision-records.md b/docs/developer/architecture-decision-records.md new file mode 100644 index 00000000..7368d750 --- /dev/null +++ b/docs/developer/architecture-decision-records.md @@ -0,0 +1,10 @@ +# Architecture Decision Records + +```{toctree} +--- +maxdepth: 1 +glob: true +--- + +adr/* +``` \ No newline at end of file diff --git a/docs/developer/index.md b/docs/developer/index.md index 704449fd..31d860f6 100644 --- a/docs/developer/index.md +++ b/docs/developer/index.md @@ -14,4 +14,5 @@ getting-started coding-conventions dependency-management architecture-and-design/index +architecture-decision-records ``` From 48ae841d8d372352fb6d348626cbf1dd24f1c755 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Thu, 11 Apr 2024 10:05:21 +0200 Subject: [PATCH 02/13] Add ADR 0002: Remove special handling of Optional and Union --- ...-special-handling-of-optional-and-union.md | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md diff --git a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md new file mode 100644 index 00000000..a8c687f0 --- /dev/null +++ b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md @@ -0,0 +1,92 @@ +# ADR 0002: Remove special handling of Optional and Union + +- Status: proposed +- Deciders: +- Date: 2024-04-11 + +## Context + +### General + +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. +Instead, an implicit `None` provider is added. +This has a series of problems, which we exemplify for the case of `Optional`. + +1. Default values (which are currently ignored by Sciline) are overridden by the implicit `None` provider. + In other words, Sciline assumes that the default value of the optional input is `None`. +2. Entire branches a pruned, which can hide bugs. + If the users added providers for the optional input, they will not be used if any of them has unintentionally unsatisfied dependencies. +3. The special mechanism prevents the (in principle very valid) use of any providers that return an `Optional` or `Union` type. +4. Optional inputs cannot be set to `None` *explicitly*. + +In summary, the special handling of `Optional` and `Union` is too implicit and causes more problems than it solves. +There are a couple more aspects to consider. + +### Readability of user code + +Handling `Optional` explicitly would make user code more readable. +Consider the following example: + +```python +pipeline[MyParam] = 1.2 +``` + +In the current implementation this gives no indication to the user that `MyParam` is not a required input. +Furthermore, if the line is removed, the user may not realize that `MyParam` is available as an optional input. +With the proposed change, the user can make this explicit: + +```python +pipeline[Optional[MyParam]] = 1.2 +``` + +Above it is clear that `MyParam` is optional, and it can be set to `None` explicitly: + +```python +pipeline[Optional[MyParam]] = None +``` + +### Code complexity and maintainability + +The special handling of `Optional` and `Union` is a significant source of complexity in the code, requiring a significant amount of unit testing. + +### Conceptual clarity + +The current redesign of Sciline highlighted that the current implementation is conceptually flawed. +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 + +#### Multiple providers may depend on the same input, but not all optionally + +This seems like a special case that we have not seen in practice, is likely not worth the complexity of the current implementation. + +#### Using a provider returning a non-optional output to fulfill an optional input + +This is a very valid use case, but it would be made impossible if we stop associating a node `T` with an optional input `Optional[T]`. +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]`. + +## Decision + +Remove the special handling of `Optional` and `Union`. + +## Consequences + +### Positive + +- Sciline's code will be simplified significantly. +- User code will be more readable. +- Implicit behavior around pruning and using `None` providers will be removed. +- Users can use providers that return `Optional` or `Union` types. +- Decouples the handling of optional inputs from the handling of default values. + This will enable us to make independent decisions about how to handle default values. + +### Negative + +- Workarounds are needed for the use case of using a provider returning a non-optional output to fulfill an optional input. \ No newline at end of file From 4643d3e20c8eeaaa7302be3b4c9ce8f1ddea30da Mon Sep 17 00:00:00 2001 From: Mridul Seth Date: Fri, 12 Apr 2024 15:31:08 +0200 Subject: [PATCH 03/13] Fix link to SANS example --- docs/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/index.md b/docs/index.md index 2988ec33..a3c96c43 100644 --- a/docs/index.md +++ b/docs/index.md @@ -116,7 +116,7 @@ The [User Guide](user-guide/index) also contains a description of more advanced At first it may feel unclear how to apply Sciline to your own code and workflows, given only the above documentation. Consider the following concrete examples of how we use Sciline in our own projects: - - [Small Angle Neutron Scattering](https://scipp.github.io/esssans/examples/sans2d.html) + - [Small Angle Neutron Scattering](https://scipp.github.io/esssans/user-guide/isis/sans2d.html) - [Neutron Powder Diffraction](https://scipp.github.io/essdiffraction/examples/POWGEN_data_reduction.html) - [Neutron Reflectometry](https://scipp.github.io/essreflectometry/examples/amor.html) From 0271bee2f09650bcde2630ff4e56d2d7c8b564ca Mon Sep 17 00:00:00 2001 From: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com> Date: Mon, 15 Apr 2024 05:19:57 +0200 Subject: [PATCH 04/13] Update docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md Co-authored-by: Sunyoung Yoo --- .../adr/0002-remove-special-handling-of-optional-and-union.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md index a8c687f0..75ebdf37 100644 --- a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md +++ b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md @@ -17,7 +17,7 @@ This has a series of problems, which we exemplify for the case of `Optional`. 1. Default values (which are currently ignored by Sciline) are overridden by the implicit `None` provider. In other words, Sciline assumes that the default value of the optional input is `None`. -2. Entire branches a pruned, which can hide bugs. +2. Entire branches are pruned, which can hide bugs. If the users added providers for the optional input, they will not be used if any of them has unintentionally unsatisfied dependencies. 3. The special mechanism prevents the (in principle very valid) use of any providers that return an `Optional` or `Union` type. 4. Optional inputs cannot be set to `None` *explicitly*. From 40d4b944470ac551660fdaf9462a98c4b4f42ce7 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 15 Apr 2024 10:34:10 +0200 Subject: [PATCH 05/13] Add more counter arguments --- ...remove-special-handling-of-optional-and-union.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md index 75ebdf37..532f5902 100644 --- a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md +++ b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md @@ -72,6 +72,19 @@ 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]`. +#### Using a provider to return one of a union's types + +Same as above, for `Optional[T]`. + +#### Setting union parameters is unwieldy + +Given a provider `f(x: A | B | C) -> D: ...`, a user would need to set a value for the input of `f` like `pipeline[A | B | C] = ...`. +It would be easier if they could be more specific, like `pipeline[A] = ...`. + +In this case, we think defining an alias for `A | B | C` would be a better solution than the current special handling of `Union`. +It would force the user to be more explicit about the input type, which is a good thing. +Conceptually the use of `Union` may just be an indicator that `f` depends on some common aspect of `A`, `B`, and `C`, which could be made explicit by defining a new type or protocol. + ## Decision Remove the special handling of `Optional` and `Union`. From 337090c60876882d3e868df69fb50267e820d425 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 15 Apr 2024 10:37:26 +0200 Subject: [PATCH 06/13] Update deciders --- .../0002-remove-special-handling-of-optional-and-union.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md index 532f5902..2b581974 100644 --- a/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md +++ b/docs/developer/adr/0002-remove-special-handling-of-optional-and-union.md @@ -1,8 +1,8 @@ # ADR 0002: Remove special handling of Optional and Union -- Status: proposed -- Deciders: -- Date: 2024-04-11 +- Status: accepted +- Deciders: Jan-Lukas, Johannes, Mridul, Simon, Sunyoung +- Date: 2024-04-15 ## Context @@ -102,4 +102,4 @@ Remove the special handling of `Optional` and `Union`. ### Negative -- Workarounds are needed for the use case of using a provider returning a non-optional output to fulfill an optional input. \ No newline at end of file +- Workarounds are needed for the use case of using a provider returning a non-optional output to fulfill an optional input, and for setting union parameters. \ No newline at end of file From bcf4f7dd05ea7b36024dccbfc9ed3fd9a3f54a04 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 15 Apr 2024 11:50:50 +0200 Subject: [PATCH 07/13] Extend the discussion --- ...instance-checks-when-setting-parameters.md | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md index d3bad07c..d89f9b7a 100644 --- a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md +++ b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md @@ -23,13 +23,38 @@ Therefore, we should remove this mechanism and replace it with a more general va The more general validation mechanism can be considered out of scope for Sciline, and should be implemented in the user code or using other common libraries such as `pydantic`. Finally, we can think of this mechanism as a form of runtime type checking. -This is not the intended scope of Sciline. +We should ask ourselves if this is the intended scope of Sciline. +If it is, shouldn't we also check that each provider actually returns the correct type? + +The main problem with not checking value types when setting parameters is that it is not possible to catch such errors with `mypy`, in contrast to return values of providers, which `mypy` *can* check. + +Consider the following example of setting $Q$ bins for a workflow, given by a `scipp.Variable`, which would then be passed to `scipp.hist` to create a histogram: + +```python +pipeline[QBins] = sc.linspace(...) +pipeline[QBins] = 1000 # error in current implementation +pipeline[QBins] = sc.linspace(..., unit='m') # no error, but wrong unit +``` + +Checking the type catches the first error, but not the second. +Paradoxically, setting an integer would often be a valid operation in the example, since `scipp.hist` can handle this case, whereas the wrong unit would not be valid. +This may indicate that defining `QBins` as an alias of `scipp.Variable` is actually an anti-pattern. +Instead, imagine we have defined a specific `class QBins`, which performs validation in its constructor, and defines `__call__` so it can be used as a provider: + +```python +pipeline.insert(QBins(sc.linspace(...))) +pipeline.insert(QBins(1000)) # ok +pipeline.insert(QBins(sc.linspace(..., unit='m'))) # error constructing QBins +``` + +This example illustrates that a clearer and more specific expression of intent can avoid the need for relying on checking the type of the value when setting a parameter. ## Decision - Remove the mechanism that checks if a value is an instance of the key when setting it as a parameter. - Encourage users to validate inputs in providers, which can also be tested in unit tests without setting up the full workflow. - Encourage users to use a more general parameter validation mechanism using other libraries. +- Consider adding a mechanism to inject a callable to use for parameter validation as a argument when creating a `Pipeline`. ## Consequences @@ -40,4 +65,4 @@ This is not the intended scope of Sciline. ### Negative -- The correctness guarantees will be slightly diminished. \ No newline at end of file +- `sciline.Pipeline` will support duck-typing for parameters, in a way that cannot be checked with `mypy`. \ No newline at end of file From 3962f7d142ec3783773a456e3aa28b77af744af4 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 15 Apr 2024 11:51:45 +0200 Subject: [PATCH 08/13] Update ADR metadata --- .../0001-remove-isinstance-checks-when-setting-parameters.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md index d89f9b7a..e3b3fd3e 100644 --- a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md +++ b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md @@ -1,8 +1,8 @@ # ADR 0001: Remove isinstance checks when setting parameters - Status: proposed -- Deciders: -- Date: 2024-04-11 +- Deciders: Jan-Lukas, Neil, Simon +- Date: 2024-04-15 ## Context From fb800cdc62dd3fc68eab0e766108627c9bb36e93 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 15 Apr 2024 12:48:56 +0200 Subject: [PATCH 09/13] Address comments --- .../0001-remove-isinstance-checks-when-setting-parameters.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md index e3b3fd3e..6661d644 100644 --- a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md +++ b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md @@ -38,7 +38,7 @@ pipeline[QBins] = sc.linspace(..., unit='m') # no error, but wrong unit Checking the type catches the first error, but not the second. Paradoxically, setting an integer would often be a valid operation in the example, since `scipp.hist` can handle this case, whereas the wrong unit would not be valid. -This may indicate that defining `QBins` as an alias of `scipp.Variable` is actually an anti-pattern. +This may indicate that defining `QBins` as an alias of `scipp.Variable` is actually an instance of an anti-pattern. Instead, imagine we have defined a specific `class QBins`, which performs validation in its constructor, and defines `__call__` so it can be used as a provider: ```python @@ -51,6 +51,8 @@ This example illustrates that a clearer and more specific expression of intent c ## Decision +- The core scope of Sciline is the definition of task graphs. + Type validation is not. - Remove the mechanism that checks if a value is an instance of the key when setting it as a parameter. - Encourage users to validate inputs in providers, which can also be tested in unit tests without setting up the full workflow. - Encourage users to use a more general parameter validation mechanism using other libraries. From 9471c2a9e0bf2ed824c9aa637a577ea65f192dca Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Mon, 15 Apr 2024 13:18:49 +0200 Subject: [PATCH 10/13] Remove unused skipifs --- tests/serialize/json_test.py | 6 ------ tests/utils_test.py | 8 -------- 2 files changed, 14 deletions(-) diff --git a/tests/serialize/json_test.py b/tests/serialize/json_test.py index fa5893cf..465dfe03 100644 --- a/tests/serialize/json_test.py +++ b/tests/serialize/json_test.py @@ -2,7 +2,6 @@ # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) # type: ignore -import sys from copy import deepcopy from typing import Any, NewType, TypeVar @@ -169,7 +168,6 @@ def node_sort_key(node: dict[str, Any]) -> str: } -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_serialize() -> None: pl = sl.Pipeline([make_int_b, zeros, to_string], params={Int[A]: 3}) graph = pl.get(str) @@ -217,7 +215,6 @@ def fn_w_kwonlyargs(*, x: int) -> float: } -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_serialize_kwonlyargs() -> None: pl = sl.Pipeline([fn_w_kwonlyargs], params={int: 3}) graph = pl.get(float) @@ -267,7 +264,6 @@ def repeated_arg(a: str, b: str) -> list[str]: } -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_serialize_repeated_arg() -> None: pl = sl.Pipeline([repeated_arg], params={str: 'abc'}) graph = pl.get(list[str]) @@ -337,7 +333,6 @@ def repeated_kwonlyargs(*, x: int, b: int) -> str: } -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_serialize_repeated_konlywarg() -> None: pl = sl.Pipeline([repeated_arg_kwonlyarg, repeated_kwonlyargs], params={int: 4}) graph = pl.get(list[str]) @@ -381,7 +376,6 @@ def test_serialize_repeated_konlywarg() -> None: } -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_serialize_lambda() -> None: lam = lambda x: float(x) # noqa: E731 lam.__annotations__['x'] = int diff --git a/tests/utils_test.py b/tests/utils_test.py index 31c944a5..96054fc2 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -63,7 +63,6 @@ def test_key_name_builtin_generic() -> None: assert _utils.key_name(dict[str, MyType]) == 'dict[str, MyType]' -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_key_name_custom_generic() -> None: MyType = NewType('MyType', float) Var = TypeVar('Var') @@ -76,7 +75,6 @@ class G(sciline.Scope[Var, str], str): assert _utils.key_name(G[MyType]) == 'G[MyType]' -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_key_name_custom_generic_two_params() -> None: MyType = NewType('MyType', float) Var1 = TypeVar('Var1') @@ -95,9 +93,6 @@ def test_key_full_qualname_builtin() -> None: assert _utils.key_full_qualname(object) == 'builtins.object' -# NewType returns a class since python 3.10, -# before that, we cannot get a proper name for it. -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_key_full_qualname_new_type() -> None: # The __qualname__ of NewTypes is the same as __name__, the result is therefore # missing test_key_full_qualname_new_type. @@ -141,7 +136,6 @@ def test_key_full_qualname_item() -> None: ) -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_key_full_qualname_builtin_generic() -> None: MyType = NewType('MyType', str) assert _utils.key_full_qualname(list) == 'builtins.list' @@ -156,7 +150,6 @@ def test_key_full_qualname_builtin_generic() -> None: ) -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_key_full_qualname_custom_generic() -> None: MyType = NewType('MyType', float) Var = TypeVar('Var') @@ -179,7 +172,6 @@ class G(sciline.Scope[Var, str], str): ) -@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher") def test_key_full_qualname_custom_generic_two_params() -> None: MyType = NewType('MyType', float) Var1 = TypeVar('Var1') From 1fd28489f5fae474fcd52cc453a2785229a71ae2 Mon Sep 17 00:00:00 2001 From: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com> Date: Mon, 15 Apr 2024 13:20:38 +0200 Subject: [PATCH 11/13] Fix status in ADR 0001 --- .../0001-remove-isinstance-checks-when-setting-parameters.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md index 6661d644..fdc30734 100644 --- a/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md +++ b/docs/developer/adr/0001-remove-isinstance-checks-when-setting-parameters.md @@ -1,6 +1,6 @@ # ADR 0001: Remove isinstance checks when setting parameters -- Status: proposed +- Status: accepted - Deciders: Jan-Lukas, Neil, Simon - Date: 2024-04-15 @@ -67,4 +67,4 @@ This example illustrates that a clearer and more specific expression of intent c ### Negative -- `sciline.Pipeline` will support duck-typing for parameters, in a way that cannot be checked with `mypy`. \ No newline at end of file +- `sciline.Pipeline` will support duck-typing for parameters, in a way that cannot be checked with `mypy`. From 15d68235e211216c24784c34b0bc956d75925dd5 Mon Sep 17 00:00:00 2001 From: Mridul Seth Date: Fri, 19 Apr 2024 15:37:33 +0200 Subject: [PATCH 12/13] MAINT: Error out gracefully with an unsupported scheduler --- src/sciline/scheduler.py | 12 +++++++++++- src/sciline/task_graph.py | 4 ++++ tests/task_graph_test.py | 10 ++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/sciline/scheduler.py b/src/sciline/scheduler.py index 2c08f98e..9d658671 100644 --- a/src/sciline/scheduler.py +++ b/src/sciline/scheduler.py @@ -1,7 +1,16 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2023 Scipp contributors (https://github.com/scipp) import inspect -from typing import Any, Callable, Dict, List, Optional, Protocol, Tuple +from typing import ( + Any, + Callable, + Dict, + List, + Optional, + Protocol, + Tuple, + runtime_checkable, +) from sciline.typing import Graph, Key @@ -10,6 +19,7 @@ class CycleError(Exception): pass +@runtime_checkable class Scheduler(Protocol): """ Scheduler interface compatible with :py:class:`sciline.Pipeline`. diff --git a/src/sciline/task_graph.py b/src/sciline/task_graph.py index ab66fd19..786603f7 100644 --- a/src/sciline/task_graph.py +++ b/src/sciline/task_graph.py @@ -81,6 +81,10 @@ def __init__( scheduler = DaskScheduler() except ImportError: scheduler = NaiveScheduler() + elif not isinstance(scheduler, Scheduler): + raise ValueError( + "Scheduler interface must becompatible with sciline.Scheduler" + ) self._scheduler = scheduler def compute( diff --git a/tests/task_graph_test.py b/tests/task_graph_test.py index 1c5fc8af..799c231c 100644 --- a/tests/task_graph_test.py +++ b/tests/task_graph_test.py @@ -81,3 +81,13 @@ def test_keys_iter() -> None: tg = pl.get(list[str]) assert len(list(tg.keys())) == 4 # there are no duplicates assert set(tg.keys()) == {A, B, Str[B], list[str]} + + +def test_scheduler_not_supported() -> None: + with pytest.raises( + ValueError, + match="Scheduler interface must be compatible with sciline.Scheduler", + ): + TaskGraph( + graph={}, targets=(), scheduler="not a scheduler" # type: ignore[arg-type] + ) From c448b1e75856cb010536a5a42a3b4f5586b2612a Mon Sep 17 00:00:00 2001 From: Mridul Seth Date: Fri, 19 Apr 2024 15:51:06 +0200 Subject: [PATCH 13/13] fix typo --- src/sciline/task_graph.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sciline/task_graph.py b/src/sciline/task_graph.py index 786603f7..e9662b93 100644 --- a/src/sciline/task_graph.py +++ b/src/sciline/task_graph.py @@ -83,7 +83,7 @@ def __init__( scheduler = NaiveScheduler() elif not isinstance(scheduler, Scheduler): raise ValueError( - "Scheduler interface must becompatible with sciline.Scheduler" + "Scheduler interface must be compatible with sciline.Scheduler" ) self._scheduler = scheduler