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

feat: add and expose api_params for OpenAIGenerator in LLMEvaluator based classes #7987

Merged
merged 6 commits into from
Jul 11, 2024
Merged

feat: add and expose api_params for OpenAIGenerator in LLMEvaluator based classes #7987

merged 6 commits into from
Jul 11, 2024

Conversation

lbux
Copy link
Contributor

@lbux lbux commented Jul 6, 2024

Related Issues

Proposed Changes:

The general change pertains to allowing for OpenAIGenerator parameters to be set when initializing any of the LLMEvaluators. One of the benefits to this is the api_base_url that allows us to set a local host (or remote host) where we serve a custom model instead of sending it to OpenAI. This, then, allows for "local" evaluation.

How did you test it?

I modified some of the tests and added a test for api_base_url.

Notes for the reviewer

For the classes that build upon LLMEvaluator, I did not serialize api_params. Since we are just passing it to the .super () Should I do so?

I am unsure of how to add a test for api_base_url when running it since we would need to have a server in the CI. I can confirm that it works locally when I spin up my own server, but I don't know if that's possible with Haystack's CI.

The test would be something like:

def test_run_with_base_url(self):
        component = LLMEvaluator(
            instructions="test-instruction",
            api_key=Secret.from_token("test-api-key"),
            api_params={"model": "phi3:mini", "api_base_url": "http://localhost:11434/v1"},
            inputs=[("predicted_answers", List[str])],
            outputs=["custom_score"],
            api="openai",
            examples=[
                {
                    "inputs": {"predicted_answers": "Damn, this is straight outta hell!!!"},
                    "outputs": {"custom_score": 1},
                },
                {
                    "inputs": {"predicted_answers": "Football is the most popular sport."},
                    "outputs": {"custom_score": 0},
                },
            ],
        )
        test_inputs = {
        "predicted_answers": [
            "Damn, this is straight outta hell!!!",
            "Football is the most popular sport."
            ]
        }
        component.run(**test_inputs)

Checklist

@lbux lbux requested review from a team as code owners July 6, 2024 20:43
@lbux lbux requested review from dfokina and julian-risch and removed request for a team July 6, 2024 20:43
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Jul 6, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jul 6, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9821988850

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 90.009%
Files with Coverage Reduction New Missed Lines %
components/evaluators/llm_evaluator.py 6 94.96%
Totals Coverage Status
Change from base Build 9804009410: 0.006%
Covered Lines: 6775
Relevant Lines: 7527
💛 - Coveralls

@julian-risch julian-risch requested review from shadeMe and removed request for julian-risch July 7, 2024 19:56
@julian-risch
Copy link
Member

@shadeMe Could you please take over the review for this PR?

@shadeMe shadeMe self-assigned this Jul 8, 2024
shadeMe
shadeMe requested changes Jul 9, 2024
View reviewed changes
Copy link
Collaborator

@shadeMe shadeMe left a comment

Choose a reason for hiding this comment

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

Many thanks for the PR. A couple of changes:

For the classes that build upon LLMEvaluator, I did not serialize api_params. Since we are just passing it to the .super () Should I do so?

Yes, you'll need to pass the new init paramter to the default_to/from_dict functions.

I am unsure of how to add a test for api_base_url when running it since we would need to have a server in the CI. I can confirm that it works locally when I spin up my own server, but I don't know if that's possible with Haystack's CI.

You can add an integration test that reads the API URL from an env var, mark it with pytest.mark.skipif and test it locally.

haystack/components/evaluators/context_relevance.py Outdated Show resolved Hide resolved
haystack/components/evaluators/faithfulness.py Outdated Show resolved Hide resolved
haystack/components/evaluators/llm_evaluator.py Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Jul 9, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9890104460

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.009%) to 90.017%
Files with Coverage Reduction New Missed Lines %
components/evaluators/llm_evaluator.py 6 94.96%
Totals Coverage Status
Change from base Build 9874914711: 0.009%
Covered Lines: 6853
Relevant Lines: 7613
💛 - Coveralls

@EdoardoAbatiTR
Copy link

Thank you very much @lbux for looking into the issue!! :)

I think this currently will not fix #7946, because the AzureOpenAI client is a bit different (or am I missing something?)
I may have time tonight or tomorrow to submit a PR with my proposal, maybe it will be easier to see what I had in mind.

@lbux
Copy link
Contributor Author

lbux commented Jul 10, 2024 •
edited
Loading

Thank you very much @lbux for looking into the issue!! :)

I think this currently will not fix #7946, because the AzureOpenAI client is a bit different (or am I missing something?) I may have time tonight or tomorrow to submit a PR with my proposal, maybe it will be easier to see what I had in mind.

This is not going to 100% solve what you are asking for, but it will lay the foundation for adding like 2-3 lines of code as an else if to allow "azure" as the API (which you can make a PR for if this gets merged). api_params can handle any of the parameters defined by the Azure component and the generation_kwargs: https://docs.haystack.deepset.ai/reference/generators-api#:~:text=Module%20azure-,AzureOpenAIGenerator,-A%20Generator%20component

I didn't add the code to allow for Azure because I wanted this PR to create and test api_params. Azure support can be added after once it has been established that api_params works for OpenAI and the OpenAI based servers (which it does seem to from my testing).

In the init of LLMEval, we would simply do:

default_generation_kwargs = {"response_format": {"type": "json_object"}, "seed": 42}
user_generation_kwargs = self.api_params.get("generation_kwargs", {})
merged_generation_kwargs = {**default_generation_kwargs, **user_generation_kwargs}
self.api_params["generation_kwargs"] = merged_generation_kwargs

if api == "openai":
    self.generator = OpenAIGenerator(api_key=api_key, **self.api_params)
elif api == "azure":
    self.generator = AzureOpenAIGenerator(api_key=api_key, **self.api_params)
else:
    raise ValueError(f"Unsupported API: {api}")

@shadeMe shadeMe merged commit 6f8834d into deepset-ai:main Jul 11, 2024
17 checks passed
@lbux lbux mentioned this pull request Jul 11, 2024
Open
@lbux lbux deleted the api_params_for_eval branch July 11, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shadeMe shadeMe shadeMe approved these changes

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

Assignees

@shadeMe shadeMe

Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for AzureOpenAI in LLMEvaluator
5 participants
@lbux @coveralls @julian-risch @EdoardoAbatiTR @shadeMe