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: Fix running Pipeline with conditional branch and Component with default inputs #7799

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

silvanocerza
Copy link
Contributor

@silvanocerza silvanocerza commented Jun 4, 2024

Related Issues

Fixes

Proposed Changes:

This PR changes a bit how we collect the initial list of Component that can run when calling Pipeline.run().

Instead of getting only Component that have no inputs, or have a least one input that is not connected or is variadic, we also get Components that receive inputs from the user.

It also changes how we handle conditional outputs. Now if a Component with a conditional output runs and doesn't send anything to a certain socket we remove from to_run and waiting_for_input all the Components connected to that socket.

How did you test it?

I updated tests locally and ran unit, Pipeline.run() and e2e tests.

Notes for the reviewer

N/A

Checklist

@silvanocerza silvanocerza self-assigned this Jun 4, 2024
@silvanocerza silvanocerza requested review from a team as code owners June 4, 2024 10:04
@silvanocerza silvanocerza requested review from dfokina and masci and removed request for a team June 4, 2024 10:04
@coveralls
Copy link
Collaborator

coveralls commented Jun 4, 2024

Pull Request Test Coverage Report for Build 9401345309

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 51 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 89.692%

Files with Coverage Reduction New Missed Lines %
components/converters/html.py 2 95.35%
core/pipeline/base.py 4 92.79%
evaluation/eval_run_result.py 4 92.19%
core/pipeline/pipeline.py 41 62.58%
Totals Coverage Status
Change from base Build 9370408614: -0.03%
Covered Lines: 6778
Relevant Lines: 7557

💛 - Coveralls

@silvanocerza silvanocerza marked this pull request as draft June 4, 2024 15:18
@silvanocerza silvanocerza removed request for dfokina and masci June 4, 2024 15:18
@silvanocerza silvanocerza force-pushed the fix-conditional-branching branch from 2a10e65 to fbee12a Compare June 5, 2024 10:39
@github-actions github-actions bot added the type:documentation Improvements on the docs label Jun 5, 2024
@silvanocerza silvanocerza marked this pull request as ready for review June 5, 2024 10:44
@silvanocerza silvanocerza requested review from masci and shadeMe June 5, 2024 10:44
@@ -719,7 +719,7 @@ def _init_inputs_state(self, data: Dict[str, Dict[str, Any]]) -> Dict[str, Dict[

return {**data}

def _init_to_run(self) -> List[Tuple[str, Component]]:
def _init_to_run(self, data: Dict[str, Any]) -> List[Tuple[str, Component]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a better name than data. pipeline_inputs?

@silvanocerza silvanocerza enabled auto-merge (squash) June 6, 2024 13:10
@silvanocerza silvanocerza merged commit 3c8569e into main Jun 6, 2024
22 checks passed
@silvanocerza silvanocerza deleted the fix-conditional-branching branch June 6, 2024 13:19
vblagoje pushed a commit that referenced this pull request Jul 3, 2024
…h default inputs (#7799)

* Fix running Pipeline with conditional branch and Component with default inputs

* Add release notes

* Change arg name of _init_to_run so it's clearer

* Enhance release note
vblagoje pushed a commit that referenced this pull request Jul 3, 2024
…h default inputs (#7799)

* Fix running Pipeline with conditional branch and Component with default inputs

* Add release notes

* Change arg name of _init_to_run so it's clearer

* Enhance release note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants