Skip to content

feat: Support calling the OpenAI API directly - #85

Merged
Daniel Ciborowski (dciborow) merged 7 commits into
microsoft:mainfrom
msnidal:msnidal/openai-api-fixes
May 15, 2023
Merged

Daniel Ciborowski (dciborow) merged 7 commits into
microsoft:mainfrom
msnidal:msnidal/openai-api-fixes

Conversation

@msnidal

Copy link
Copy Markdown
Contributor

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 Mark Snidal (msnidal) changed the title feat:Better support for calling the OpenAI API directly feat: Support calling the OpenAI API directly May 13, 2023

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Comment thread src/gpt_review/_llama_index.py Resolved
Comment thread src/gpt_review/_llama_index.py Resolved
Comment thread src/gpt_review/_llama_index.py Resolved
Comment thread src/gpt_review/_llama_index.py Resolved
Comment thread src/gpt_review/_review.py Resolved
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented May 13, 2023 •

Copy link
Copy Markdown

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
Copy Markdown
Collaborator

Mark Snidal (@msnidal) , looks great! Will get this sorted out and merged in.

@dciborow

Copy link
Copy Markdown
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.

Comment thread src/gpt_review/constants.py Resolved
Comment thread src/gpt_review/context.py Resolved
Comment thread src/gpt_review/context.py Resolved
Comment thread src/gpt_review/context.py Resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Comment thread src/gpt_review/context.py Resolved
@dciborow

Copy link
Copy Markdown
Collaborator

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

@dciborow
Daniel Ciborowski (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