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: Support calling the OpenAI API directly #85

Merged
merged 7 commits into from
May 15, 2023

Conversation

msnidal
Copy link
Contributor

@msnidal msnidal commented May 13, 2023

Description

Support for going through the OpenAI API directly in addition to the existing Azure support (API and secrets). Also moves to the model notation as the engine param is deprecated and failed when hitting the OpenAI API directly.

Updates the README with more clarity on the order of operations

Testing

TODO

Additional context

I dont work at microsoft nor have an Azure account so I'm not sure how the engine vs. model distinction works on a custom Azure OpenAI endpoint 🤔

@msnidal msnidal changed the title feat:Better support for calling the OpenAI API directly feat: Support calling the OpenAI API directly May 13, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Summary by GPT-4

In this commit, the README.md file has been updated to provide clearer instructions on how to install and set up the CLI tool. The _llama_index.py file has been updated to support both Azure OpenAI and OpenAI API types. The _openai.py file has been updated to use the term "model" instead of "engine" for consistency. The context.py file has been updated to load context from environment variables or context files, with precedence given to config files if available. The test files have also been updated accordingly.

Overall, these changes improve the clarity of instructions and provide better support for different API types.

Suggestions

Here are some suggestions for improving the changes in this PR:

  1. In the README.md file, change the title "How to install CLI" to "How to Install CLI" for consistency in capitalization.

  2. In the _llama_index.py file, add a comment explaining the difference between using Azure OpenAI API and OpenAI API directly in the _load_service_context() function.

  3. In the context.py file, update the docstring of _load_azure_openai_context() function to mention that it also checks for OPENAI_API_KEY environment variable if Azure OpenAI API is not available.

  4. In the test_gpt_cli.py file, update the import statement to follow PEP8 guidelines:

    from gpt_review._gpt_cli import cli
    import gpt_review.constants as C
  5. In the test_openai.py file, update function names and test names to reflect changes from "engine" to "model":

    • Rename get_engine_test() to get_model_test()
    • Rename test_get_engine() to test_get_model()
    • Rename test_int_get_engine() to test_int_get_model()
  6. Update comments and docstrings throughout the codebase where "engine" is mentioned but should now be "model".

@msnidal
Copy link
Contributor Author

msnidal commented May 13, 2023

@microsoft-github-policy-service agree

src/gpt_review/_llama_index.py Outdated Show resolved Hide resolved
src/gpt_review/_llama_index.py Outdated Show resolved Hide resolved
src/gpt_review/_llama_index.py Outdated Show resolved Hide resolved
src/gpt_review/_llama_index.py Outdated Show resolved Hide resolved
src/gpt_review/_review.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2023

Codecov Report

Merging #85 (4611742) into main (60c0d8e) will decrease coverage by 6.52%.
The diff coverage is 63.15%.

❗ Current head 4611742 differs from pull request most recent head b63415e. Consider uploading reports for the commit b63415e to get more accurate results

@@             Coverage Diff             @@
##              main      #85      +/-   ##
===========================================
- Coverage   100.00%   93.48%   -6.52%     
===========================================
  Files           13       13              
  Lines          462      476      +14     
  Branches        67       74       +7     
===========================================
- Hits           462      445      -17     
- Misses           0       22      +22     
- Partials         0        9       +9     
Flag Coverage Δ
integration ?
unittests 93.06% <63.15%> (-3.91%) ⬇️
unittests-3.10 88.65% <63.15%> (-3.77%) ⬇️
unittests-3.11 88.65% <63.15%> (-3.77%) ⬇️
unittests-3.8 88.65% <63.15%> (-3.77%) ⬇️
unittests-3.9 88.65% <63.15%> (-3.77%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/gpt_review/context.py 60.00% <26.66%> (-40.00%) ⬇️
src/gpt_review/_llama_index.py 82.69% <62.50%> (-17.31%) ⬇️
src/gpt_review/_ask.py 100.00% <100.00%> (ø)
src/gpt_review/_git.py 100.00% <100.00%> (ø)
src/gpt_review/_github.py 100.00% <100.00%> (ø)
src/gpt_review/_gpt_cli.py 100.00% <100.00%> (ø)
src/gpt_review/_openai.py 81.25% <100.00%> (-18.75%) ⬇️
src/gpt_review/_review.py 100.00% <100.00%> (ø)
src/gpt_review/constants.py 100.00% <100.00%> (ø)
src/gpt_review/prompts/_prompt.py 100.00% <100.00%> (ø)

@dciborow
Copy link
Collaborator

@msnidal , looks great! Will get this sorted out and merged in.

@dciborow
Copy link
Collaborator

I realize now my isort has not been working. I am going to create a new PR that just takes care of all the import fixes.

src/gpt_review/context.py Outdated Show resolved Hide resolved
src/gpt_review/context.py Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/gpt_review/context.py Outdated Show resolved Hide resolved
@dciborow
Copy link
Collaborator

i was expecting the tests to run using my repos creds.... so that the tests would run properly.

@dciborow dciborow self-requested a review May 15, 2023 20:24
@dciborow dciborow merged commit ae385eb into microsoft:main May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants