Skip to content
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

Fix JoinAnswer/JoinNode #2612

Merged
merged 19 commits into from
Jun 17, 2022
Merged

Fix JoinAnswer/JoinNode #2612

merged 19 commits into from
Jun 17, 2022

Conversation

MichelBartels
Copy link
Contributor

Proposed changes:
As described in #2599 and #2593, there are currently issues with nodes that can accept multiple inputs. This PR addresses these by changing the nodes and not the pipelines like in #2593.

Status (please check what you already did):

@MichelBartels MichelBartels requested a review from ZanSara May 31, 2022 11:03
@ZanSara
Copy link
Contributor

ZanSara commented May 31, 2022

@MichelBartels I had a look at the changes, good progess! I will review them shortly, but first of all let me highlight one thing: let's not mess with the BaseComponent.run() signature. It's unfixable in its current form, even adding **kwargs is going to make mypy go mad. Please leave it alone 🙏

JoinNode.run() method should simply silence the mypy error. It's really ugly, but that's how BaseComponent is right now. Sorry for that 😅 On the same note, the changes in BaseComponent about take_kwargs are also better kept for a more comprehensive PR about improving the situation with BaseComponent, unless you think those kwargs are really necessary to some node. In that case, let's review why: **kwargs are dangerous in the current situation.

I will link this PR to the ones related to BaseComponent.run() as yet another proof that stuff here needs a good rewrite fast...

@ZanSara
Copy link
Contributor

ZanSara commented Jun 15, 2022

Hey @MichelBartels how about this one? Shall I take care of it? It's not urgent but I'd like to see this in master 🙂

@MichelBartels
Copy link
Contributor Author

Hi @ZanSara, sorry forgot a bit about this. I have now done your suggested changes. kwargs is now no longer used. Not using it will probably make things more cumbersome in the future, but I understand that it makes sense to wait.

Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you very much for this PR 😊 Hopefully the issues you had to work around will be fixed very soon.

@ZanSara ZanSara merged commit 964e6cd into master Jun 17, 2022
@ZanSara ZanSara deleted the fix-join-nodes branch June 17, 2022 14:29
Krak91 pushed a commit to Krak91/haystack that referenced this pull request Jul 26, 2022
* fix join nodes

* Update Documentation & Code Style

* fix unused import

* change arg order

* Update Documentation & Code Style

* fix kwargs check

* add warning when there is only one input node

* Update Documentation & Code Style

* fix type hint

* fix wrong import order

* Update Documentation & Code Style

* undo kwargs

* add accidentally deleted newline#

* fix type hint

* fix type hint

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:pipeline type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants