Skip to content

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

Conversation

Gable-github
Copy link

@Gable-github Gable-github commented May 23, 2025

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:

  1. Vertexai
  • it does this
if api_key and "api_key" not in kwargs:
                kwargs["api_key"] = api_key

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 the api_key variable. Doing it the bot's way would pass the api_key into from_genai() which is not what we want.

  • vertex also uses the Client which takes in an api_key and handles it in the package.
  1. Perplexity
  • Its handling both cases where api_key is in **kwargs but I think its a wrong way to handle it. I handled it in the case where api_key is not in **kwargs.
  1. generative-ai
  • I've updated it to handle api_key according to this documentation

Other change descriptions:

  • Made error message for missing 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

  • I have performed a self-review of my code

Important

Adds api_key parameter to from_provider() in instructor/auto_client.py, supporting direct API key input for multiple providers and correcting handling for VertexAI and Perplexity.

  • Behavior:
    • Adds api_key parameter to from_provider() in instructor/auto_client.py to allow direct API key input.
    • Corrects api_key handling for VertexAI and Perplexity providers.
    • Updates error messages for missing api_key in Mistral and Perplexity.
  • Providers:
    • OpenAI, Anthropic, Google, Mistral, Cohere, Perplexity, Groq, Writer, Bedrock, Cerebras, Fireworks, VertexAI, Generative-AI now support api_key parameter.
  • Misc:
    • Updates from_provider() docstring to include api_key parameter description.

This description was created by Ellipsis for 255810d. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 85% 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% <= threshold 85% 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% <= threshold 85% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@jxnl
Copy link
Collaborator

jxnl commented May 23, 2025

shouldn't it just be in **kwargs and passed into all client initiliazation?

@Gable-github
Copy link
Author

Thanks for the feedback! I considered handling api_key via **kwargs and extracting it as needed (e.g., api_key = kwargs.pop('api_key', None), but in practice, we still need to check for its presence before passing it to each client’s initializer.

The main reason for keeping api_key as an explicit keyword argument is that not all provider clients accept api_key or arbitrary keyword arguments (boto3). Passing **kwargs to every client can possibly raise runtime TypeErrors if a client does not expect certain arguments. I opted to explicitly pass only the arguments that each client supports.

I did notice that the vertexai provider is an exception it appears to accept additional keyword arguments. Would you prefer that I use **kwargs throughout for consistency, update the vertexai initialization to be more explicit (no args passed into it like the other providers), or keep the current approach?

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!

@ivanleomk
Copy link
Collaborator

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 Cairo, I normally find that I have to set this path

export DYLD_LIBRARY_PATH=/opt/homebrew/lib

@ivanleomk
Copy link
Collaborator

@Gable-github thanks for this PR!

@Gable-github
Copy link
Author

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

@jxnl jxnl enabled auto-merge (squash) June 3, 2025 17:28
@jxnl jxnl closed this Jun 27, 2025
auto-merge was automatically disabled June 27, 2025 18:14

Pull request was closed

@Gable-github
Copy link
Author

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

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