-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support replacing params and providers #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just suggested more unit tests but maybe those cases are already implied in other unit cases...
pl.insert(func) | ||
pl[int] = 1 | ||
assert pl.compute(int) == 1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_setitem_can_replace_instance() -> None: | |
pl = sl.Pipeline() | |
original_number = 1 | |
pl[int] = original_number | |
assert pl.compute(int) is original_number | |
new_number = 1 | |
pl[int] = new_number | |
assert pl.compute(int) == original_number | |
assert pl.compute(int) is not original_number | |
assert pl.compute(int) is new_number | |
Can we also have a test case that shows instance check like this...?
Just to show that it's passed by lambda function, so the instance is kept not just the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int
is immutable, not sure I see the value of such a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it shows that it doesn't copy the value and return it but keep the instance itself.
In case we want to pass around dict
, it might be useful information I thought...?
I just suggested what I wanted to try with the changes.
Feel free to drop them...!
pl[A[T]] = A[T](1) # type: ignore[valid-type] | ||
assert pl.compute(A[int]) == A[int](1) | ||
pl[A[T]] = A[T](2) # type: ignore[valid-type] | ||
assert pl.compute(A[int]) == A[int](2) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_setitem_replace_generic_param_not_prioritized() -> None: | |
T = TypeVar('T') | |
@dataclass | |
class A(Generic[T]): | |
value: T | |
pl = sl.Pipeline(params={A[int]: 1}) | |
pl[A[T]] = A[T](0.1) # type: ignore[valid-type] | |
assert pl.compute(A[int]) == A[int](1) | |
assert pl.compute(A[float]) == A[float](0.1) | |
Maybe it's worth show/check that the inserted generic provider doesn't overwrite or be prioritized to the explicit one...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to this change, I think? There are already tests for specializations.
Fixes #57