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

feat: Expose output_variable in PromptNode result, adjust unit tests #3892

Merged
merged 1 commit into from
Jan 26, 2023
Merged

feat: Expose output_variable in PromptNode result, adjust unit tests #3892

merged 1 commit into from
Jan 26, 2023

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Jan 19, 2023

Related Issues

Proposed Changes:

As noted by the author of #3878 the results of intermediate PromptNode(s) in the pipeline are stored under the invocation_context results variable thus making them a bit less accessible than having it ideally as they key under the root of the resulting dictionary returned to the Haystack pipeline user.

The change required is relatively simple. We elevate output_variable value under the root output dictionary returned to the Haystack pipeline user. However, we also leave it in invocation_context so other nodes down the pipeline can use it.

How did you test it?

No new tests were added but the existing tests have been adjusted to check for presence of output_variable in the resulting dictionary. We also check the value of that variable for validity.

Notes for the reviewer

There is some duplication of keys and values in the resulting dictionary. The reason for that is that we need to put key/value resulting pair in the invocation_context as well. We'll remove the duplication once we refactor pipelines inner logic and clean up pipeline run signatures.

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 vblagoje requested a review from a team as a code owner January 19, 2023 13:19
@vblagoje vblagoje requested review from masci and tstadel and removed request for a team January 19, 2023 13:19
@vblagoje vblagoje changed the title Expose output_variable in PromptNode result, adjust unit tests feat: Expose output_variable in PromptNode result, adjust unit tests Jan 19, 2023
@vblagoje
Copy link
Member Author

@masci, it would be great to include this PR soon. It seems like a relatively low-risk but meaningful improvement for users per @tstadel recommendations. LMK if you are time constrained so we can potentially find another reviewer.

Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

LGTM!

@vblagoje vblagoje merged commit b945eae into deepset-ai:main Jan 26, 2023
@vblagoje vblagoje deleted the expose_prompt_node_results branch March 31, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Results of intermediate PromptNodes in a Multi-PromptNode-Pipelines are burried
3 participants