-
-
Notifications
You must be signed in to change notification settings - Fork 836
feat: implemented support for accepting api_key in form_provider #1556
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: implemented support for accepting api_key in form_provider #1556
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 255810d in 2 minutes and 1 seconds. Click for details.
- Reviewed
288
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
7
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/auto_client.py:268
- Draft comment:
In the Perplexity provider block, the client is instantiated using 'api_key' directly rather than the computed 'perplexity_api_key'. This may bypass the API key from the environment; please update to use 'perplexity_api_key'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. instructor/auto_client.py:215
- Draft comment:
The error message for missing MISTRAL_API_KEY lacks a space between the concatenated strings. Consider adding a space (or reformatting) to improve readability. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
3. instructor/auto_client.py:116
- Draft comment:
The pattern of checking if api_key is provided and then choosing between async and sync client instantiation is duplicated across many provider blocks. Consider refactoring this logic to adhere to the DRY principle. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold85%
None
4. instructor/auto_client.py:399
- Draft comment:
The VertexAI provider block does not use the provided 'api_key'. If this is intentional due to how the underlying package handles authentication, please document this behavior; otherwise consider implementing support. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold85%
None
5. instructor/auto_client.py:216
- Draft comment:
Typographical error: The concatenation of the two string literals in the ValueError message will result in the strings being merged without an intervening space. Consider adding a space either at the end of the first string or at the beginning of the second string. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 85% This is a real issue in code that was changed in this PR (the api_key parameter addition affected this error message). The strings would concatenate as "...MISTRAL_API_KEY is not set.Set it with..." and "...your-api-key>or pass it..." without proper spacing. This affects readability of error messages which is important for user experience. The issue is minor and doesn't affect functionality. The error message would still be understandable even without proper spacing. While minor, proper formatting of error messages is important for professional quality code, and this is a trivial fix that improves user experience. Keep the comment as it identifies a real formatting issue in changed code that should be fixed for better error message readability.
6. instructor/auto_client.py:265
- Draft comment:
Typographical note: The concatenated error message might be missing a space between the two string literals. Consider adding a trailing space in the string on line 264 or a leading space on line 265 to ensure proper separation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% While technically correct, this is an extremely minor formatting issue in an error message. The message is still readable and understandable without the extra space. The rules state not to make comments that are obvious or unimportant. This feels like a very low-priority nitpick that doesn't meaningfully impact code quality or functionality. The error message readability could be slightly impacted without proper spacing, and consistency in error message formatting is a valid concern. However, the impact is minimal - the message is still perfectly understandable, and this level of nitpicking about spaces in error messages doesn't meet the bar for "clearly a code change required" as specified in the rules. Delete this comment as it's too minor and doesn't meet the threshold for requiring a code change.
7. instructor/auto_client.py:393
- Draft comment:
Typo: There's an unnecessary trailing space at the end of the error message string. Consider removing the extra space after 'provider.'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_CPdqAufr6qN3NiuK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
shouldn't it just be in **kwargs and passed into all client initiliazation? |
Thanks for the feedback! I considered handling The main reason for keeping I did notice that the vertexai provider is an exception it appears to accept additional keyword arguments. Would you prefer that I use Please let me know if you’d like me to make this change or how you would want me to approach it in some way that i didnt realise! |
Ok I've cleaned this up a fair bit. It seems like vertexai with genai does not work even on their own docs so not going to worry about that. Would it be possible to help update the docs with this new API key? You should be able to run this locally with mkdocs serve. If you get some errors with export DYLD_LIBRARY_PATH=/opt/homebrew/lib |
@Gable-github thanks for this PR! |
…ified-provider-interface and integrations/index
i think there are other places i could have added this (individual provider pages, advanced examples, etc) but maybe better to do a follow-up docs PR for the rest? didn't want to make this one huge and hard to review / too out of scope |
Pull request was closed
I noticed my PR was eventually closed after some time, and as of right now I'm not sure if there was anything specific I could have done differently. Totally understand if my code wasn't up to par. But I’m eager to learn and try contributing again in the future. Would you be able to share any feedback or point me toward what I could improve on next time? I really appreciate your time and any guidance you can offer @jxnl |
This PR adds api_key parameter to from_provider function
Description:
This PR addresses issue #1542 by adding an api_key parameter to the from_provider function. This allows users to provide API keys directly to the function rather than having to set them in environment variables or pass them to the client directly.
Yes its the same description as the bot PR. However I noticed some possible errors in its implementation, specifically for these providers:
and then passes the
**kwargs
to the genai.Client object which is wrong.**kwargs
is only supposed to be passed to the from_genai() function where it dosen't include theapi_key
variable. Doing it the bot's way would pass theapi_key
into from_genai() which is not what we want.api_key
and handles it in the package.**kwargs
but I think its a wrong way to handle it. I handled it in the case whereapi_key
is not in**kwargs
.api_key
according to this documentationOther change descriptions:
api_key
arguments that we have to handle ourselves in this file clearer.Apologies if I failed to follow any convention and I look forward your kind comments/review :)
Issue ticket number and link
#1542
Checklist before requesting a review
Important
Adds
api_key
parameter tofrom_provider()
ininstructor/auto_client.py
, supporting direct API key input for multiple providers and correcting handling for VertexAI and Perplexity.api_key
parameter tofrom_provider()
ininstructor/auto_client.py
to allow direct API key input.api_key
handling for VertexAI and Perplexity providers.api_key
in Mistral and Perplexity.api_key
parameter.from_provider()
docstring to includeapi_key
parameter description.This description was created by
for 255810d. You can customize this summary. It will automatically update as commits are pushed.