- 
                Notifications
    You must be signed in to change notification settings 
- Fork 104
          Replace api_key with credentials
          #799
        
          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
base: main
Are you sure you want to change the base?
Conversation
And ignore credentials when hashing
Rather than hashing the complete provider object, we should just hash the most critical components. This trades a little safety for convenience; I don't think there are likely to be too many cases where you re-call `batch_chat()` with a subtly different provider object and this makes it more likely that you can retrieve results stored with a different version of ellmer. I've also added an escape hatch so you can use chats saved by ellmer 0.3.0 in ellmer 0.4.0 Extracted out from #799.
Rather than hashing the complete provider object, we should just hash the most critical components. This trades a little safety for convenience; I don't think there are likely to be too many cases where you re-call `batch_chat()` with a subtly different provider object and this makes it more likely that you can retrieve results stored with a different version of ellmer. I've also added an escape hatch so you can use chats saved by ellmer 0.3.0 in ellmer 0.4.0 Extracted out from #799.
| @gadenbuie this is a massive PR but you don't need to review it in detail. I think what would be most useful is if you looked at the changes to a couple of  | 
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.
Looks great! I think the reason for credentials is well-motivated and clear and the docs all hit the right balance for this feature (agreed that most people don't need to know much, but there's a good escape hatch for edge-cases).
Just a few small comments...
| #' @param model Name of the model. | ||
| #' @param base_url The base URL for the API. | ||
| #' @param params A list of standard parameters created by [params()]. | ||
| #' @param credentials A zero-argument function that returns credential to use | 
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.
| #' @param credentials A zero-argument function that returns credential to use | |
| #' @param credentials A zero-argument function that returns the credentials to use | 
| @@ -1,5 +1,6 @@ | |||
| # ellmer (development version) | |||
|  | |||
| * `chat_*()` functions now use a credentials functions instead of an `api_key` (#613). This means that the `api_key` is never stored in the provider object (which might be saved to disk), but is instead retrieved on demand as needed. You generally shouldn't need to use the `credentials` argument, but when you do, you should use it to dynamically retrieve the API key from some other source (i.e. never inline a secret directly into a function call). | |||
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.
| * `chat_*()` functions now use a credentials functions instead of an `api_key` (#613). This means that the `api_key` is never stored in the provider object (which might be saved to disk), but is instead retrieved on demand as needed. You generally shouldn't need to use the `credentials` argument, but when you do, you should use it to dynamically retrieve the API key from some other source (i.e. never inline a secret directly into a function call). | |
| * `chat_*()` functions now use a `credentials` function instead of an `api_key` (#613). This means that API keys are never stored in the chat object (which might be saved to disk), but is instead retrieved on demand as needed. You generally shouldn't need to use the `credentials` argument, but when you do, you should use it to dynamically retrieve the API key from some other source (i.e. never inline a secret directly into a function call). | 
| chat_openai_test(credentials = function() 1) | ||
| Condition | ||
| Error in `chat_openai()`: | ||
| ! `credentials()` must be a string or a named list, not the number 1. | 
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.
I don't know if you have much control over this, but given that we often reference functions like across() or pivot_wider(), it's easy to miss that in this error message credentials() means the result of evaluating credentials.
credentialsis a zero-arg callback that returns the API key (or list of other headers). It is called dynamically so that secrets are never stored in the provider object and hence can't accidentally be serialised to disk.Includes fixes for
batch_*()to improve provider hashing and provide an escape hatch if I get the hashing wrong.Fixes #613
To do:
as_credentials(api_key, credentials, function_name)ellmer_req_credentials()in all providers and extend it to optionally take just a string too (need to supply header name in argument)batch_chat_*()#802 is merged