fix: Use isinstance to support subclasses of values for deepmap #228
Conversation
For example this enables support for NamedTuples, making return types more clear from pipedag tasks
There was a problem hiding this comment.
I am not sure your goal is achieved by this change. The NamedTuple is converted to a tuple and loses its names.
We actually have deep_map as a pretty fundamental tool in both pipedag and transform which means it should work quite consistently over a very large code base according to general assumptions. I would expect deep_map(v, lambda x: x) to behave very close to deep_copy for collections (non-collection elements are not copied).
I suggest to explicitly handle NamedTuple and python data-classes to produce exactly the same type after the copy than it was before.
| y = _deep_map_list(x, fn, memo) | ||
| elif cls == tuple: | ||
| elif isinstance(x, tuple): |
There was a problem hiding this comment.
@btat-qc we talked about extensions like this in deep_map before. The question is what the expected behavior should be. deep_map(v, lambda x: x) should be very close to a deep copy without changing anything. With the change you proposed, the NamedTuple would be converted to a tuple if I am not mistaken.
I would suggest to explicitly handle NamedTuples and data classes here to guarantee that they are deep copied to the same type.
There was a problem hiding this comment.
I see - that makes sense! Thanks I'll think about how to solve this and add some tests
For example this enables support for NamedTuples, making return types more clear from pipedag tasks
Checklist
docs/source/changelog.mdentrydocs/source/docs/source/examples.md