Propose ADR 0001: Remove isinstance checks when setting parameters #153
Conversation
SimonHeybrock
changed the title
|
|
||
| - 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. |
There was a problem hiding this comment.
Maybe we can also provide a small framework to help users carry out validation?
There was a problem hiding this comment.
My plan is to add something basic in essreduce, which will also be used to auto-generate widgets for setup up workflows.
|
👍 |
There was a problem hiding this comment.
I'm not sure this is a good idea. It would not solve #140 and #144. E.g., this
def foo(p: Path) -> str:
return p.read_text()
pl = Pipeline([foo], params={Path: "my/file"})
tg = pl.get(str)would work but it would fail during computation.
As I see it, this introduces duck-typing for functions that are annotated with strict type annotations and thus violate the assumptions of the author of the function.
To give another example, a function annotated to take a DataGroup may be able to work with any Mapping. But that would be coincidental and an implementation detail because the function is allowed to use the full interface of DataGroup.
So I think this proposal only solves #145.
You can argue that at runtime, we strictly speaking always have duck-typing. But that can be checked by a type checker. What you are proposing is uncheckable, as far as I can tell.
There was a problem hiding this comment.
I don't think it violates the assumptions of the author. Unless we explicitly promise to validate types, users should assume the same level of type checking as anywhere else in python land (that is, duck typing).
I think you're right that setting parameter values would be difficult to check by a type checker. Not sure.
There was a problem hiding this comment.
As I see it, this introduces duck-typing for functions that are annotated with strict type annotations and thus violate the assumptions of the author of the function.
Not sure I understand the argument. Don't you have the same problem with providers that lie about their return type? Is your concern that providers can be checked with mypy but Pipeline.__setitem__ cannot?
Regarding DataGroup, in general I would argue that the use of domain types (type aliases) is a way of encoding a potentially very complex contract into a brief name. Does this absolve the consumer from checking relevant parts of the contract? Is it not allowed to fail if the contract is violated? I would see the type as part of the contract.
Not sure why you think #144 is not solved. We can argue about #140, but wait for ADR-0002 where I will propose to remove special handling of Optional and Union.
|
|
||
| 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. |
There was a problem hiding this comment.
| 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 not precise enough. |
It's a too specific case to call it a pattern.
| 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? |
There was a problem hiding this comment.
Can you reformulate to turn it into a statement that this is not the scope of sciline?
There was a problem hiding this comment.
I guess I should put that statement into the "Decision" section?
Please comment!