fix: add option to not override results by `Shaper` #4231

tstadel · 2023-02-22T10:51:06Z

Related Issues

fixes documents get overridden by Shaper #4230

Proposed Changes:

add publish_outputs option, defaults to True (same behavior as before)
when False outputs won't be published to the pipeline result
add answers to the eligible outputs that can be published

How did you test it?

added tests

Notes for the reviewer

This will save us two additional Shapers in a QA pipeline using PromptNode. See the issue above for more information.
The yaml of the issue pipeline would then look like this:

version: '1.13.2'
components:
  - name: DocumentStore
    type: InMemoryDocumentStore
  - name: Retriever
    type: BM25Retriever
    params:
      document_store: DocumentStore
      top_k: 3
  - name: PromptModel
    type: PromptModel
    params:
      model_name_or_path: google/flan-t5-large
      model_kwargs:
        model_kwargs:
          num_return_sequences: 3
          num_beams: 3
  - name: PromptNode 
    type: PromptNode
    params:
      default_prompt_template: question-answering
      model_name_or_path: PromptModel
  - name: InputDocumentShaper
    type: Shaper
    params:
      func: join_documents
      inputs:
        documents: documents
      outputs:
        - documents
      publish_outputs: false # this line can save two shapers
      params:
        delimiter: " - "
  - name: InputQuestionsShaper
    type: Shaper
    params:
      func: value_to_list
      inputs:
        value: query
      outputs:
        - questions
      params:
        target_list: [1]
  - name: OutputAnswerShaper
    type: Shaper
    params:
      func: strings_to_answers
      inputs:
        strings: results
      outputs:
        - answers
pipelines:
  - name: query
    nodes:
      - name: Retriever
        inputs: [Query]
      - name: InputDocumentShaper
        inputs: [Retriever]
      - name: InputQuestionsShaper
        inputs: [InputDocumentShaper]
      - name: PromptNode
        inputs: [InputQuestionsShaper]
      - name: OutputAnswerShaper
        inputs: [PromptNode]

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 tests that demonstrate the correct behavior of the change
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

vblagoje · 2023-02-22T11:00:07Z

@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 if output_key in ["query", "file_paths", "labels", "documents", "meta"]:?

tstadel · 2023-02-22T11:03:55Z

@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 if output_key in ["query", "file_paths", "labels", "documents", "meta"]:?

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?

vblagoje · 2023-02-22T11:14:39Z

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?

ZanSara

LGTM!

tstadel · 2023-02-22T11:17:18Z

haystack/nodes/other/shaper.py

@@ -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"]:


Ok, I'll do so.

tstadel · 2023-02-22T11:16:59Z

@vblagoje running the pipeline above works. Besides that I don't have any tests. But we would be able to control this via publish_outputs, just in case.

tstadel · 2023-02-22T11:17:18Z

haystack/nodes/other/shaper.py

@@ -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"]:


Ok, I'll do so.

ZanSara · 2023-02-22T11:18:30Z

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.

tstadel · 2023-02-22T11:19:07Z

@vblagoje @ZanSara do you think we need finer-grained control about the publishing? E.g. if there is a function that returns two outputs and only one of them should be published?
For now, we don't even have such a function. But I want to here your opinions on that.

vblagoje · 2023-02-22T11:23:18Z

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.

tstadel · 2023-02-22T11:24:37Z

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?!

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

if key in run_signature_args:

But I'll add a test to be safe.

vblagoje · 2023-02-22T11:25:59Z

@vblagoje @ZanSara do you think we need finer-grained control about the publishing? E.g. if there is a function that returns two outputs and only one of them should be published? For now, we don't even have such a function. But I want to here your opinions on that.

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?

tstadel · 2023-02-22T11:42:42Z

@vblagoje @ZanSara do you think we need finer-grained control about the publishing? E.g. if there is a function that returns two outputs and only one of them should be published? For now, we don't even have such a function. But I want to here your opinions on that.

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 Union[bool, List[str]] and having True as the default value? That way you can just switch it on or off for all and if you want to have finer-grained control, you go for a list.

vblagoje · 2023-02-22T11:52:30Z

@vblagoje @ZanSara do you think we need finer-grained control about the publishing? E.g. if there is a function that returns two outputs and only one of them should be published? For now, we don't even have such a function. But I want to here your opinions on that.

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 seem intuitive that the default value (=None) would result in all outputs being published. How about converting it into Union[bool, List[str]] and having True as the default value? That way you can just switch it on or off for all and if you want to have finer-grained control, you go for a list.

Sounds good to me. I'll let @ZanSara have the last word

ZanSara · 2023-02-22T12:00:33Z

Sounds good to me too! Let's implement it that way

tstadel · 2023-02-22T13:18:59Z

@ZanSara done. I'll merge once all tests passed.

* add option to shaper and support answers * remove publish restrictions on outputs * support list

add option to shaper and support answers

826f60b

tstadel requested a review from a team as a code owner February 22, 2023 10:51

tstadel requested review from ZanSara and removed request for a team February 22, 2023 10:51

tstadel added this to the 1.14.0 milestone Feb 22, 2023

github-actions bot added topic:pipeline topic:tests labels Feb 22, 2023

ZanSara approved these changes Feb 22, 2023

View reviewed changes

tstadel commented Feb 22, 2023

View reviewed changes

remove publish restrictions on outputs

7ceb96b

tstadel added 2 commits February 22, 2023 13:46

support list

5b3f579

Merge branch 'main' into feat/shaper_publish_outputs

fa47ca0

tstadel merged commit 32b2abf into main Feb 22, 2023

tstadel deleted the feat/shaper_publish_outputs branch February 22, 2023 13:37

vblagoje pushed a commit that referenced this pull request Feb 22, 2023

fix: add option to not override results by Shaper (#4231)

27ece02

* add option to shaper and support answers * remove publish restrictions on outputs * support list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add option to not override results by `Shaper` #4231

fix: add option to not override results by `Shaper` #4231

tstadel commented Feb 22, 2023 •

edited

Loading

vblagoje commented Feb 22, 2023

tstadel commented Feb 22, 2023

vblagoje commented Feb 22, 2023

ZanSara left a comment

tstadel Feb 22, 2023

tstadel commented Feb 22, 2023

tstadel Feb 22, 2023

ZanSara commented Feb 22, 2023 •

edited

Loading

tstadel commented Feb 22, 2023

vblagoje commented Feb 22, 2023 •

edited

Loading

tstadel commented Feb 22, 2023 •

edited

Loading

vblagoje commented Feb 22, 2023

tstadel commented Feb 22, 2023 •

edited

Loading

vblagoje commented Feb 22, 2023

ZanSara commented Feb 22, 2023

tstadel commented Feb 22, 2023

fix: add option to not override results by Shaper #4231

fix: add option to not override results by Shaper #4231

Conversation

tstadel commented Feb 22, 2023 • edited Loading

Related Issues

Proposed Changes:

How did you test it?

Notes for the reviewer

Checklist

vblagoje commented Feb 22, 2023

tstadel commented Feb 22, 2023

vblagoje commented Feb 22, 2023

ZanSara left a comment

Choose a reason for hiding this comment

tstadel Feb 22, 2023

Choose a reason for hiding this comment

tstadel commented Feb 22, 2023

tstadel Feb 22, 2023

Choose a reason for hiding this comment

ZanSara commented Feb 22, 2023 • edited Loading

tstadel commented Feb 22, 2023

vblagoje commented Feb 22, 2023 • edited Loading

tstadel commented Feb 22, 2023 • edited Loading

vblagoje commented Feb 22, 2023

tstadel commented Feb 22, 2023 • edited Loading

vblagoje commented Feb 22, 2023

ZanSara commented Feb 22, 2023

tstadel commented Feb 22, 2023

fix: add option to not override results by `Shaper` #4231

fix: add option to not override results by `Shaper` #4231

tstadel commented Feb 22, 2023 •

edited

Loading

ZanSara commented Feb 22, 2023 •

edited

Loading

vblagoje commented Feb 22, 2023 •

edited

Loading

tstadel commented Feb 22, 2023 •

edited

Loading

tstadel commented Feb 22, 2023 •

edited

Loading