Add pipeline copy method #91
Conversation
tests/pipeline_test.py
Outdated
There was a problem hiding this comment.
Missing tests that modifying a or b after the copy does not affect the other pipeline.
src/sciline/pipeline.py
Outdated
| @@ -322,6 +322,9 @@ def __init__( | |||
| params: | |||
| Dictionary of concrete values to provide for types. | |||
| """ | |||
| self._copy_providers = list(providers or []) | |||
| self._copy_params: Dict[type, Any] = {} | |||
There was a problem hiding this comment.
I don't understand why those are needed? Can't you just make copies of self._providers, etc.?
There was a problem hiding this comment.
No because self._providers contains all the original providers, plus a whole bunch of lambda functions that were made from the parameters, which then fail if you try to construct a new pipeline with them.
Also, going through the sub-providers is a bit of a mess, especially with generics. So I thought just saving the constructor params was far easier. But maybe @SimonHeybrock has a better idea?
There was a problem hiding this comment.
Why are the lambdas failing?
There was a problem hiding this comment.
ValueError: Provider <function Pipeline.__setitem__.<locals>.<lambda> at 0x7f492825f130> lacks type-hint for return value
There was a problem hiding this comment.
The lambda is raising that? That is really odd.
There was a problem hiding this comment.
Just to be clear, I get the error from the lambdas when I do:
Pipeline(existing_pipeline._providers.values())There was a problem hiding this comment.
Making a copy of the dicts internally works fine.
…d on copy, and vice versa
|
Looking at self._providers: Dict[Key, Provider] = {}
self._subproviders: Dict[type, Dict[Tuple[Key | TypeVar, ...], Provider]] = {}
self._param_tables: Dict[Key, ParamTable] = {}
self._param_name_to_table_key: Dict[Key, Key] = {}Why can't we just copy these four dicts, and copy the nested dicts in |
Can you have more than one level of nesting? |
No. |
| @@ -794,3 +794,14 @@ def bind_and_call( | |||
| if not return_tuple: | |||
| return results[0] | |||
| return results | |||
|
|
|||
| def copy(self) -> Pipeline: | |||
There was a problem hiding this comment.
Do we actually need a custom copy implementation? Or would this be enough with the default __copy__?
def copy(self) -> Pipeline:
from copy import copy
return copy(self)There was a problem hiding this comment.
I think we need to copy the (nested) dicts ourselves, no?
There was a problem hiding this comment.
Right! It's deep as far as the dicts are concerned but not the dict elements.
This makes it possible to make a copy of an existing pipeline.
One can then, for example modify the parameters of the new pipeline.