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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

added llm_base_url to llm.client.__init__.py #1963

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

osok
Copy link

@osok osok commented Jun 21, 2024

Description

Related Issue

Type of Change

  • 馃摎 Examples / docs / tutorials / dependencies update
  • [ X] 馃敡 Bug fix (non-breaking change which fixes an issue)
  • 馃 Improvement (non-breaking change which improves an existing feature)
  • 馃殌 New feature (non-breaking change which adds functionality)
  • 馃挜 Breaking change (fix or feature that would cause existing functionality to change)
  • 馃攼 Security fix

Checklist

  • [X ] I've read the CODE_OF_CONDUCT.md document.
  • [ X] I've read the CONTRIBUTING.md guide.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.
  • I've updated the pdm.lock running pdm update-lock (only applicable when pyproject.toml has been
    modified)

I'm new to contributing. I didnt write test cases, I"m not sure how to. The change is like ~10 lines of code. I do realize even one line of code can break a release. I sorted out the fix for the problem I was having and at least wanted to provide something to make things easier.

If there is a resource for how to properly work on python projects like this and build test cases, I'm eager to review it. I'm used to Java projects from like 15 years ago when I did a lot more team development.

Copy link

sentry-io bot commented Jun 21, 2024

馃攳 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

馃搫 File: giskard/llm/client/init.py

Function Unhandled Issue
get_default_client OpenAIError: The api_key client option must be set either by passing api_key to the client or by setting the O... ...
Event Count: 8
get_default_client AttributeError: 'ChatMessage' object has no attribute 'get' main ...
Event Count: 3
get_default_client LLMImportError: It seems that you are using Giskard LLM functionality but you are missing some required package. ... ...
Event Count: 1
get_default_client NameError: name 'data' is not defined main in...
Event Count: 1

Did you find this useful? React with a 馃憤 or 馃憥

@kevinmessiaen
Copy link
Member

Thanks for your contribution!

I've done minor change due to lint error and some issue in the set_llm_base_url method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants