feat: Add tests for auto joiner with auto variadic behavior#10949
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Pull Request Test Coverage Report for Build 23642363216Details
💛 - Coveralls |
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Overall this looks good, but there is an issue regarding the output order. In my opinion users would expect the joined lists to retain the order in which the components were connected. This doesn't seem to be the case if the two input components to a receiver component produce different types.
(Also, not sure about the feat commit type and release note in the current state of the PR - technically this is just adding tests, but I also see that having the release note will increase visibility that this behavior is supported.)
| p.connect("str_producer.text", "receiver.texts") | ||
| p.connect("list_producer.texts", "receiver.texts") | ||
| result = p.run({}) | ||
| assert result["receiver"]["result"] == ["world", "!", "hello"] |
There was a problem hiding this comment.
I'm not sure about the result order here. Since str_producer is connected before list_producer, I would intuitively expect the joined output to retain that connection order, so ["hello", "world", "!"].
There was a problem hiding this comment.
The result order is actually based on alphabetical order of the component names in this case which is why the order is slightly different. It's because we run ordered_component_names = sorted(self.graph.nodes.keys()) internally and calculate priorities of components based on this ordered list.
There was a problem hiding this comment.
And in the case of AsyncPipeline actually these two components str_producer and list_producer would actually run in parallel so the order would be non-deterministic.
So basically in general we don't expect inputs to joiners to be deterministic.
| p.connect("str_producer.text", "receiver.texts") | ||
| p.connect("chat_producer.message", "receiver.texts") | ||
| result = p.run({}) | ||
| assert result["receiver"]["result"] == ["world", "hello"] |
There was a problem hiding this comment.
Here I'd expect the output order to match the connection order as well.
| p.connect("str_producer.text", "receiver.messages") | ||
| p.connect("chat_producer.message", "receiver.messages") | ||
| result = p.run({}) | ||
| assert [m.text for m in result["receiver"]["result"]] == ["world", "hello"] |
There was a problem hiding this comment.
Here I'd expect the output order to match the connection order as well.
Yeah I was going for increasing the visibility of this, but I agree it's just tests. |
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Looking good, I'll open an issue to better document the order.
3 participants
Related Issues
Proposed Changes:
Adding support for auto joiner and auto conversion behavior. It actually already works so this PR just adds tests for the different scenarios.
How did you test it?
Added new unit tests.
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.