fix: num_return_sequences should be less than num_beams, not top_k

[faaany]

Related Issues

• fixes num_return_sequences should be less than num_beams, not top_k #5279

Proposed Changes:

1. use num_beams instead of top_k and remove top_k, because the real top_k is used for top-k sampling or sampling decoding methods not beam search
2. let user set the num_return_sequences, not by us

How did you test it?

manual test

Notes for the reviewer

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 unit tests and updated the docstrings
• 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

[coveralls]

Pull Request Test Coverage Report for Build 5518507439

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 180 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.2%) to 45.011%

---

Files with Coverage Reduction	New Missed Lines	%
nodes/prompt/invocation_layer/hugging_face.py	3	91.73%
document_stores/elasticsearch/es7.py	8	80.43%
nodes/retriever/dense.py	169	26.45%

Totals
Change from base Build 5487781909:	0.2%
Covered Lines:	10379
Relevant Lines:	23059

---

💛 - Coveralls

[julian-risch]

@faaany Thank you for this contribution! The pull request looks very good to me already. Just two small things that could be improved and then we can merge it.
The warning message should include both values num_beams and num_return_sequences to make it easier for the user to understand how to set the parameters correctly.
Second, could you please add a small unit test that checks that the warning is displayed if
num_return_sequences > num_beams? You can add the test to the file test/prompt/invocation_layer/test_hugging_face.py and it should be similar to the test called test_ensure_token_limit_negative here: https://github.com/deepset-ai/haystack/blob/main/test/prompt/invocation_layer/test_hugging_face.py#L189
You shouldn't need a real model for that test so you can use mocking as in this other test:

haystack/test/prompt/invocation_layer/test_hugging_face.py

Line 254 in 08f1865

def test_constructor_with_various_kwargs(mock_pipeline, mock_get_task):

Happy to help with the mocking if that part is unclear. Just let me know. 🙂

[faaany]

done. My test run through, but I am not quite sure whether there is a better way to write this test. Feel free to leave comments, so I can update it quickly and write better unit tests in the future. Thanks a lot!

[faaany]

@julian-risch any feedback on this?

[julian-risch]

@faaany Thank you for another contribution to Haystack! The pull request looks very good to me now. 👍 I extended the tests a bit to also check that num_return_sequences is set to num_beams as expected. I also changed the docstring describing the test accordingly. The init of HFLocalInvocationLayer I was able to simplify. Have a look if you're interested. Your test was quite good already before this final polishing step. Great work! 🙂

[faaany]

@faaany Thank you for another contribution to Haystack! The pull request looks very good to me now. 👍 I extended the tests a bit to also check that num_return_sequences is set to num_beams as expected. I also changed the docstring describing the test accordingly. The init of HFLocalInvocationLayer I was able to simplify. Have a look if you're interested. Your test was quite good already before this final polishing step. Great work! 🙂

Wow, the tests look really good now! Thanks so much! Learned new stuff again. lol