Skip to content

Fix/informative error message #9661

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix/informative error message #9661

wants to merge 7 commits into from
+46 −2

Conversation

RafaelJohn9
Copy link
Contributor

Related Issues

Proposed Changes:

Made the error more descriptive using the formatted recommended by @sjrl .

How did you test it?

  • manual verification

Notes for the reviewer

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added unit tests and updated the docstrings
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@RafaelJohn9 RafaelJohn9 requested a review from a team as a code owner July 29, 2025 17:41
@RafaelJohn9 RafaelJohn9 requested review from mpangrazzi and removed request for a team July 29, 2025 17:41
@coveralls
Copy link
Collaborator

coveralls commented Jul 29, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 16902023987

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 50 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.001%) to 91.97%
Files with Coverage Reduction New Missed Lines %
core/pipeline/base.py 50 86.92%
Totals Coverage Status
Change from base Build 16880323123: 0.001%
Covered Lines: 12840
Relevant Lines: 13961
💛 - Coveralls

@sjrl sjrl self-requested a review August 4, 2025 07:41
sjrl
sjrl reviewed Aug 4, 2025
View reviewed changes
@RafaelJohn9
Copy link
Contributor Author

Hey @sjrl ,

On the first recommended error message when sender_sockets is not empty.

haystack.core.errors.PipelineConnectError: 'valid_component.result does not exist. Output connections of valid_component are:

do you think this would be a more clear error message:

haystack.core.errors.PipelineConnectError: Components 'comp1' and 'comp2' are already connected. 'comp1.value' is already connected to 'comp2.value'

Reason

I was debugging the test : test/core/pipeline/test_pipeline_base.py::TestPipelineBase::test_connect_already_connected
which now fails with the new logic included.

With the current error message this is what it looks like

FAILED test/core/pipeline/test_pipeline_base.py::TestPipelineBase::test_connect_already_connected - haystack.core.errors.PipelineConnectError: 'comp1.value' does not exist. Output connections of comp1 are: value (type int)

and I thought this would add more clarity

FAILED test/core/pipeline/test_pipeline_base.py::TestPipelineBase::test_connect_already_connected - haystack.core.errors.PipelineConnectError: Components 'comp1' and 'comp2' are already connected. 'comp1.value' is already connected to 'comp2.value'.

Would love to hear your thoughts on this

@sjrl
Copy link
Contributor

sjrl commented Aug 5, 2025 •
edited
Loading

and I thought this would add more clarity

FAILED test/core/pipeline/test_pipeline_base.py::TestPipelineBase::test_connect_already_connected - haystack.core.errors.PipelineConnectError: Components 'comp1' and 'comp2' are already connected. 'comp1.value' is already connected to 'comp2.value'.

Would love to hear your thoughts on this

Great catch! Yes that would be a much better error message to provide. So let's go with your suggestion for this third case.

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Aug 11, 2025
@RafaelJohn9 RafaelJohn9 requested a review from sjrl August 11, 2025 17:06
@RafaelJohn9
Copy link
Contributor Author

I do apologize for disabling C901 complexity check inside PipelineBase.connect method, though I had the following precommit error.

haystack/core/pipeline/base.py:434:9: C901 connect is too complex (21 > 20)    |432 |

sjrl
sjrl reviewed Aug 12, 2025
View reviewed changes
sjrl
sjrl reviewed Aug 12, 2025
View reviewed changes
haystack/core/pipeline/base.py
Comment on lines +459 to +464
if not sender_sockets:
raise PipelineConnectError(
f"'{sender_component_name}' does not have any output connections. "
f"Please check that the output types of '{sender_component_name}.run' are set, "
f"for example by using the '@component.output_types' decorator."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want a similar check and error message if receiver_sockets don't exist either right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrl sjrl sjrl left review comments

@mpangrazzi mpangrazzi Awaiting requested review from mpangrazzi mpangrazzi is a code owner automatically assigned from deepset-ai/open-source-engineering

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
topic:core topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return a more informative error message when trying to connect two components when the sender component is missing it's OutputSockets
3 participants
@RafaelJohn9 @coveralls @sjrl