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

fix: Support isolated node eval in run_batch in Generators #5291

Merged
merged 2 commits into from
Jul 7, 2023

Conversation

bogdankostic
Copy link
Contributor

Related Issues

Proposed Changes:

This PR adds the parameter add_isolated_node_eval to the run_batch method of the BaseGenerator. This is needed for cases where a user calls eval_batch on a Pipeline with add_isolated_node_eval=True. Previously, this parameter was just ignored.

How did you test it?

I added two unit tests, one for run and one for run_batch.

Notes for the reviewer

I moved the EVAL_LABELS from test_eval.py to a fixture in conftest.py to be able to use them in test_generator.py.

Checklist

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 5476609584

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 20 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 44.712%

Files with Coverage Reduction New Missed Lines %
nodes/answer_generator/base.py 20 54.88%
Totals Coverage Status
Change from base Build 5474864964: 0.06%
Covered Lines: 10294
Relevant Lines: 23023

💛 - Coveralls

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! 👍 The unit tests could have maybe checked whether the answer from the mocked prediction appears in the result. Right now it's basically just answers_isolated in result so the check could be more detailed.
I saw that you updated the related issue, which is good. 👍 For Generators I consider the problem fixed. Let's see whether we can add the functionality for PromptNode too in the next sprint.
Thank you for the extra effort in this sprint to reduce our backlog! 🙌

@bogdankostic bogdankostic merged commit 0697f5c into main Jul 7, 2023
60 checks passed
@bogdankostic bogdankostic deleted the isolated_node_eval_generators branch July 7, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants