-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: add option to not override results by Shaper
#4231
Conversation
@tstadel it seems totally reasonable and a small change. I was just wondering why did we restrict output to only the variables listed on line |
TBH I don't know. Maybe we don't want to spoil the pipeline result with intermediate results. But I agree, I guess with this option this selection seems superfluous. @vblagoje Shall we remove it then? |
Removing it seems risky tbh, although all the tests pass (at least for me locally). But that doesn't make it safe. Do you have some additional tests besides shaper and prompt node tests? |
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.
LGTM!
haystack/nodes/other/shaper.py
Outdated
@@ -404,7 +425,7 @@ def run( # type: ignore | |||
results = {} | |||
for output_key, output_value in zip(self.outputs, output_values): | |||
invocation_context[output_key] = output_value | |||
if output_key in ["query", "file_paths", "labels", "documents", "meta"]: | |||
if self.publish_outputs and output_key in ["query", "file_paths", "labels", "documents", "meta", "answers"]: |
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.
Ok, I'll do so.
@vblagoje running the pipeline above works. Besides that I don't have any tests. But we would be able to control this via |
haystack/nodes/other/shaper.py
Outdated
@@ -404,7 +425,7 @@ def run( # type: ignore | |||
results = {} | |||
for output_key, output_value in zip(self.outputs, output_values): | |||
invocation_context[output_key] = output_value | |||
if output_key in ["query", "file_paths", "labels", "documents", "meta"]: | |||
if self.publish_outputs and output_key in ["query", "file_paths", "labels", "documents", "meta", "answers"]: |
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.
Ok, I'll do so.
Let's add a test for what happens if there are nodes downstream, though: @vblagoje is right, Pipeline might fail if it receives unexpected output that is not included in that list. Not critical, but might be a source of weird bugs. |
That's exactly what I was worried about @ZanSara . Some component downstream from Shaper that doesn't declare run variable that the Shaper outputs?! It should be fine if Shaper is the last component in the pipeline. |
Theoretically it should be safe as we're inspecting the run_signature before passing it, see haystack/haystack/nodes/base.py Line 240 in 274746d
But I'll add a test to be safe. |
How about a list of published outputs? If it is not set we publish the default vars (what we have now) and if it is set we publish the list of init declared vars. This could be useful in the last Shaper in the pipeline where we want to have those vars in the root of results dict? |
By just having a list, it might not seem intuitive that the default value (=None) would result in all outputs being published. How about converting it into |
Sounds good to me. I'll let @ZanSara have the last word |
Sounds good to me too! Let's implement it that way |
@ZanSara done. I'll merge once all tests passed. |
Related Issues
documents
get overridden byShaper
#4230Proposed Changes:
publish_outputs
option, defaults toTrue
(same behavior as before)False
outputs
won't be published to the pipeline resultanswers
to the eligible outputs that can be publishedHow did you test it?
Notes for the reviewer
Shapers
in a QA pipeline usingPromptNode
. See the issue above for more information.Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.