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.

Sign up for GitHub

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

Jump to bottom

fix: PromptNode run raises on empty inputs #7734

Merged
merged 3 commits into from
May 24, 2024
Merged

fix: PromptNode run raises on empty inputs #7734

merged 3 commits into from
May 24, 2024

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented May 23, 2024 •
edited
Loading

Related Issues

  • fixes raising the following error if running prompt_node.run(query="", documents=documents) 
    Exception while running node 'PromptNode': Expected prompt parameters ['documents', 'query'] to be provided but got only 
['documents']. Make sure to provide all template parameters.
Enable debug logging to see the data that was passed when the pipeline failed.

Proposed Changes:

  • include any variable into invocation_context that is not None

How did you test it?

  • added test

Notes for the reviewer

Checklist

@tstadel tstadel requested a review from a team as a code owner May 23, 2024 10:34
@tstadel tstadel requested review from julian-risch and removed request for a team May 23, 2024 10:34
@tstadel tstadel requested a review from a team as a code owner May 23, 2024 10:38
@tstadel tstadel requested review from dfokina and removed request for a team May 23, 2024 10:38
@tstadel tstadel added the 1.x label May 23, 2024
julian-risch
julian-risch requested changes May 23, 2024
View reviewed changes
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

The changes in haystack/nodes/prompt/prompt_node.py look good to me. I don't see a need for an integration test and suggest we create a unit test instead without using an actual language model. What do you think?

test/prompt/test_prompt_node.py Outdated
@@ -1212,3 +1212,22 @@ def test_prompt_no_truncation(mock_model, caplog):
with caplog.at_level(logging.DEBUG):
_ = node.prompt(PromptTemplate(prompt))
assert prompt in caplog.text


@pytest.mark.integration
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need a real model to test that the invocation_context is set as expected. Let's not use google/flan-t5-base (that's what prompt_model "hf" gets) to check this. Instead let's mock the model and make it a unit test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I updated the test :-)

@tstadel tstadel requested a review from julian-risch May 24, 2024 12:34
julian-risch
julian-risch approved these changes May 24, 2024
View reviewed changes
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for updating the PR. 👍

@tstadel tstadel merged commit 9b7f9fb into v1.x May 24, 2024
53 checks passed
@tstadel tstadel deleted the fix/promptnode_w_empty_inputs branch May 24, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julian-risch julian-risch julian-risch approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

Assignees
No one assigned
Labels
1.x topic:LLM topic:promptnode topic:tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tstadel @julian-risch