feat: Join component
#5852
feat: Join component
#5852
Conversation
github-actions
bot
added
topic:tests
type:documentation
| :param inputs_count: the number of inputs to expect. | ||
| :param inputs_type: the type of the inputs. Every type that supports the + operator works. | ||
| """ | ||
| self.inputs_count = inputs_count |
There was a problem hiding this comment.
Why not set it to 2 by default?
There was a problem hiding this comment.
Why having inputs_count at all? The degenerate case would be having only one input, that would make the component a no-op
There was a problem hiding this comment.
Sorry @masci I don't get your point. I can set it to be minimum two, but in principle being able to set how many inputs to expect seems way more useful than fixing it to two. There are many cases where you want to aggregate several values (just look at this pipeline)
There was a problem hiding this comment.
Why having inputs_count at all?
I'm not saying to hardcode to two, I'm talking about removing it, why do we need an upper limit?
There was a problem hiding this comment.
This is not an upper limit: you need to specify in advance how many inputs you expect. I mean, I could set it to a huge number and make them all optional, but it will make quite some noise in the error messages: if you fail to connect it, the error message will list all the possible input connections and the error becomes seriously unreadable 😅 I'd rather let the user specify how many they want and stick to that. It makes debugging easier.
If this is a big deal let's discuss it offline.
There was a problem hiding this comment.
Yeah, it would be great to remove input_counts parameter if possible.
Pull Request Test Coverage Report for Build 6301669713
💛 - Coveralls |
| :param inputs_count: the number of inputs to expect. | ||
| :param inputs_type: the type of the inputs. Every type that supports the + operator works. | ||
| """ | ||
| self.inputs_count = inputs_count |
There was a problem hiding this comment.
Why having inputs_count at all? The degenerate case would be having only one input, that would make the component a no-op
|
Closing for now in favor of deepset-ai/canals#116 |
Related Issues
Proposed Changes:
How did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.