fix: cyclic pipelines should run stable

[mathislucka]

Related Issues

• PR mirrors Fix: Pipeline.run logic haystack#8707

Proposed Changes:

How did you test it?

Notes for the reviewer

Note that the PR does not add any new code in base.py. Check the original branch to see the diff. The PR only removes code from base.py.

Also note, that the AsyncPipeline will not run anymore when this change is merged to haystack main as it relies on some methods in PipelineBase that I removed.

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:.
• I documented my code
• I ran pre-commit hooks and fixed any issue

[coveralls]

Pull Request Test Coverage Report for Build 12952190375

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-14.3%) to 64.394%

---

Totals
Change from base Build 12948231503:	-14.3%
Covered Lines:	1483
Relevant Lines:	2303

---

💛 - Coveralls

[davidsbatista]

@mathislucka I think this PR should have at least one more reviewer, ideally +2, this is a big change

[mathislucka]

@mathislucka I think this PR should have at least one more reviewer, ideally +2, this is a big change

Yes, @Amnah199 already started reviewing the PR in Haystack (core). @julian-risch will also review.

[davidsbatista]

we need to add Pipeline to the correct __init__.py hierarchy so that after the release users can simply do:

from haystack_experimental import Pipeline

I've been using it to test several pipelines and always have to do a long import, i.e.:

from haystack_experimental.core.pipeline.pipeline import Pipeline

[davidsbatista]

regarding the test files - I tried to run them but it failed due to a mixing fixture - fixture 'spying_tracer' not found - then later, talking with @julian-risch, I understood that we want to keep the behavioral tests out of this repo (at least for now).

I would suggest we completely remove them or add them in total/working - just to keep the PR in a clean state - we can then later add the full tests in another PR - so that if we need to rollback for whatever reason things are in a clean state.

[mathislucka]

I found another bug that affects non-cyclic pipelines while working on super components.

Consider the pipeline that is created here:

haystack-experimental/haystack_experimental/super_components/converters/file_converter.py

Line 88 in 72528c5

# initialize components

And have a look at the example notebook for the AutoFileConverter.

I found that depending on the insertion and connection order, the tabular_joiner might run with partial inputs which leads to a change in results.

The tabular_joiner will run twice in these cases and it will only ever output either the documents coming from joiner or from the csv converter.

This happens because after running all the file converters, the tabular_joiner can be the only component in the run queue. Once we pop it from the run queue it will be empty.

This is a problem because: https://github.com/deepset-ai/haystack/blob/c989d9c483a4fc74147e17065779fd6103f4a084/haystack/core/pipeline/pipeline.py#L429

Will check the empty run queue (all([])) which is True. The component will then be executed, removing (for example) the output from the csv converter from its inputs. This will lead to incomplete inputs when it runs again.

Edit: To clarify, this bug affects the original logic but probably not this one. We should add a test case though.

[julian-risch]

Looks good to me!
We tested many different pipelines and will continue with comparing traces of pipelines running with the old and the new logic after this PR is merged. We're checking for any differences in pipeline execution order, changes in component inputs/outputs, and decreases/increases in speed.
In addition, we're still testing the new logic with a custom component that is greedy variadic and has an optional input as described here. And we'll need to add a test case for the tabular_joiner issue described above.