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

feat(corpora_ai): support azure_endpoint for openai as well #56

Merged
merged 7 commits into from
Nov 19, 2024
Merged

Conversation

skyl
Copy link
Owner

@skyl skyl commented Nov 19, 2024

PR Type

Enhancement, Tests


Description

  • Added support for Azure endpoints in the OpenAI client, allowing integration with Azure's OpenAI service.
  • Updated the load_llm_provider function to handle the azure_endpoint environment variable.
  • Modified tests to ensure the azure_endpoint parameter is correctly passed and handled.
  • Introduced AzureOpenAI client in llm_client.py to manage connections to Azure endpoints.

Changes walkthrough 📝

Relevant files
Enhancement
provider_loader.py
Add support for Azure endpoint in OpenAI client                   

py/packages/corpora_ai/provider_loader.py

  • Added support for azure_endpoint in the OpenAI client.
  • Improved handling of environment variables for Azure OpenAI.
  • +7/-4     
    llm_client.py
    Implement AzureOpenAI client for Azure endpoint handling 

    py/packages/corpora_ai_openai/llm_client.py

  • Introduced AzureOpenAI client for handling Azure endpoints.
  • Conditional logic to choose between OpenAI and AzureOpenAI.
  • +11/-2   
    Tests
    test_provider_loader.py
    Update tests for Azure endpoint support                                   

    py/packages/corpora_ai/test_provider_loader.py

  • Updated test to include azure_endpoint parameter.
  • Ensured mock client is called with the new parameter.
  • +5/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Nov 19, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit c105ebd)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Smell
    The use of os.getenv("OPENAI_AZURE_ENDPOINT", None) could be improved by explicitly checking for the presence of the environment variable and handling potential issues with its value.

    Code Smell
    The comment about not pinning the API version in the AzureOpenAI client initialization raises concerns about potential future compatibility issues. Consider clarifying or addressing this comment.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Verify the success of the build process before uploading the artifact

    Add a step to verify the success of the build process before uploading the artifact
    to ensure that only successful builds are stored.

    .github/workflows/ci-xcompile.yml [47-51]

     - name: Build the project
       working-directory: rs
       run: |
         rustup target add ${{ matrix.target }}
         cargo build --release --target ${{ matrix.target }}
    +    if [ $? -ne 0 ]; then exit 1; fi
    Suggestion importance[1-10]: 8

    Why: Adding a check to verify the success of the build process before uploading the artifact is a valuable enhancement. It ensures that only successful builds are stored, which can prevent issues with deploying faulty binaries.

    8
    Possible issue
    Validate the azure_endpoint before using it to initialize the AzureOpenAI client

    Ensure that the azure_endpoint is validated before using it to initialize the
    AzureOpenAI client to prevent potential misconfigurations or runtime errors.

    py/packages/corpora_ai_openai/llm_client.py [20-25]

    -if azure_endpoint:
    +if azure_endpoint and is_valid_endpoint(azure_endpoint):
         self.client = AzureOpenAI(
             api_key=api_key,
             azure_endpoint=azure_endpoint,
             # What's the behavior of not pinning the API version?
             # api_version="2024-10-01-preview",
         )
    Suggestion importance[1-10]: 7

    Why: The suggestion to validate the azure_endpoint before using it can prevent potential misconfigurations or runtime errors, enhancing the robustness of the code. However, the implementation of is_valid_endpoint is not provided, which slightly reduces the immediate applicability of the suggestion.

    7

    @skyl
    Copy link
    Owner Author

    skyl commented Nov 19, 2024

    cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR

    Could not find openssl via pkg-config:

    pkg-config exited with status code 1

    PKG_CONFIG_PATH=/usr/lib/aarch64-linux-gnu/pkgconfig/: PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 pkg-config --libs --cflags openssl

    The system library openssl required by crate openssl-sys was not found.
    The file openssl.pc needs to be installed and the PKG_CONFIG_PATH environment variable must contain its parent directory.
    PKG_CONFIG_PATH contains the following:
    - /usr/lib/aarch64-linux-gnu/pkgconfig/
    -

    HINT: you may need to install a package such as openssl, openssl-dev or openssl-devel.

    cargo:warning=Could not find directory of OpenSSL installation, and this -sys crate cannot proceed without this knowledge. If OpenSSL is installed and this crate had trouble finding it, you can set the OPENSSL_DIR environment variable for the compilation process. See stderr section below for further information.

    --- stderr

    Could not find directory of OpenSSL installation, and this -sys crate cannot
    proceed without this knowledge. If OpenSSL is installed and this crate had
    trouble finding it, you can set the OPENSSL_DIR environment variable for the
    compilation process.

    Make sure you also have the development packages of openssl installed.
    For example, libssl-dev on Ubuntu or openssl-devel on Fedora.

    If you're in a situation where you think the directory should be found
    automatically, please open a bug at https://github.com/sfackler/rust-openssl
    and include information about your system as well as this message.

    $HOST = x86_64-unknown-linux-gnu
    $TARGET = aarch64-unknown-linux-gnu
    openssl-sys = 0.9.104

    warning: build failed, waiting for other jobs to finish...
    Error: Process completed with exit code 101.

    @skyl skyl changed the title feat(ci): build binaries maybe feat(corpora_ai): support azure_endpoint for openai as well Nov 19, 2024
    @skyl
    Copy link
    Owner Author

    skyl commented Nov 19, 2024

    /describe

    @skyl
    Copy link
    Owner Author

    skyl commented Nov 19, 2024

    /review

    Copy link

    PR Description updated to latest commit (c105ebd)

    Copy link

    Persistent review updated to latest commit c105ebd

    @skyl skyl merged commit 6379ca3 into main Nov 19, 2024
    1 check passed
    @skyl skyl deleted the back-to-work branch November 19, 2024 01:11
    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.

    1 participant