-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor: Isolate logic that finds next runnable component waiting for input #7880
Conversation
Pull Request Test Coverage Report for Build 9549247852Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9549480203Details
💛 - Coveralls |
71f7cb6
to
d957f6a
Compare
Pull Request Test Coverage Report for Build 9549815154Details
💛 - Coveralls |
haystack/core/pipeline/base.py
Outdated
for other_name, other_comp in waiting_for_input: | ||
if name == other_name: | ||
continue | ||
there_are_only_lazy_variadics &= ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rewrite this check to break early if a non(-lazy)-variadic component is encountered.
haystack/core/pipeline/base.py
Outdated
is_variadic = any(socket.is_variadic for socket in comp.__haystack_input__._sockets_dict.values()) # type: ignore | ||
if is_variadic and not comp.__haystack_is_greedy__: # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract this into a nested helper function.
haystack/core/pipeline/base.py
Outdated
is_variadic = any(socket.is_variadic for socket in comp.__haystack_input__._sockets_dict.values()) # type: ignore | ||
if is_variadic and not comp.__haystack_is_greedy__: # type: ignore[attr-defined] | ||
there_are_only_lazy_variadics = True | ||
for other_name, other_comp in waiting_for_input: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the looks of this, we only need to perform this check once. Let's move it outside of the outer loop.
haystack/core/pipeline/base.py
Outdated
) | ||
if has_only_defaults: | ||
there_are_only_components_with_defaults = True | ||
for other_name, other_comp in waiting_for_input: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - Doesn't look like it needs to be recalculated for every component.
haystack/core/pipeline/base.py
Outdated
there_are_only_components_with_defaults &= all( | ||
not s.is_mandatory | ||
for s in other_comp.__haystack_input__._sockets_dict.values() # type: ignore | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - We can break early.
haystack/core/pipeline/base.py
Outdated
if input_socket.is_mandatory and input_socket.name not in inputs_by_component[name]: | ||
has_enough_inputs = False | ||
break | ||
if input_socket.is_mandatory: | ||
continue | ||
|
||
if input_socket.name not in inputs_by_component[name]: | ||
inputs_by_component[name][input_socket.name] = input_socket.default_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rewrite this to just use a single if input_socket.name not in inputs_by_component[name]:
- break if it's mandatory, copy default value otherwise.
I updated the comment. |
Pull Request Test Coverage Report for Build 9562857219Warning: 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
💛 - Coveralls |
After some discussion with @shadeMe we decided to change the refactoring approach a bit. |
Pull Request Test Coverage Report for Build 9565856215Warning: 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
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9566408308Warning: 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
💛 - Coveralls |
Related Issues
Part of #7614.
Proposed Changes:
This PR isolates the logic that moves Components from
waiting_for_input
toto_run
queue whento_run
is empty andwaiting_for_input
is not.How did you test it?
I added unit tests and ran tests locally.
Notes for the reviewer
There's some formatting changes too as we changed the default formatted from
black
toruff
.Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.