Skip to content

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.

@sentry
Copy link

sentry 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.

@kevinmessiaen kevinmessiaen enabled auto-merge July 3, 2024 05:06
@kevinmessiaen kevinmessiaen disabled auto-merge July 3, 2024 05:16
@kevinmessiaen kevinmessiaen merged commit 485c77a into Giskard-AI:main Sep 2, 2024
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.

3 participants