AWS Bedrock Support #670
AWS Bedrock Support #670
Conversation
gptel-bedrock.el
Outdated
| @@ -0,0 +1,586 @@ | |||
| ;;; gptel-bedrock.el --- AWS Bedrock support for gptel -*- lexical-binding: t; -*- | |||
|
|
|||
| ;; Copyright (C) 2024 [Your Name] | |||
There was a problem hiding this comment.
By 2025, all files in the project appear to have copyright attributed to the main author, @karthink. This structure could facilitate any future changes in licensing or transfers of copyright to an organization like the FSF, if he so decides.
Additionally, ensure that the Author field is accurate or omitted if anonymity is preferred?
| "Parse TOOLS and return a list of ToolSpecification objects. | ||
|
|
||
| TOOLS is a list of `gptel-tool' structs, which see." | ||
| ;; https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ToolSpecification.html |
There was a problem hiding this comment.
How about having these URLs in the docstring instead? (of course longer than default byte-compile-docstring-max-column which might need adjusting in file local variables)
There was a problem hiding this comment.
The URLs I think are more developer docs no?
gptel-bedrock.el
Outdated
| (require 'gptel-anthropic) | ||
| (require 'mail-parse) | ||
|
|
||
| (defvar json-object-type) |
There was a problem hiding this comment.
Hmm, why exactly is defvar this needed? Is json.el used at all?
There was a problem hiding this comment.
I think it's used when native JSON parsing isn't compiled in. But the gptel--json* macros all defvar whatever they need, so I don't think this is necessary after all. I've removed
| :protocol protocol | ||
| :endpoint "" ; Url is dynamically constructed based on other args | ||
| :stream stream | ||
| :coding-system (and stream 'binary) |
There was a problem hiding this comment.
Compiler-macro error for gptel--make-bedrock: (error "Keyword argument :coding-system not one of (:name :host :header :protocol :stream :endpoint :key :models :url :request-params :curl-args)")
There was a problem hiding this comment.
How are you triggering this error? I'm not able to reproduce this on an emacs -Q, but I see something similar when I've loaded the previous gptel-openai.el without the new field
|
Thank you for doing this! Excited to try it out.
UPDATE: I think I had something hanging around my environment. After nuking everything, restarting, then manually eval'ing each file things seem to work. I'm now getting HTTP 403s saying the request signature is invalid. Turning logging on, I see: I'm using I was able to fix that by switching to homebrew's version |
|
An easy way to test if your curl -XPOST -d'{"messages":[{"role":"user","content":[{"text":"Who are you?"}]}]}' \
--user "$AWS_ACCESS_KEY_ID:${AWS_SECRET_ACCESS_KEY}" "-Hx-amz-security-token: ${AWS_SECURITY_TOKEN}" \
--aws-sigv4 "aws:amz:us-east-1:bedrock" --output "/dev/stdout" \
'https://bedrock-runtime.us-east-1.amazonaws.com/model/meta.llama3-8b-instruct-v1:0/converse' \
| jq -r '.output.message.content[0].text'You'll need to ensure your account has access to the model first |
|
Just to be clear - my curl had support for AWS, and I had access. The issue was with the macOS version, as updating to the homebrew version fixed the problem. I'll leave my error message in this thread so others can find it with a search. Things appear to be working for me now. Thanks again. |
|
|
||
| (defvar gptel--bedrock-models | ||
| (let ((known-ids (mapcar #'car gptel-bedrock-model-ids))) | ||
| (cl-remove-if-not (lambda (model) (memq (car model) known-ids)) gptel--anthropic-models)) |
There was a problem hiding this comment.
Do you mean why re-use the anthropic model descriptions? Just because it felt silly to duplicate those here
There was a problem hiding this comment.
Yes exactly.
My rationale was as follows:
Bedrock has lots of model families (Claude, Jamba, Titan, Llama, Mistral, etc.), and their offering is constantly evolving. In fact, users can create custom models (through fine-tuning, distillation, or more pre-training), so the list of models is not really finite. So the situation for this backend is somewhere between Ollama (where gptel doesn't provide a default model list) and Anthropic (where gptel provides a comprehensive model list).
I therefore decided to provide a default model list which was useful (the Anthropic subset that's served by AWS) but not exhaustive. I could be convinced to add more model families to the default list; perhaps Llama and Mistral?
|
Would you be willing to add something like this to the PR? (defun gptel-bedrock--get-credentials-from-env ()
"Return AWS credentials from environment variables.
Returns a list of 2-3 elements, depending on whether a session
token is needed, with this form: (AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY
AWS_SESSION_TOKEN)."
(let ((key-id (getenv "AWS_ACCESS_KEY_ID"))
(secret-key (getenv "AWS_SECRET_ACCESS_KEY"))
(token (getenv "AWS_SESSION_TOKEN")))
(cond
((and key-id secret-key token) (cl-values key-id secret-key token))
((and key-id secret-key) (cl-values key-id secret-key))
(t (user-error "Missing AWS credentials; currently only environment variables are supported")))))
(defcustom gptel-bedrock-credentials-function #'gptel-bedrock--get-credentials-from-env
"Function to get AWS credentials for Bedrock API requests.
This function should return a list of 2-3 elements:
(AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY [AWS_SESSION_TOKEN])
where the session token is optional.
By default, credentials are read from environment variables."
:group 'gptel
:type '(choice (function :tag "Credential function")
(const :tag "No credentials" nil)))
(defun gptel-bedrock--get-credentials ()
"Return the AWS credentials to use for the request.
Calls `gptel-bedrock-credentials-function' to retrieve credentials.
Convenient to use with `cl-multiple-value-bind'."
(if gptel-bedrock-credentials-function
(funcall gptel-bedrock-credentials-function)
(user-error "No credential function configured for AWS Bedrock"))) |
I was envisioning adding support for more credential sources in a follow up PR. I'd like to either use the AWS CLI or at least replicate the logic for where it looks for credential sources. With the current code, it doesn't seem like the |
|
I don't think the AWS CLI lets you print credentials - probably for security reasons. So I think we're either forced to write/reuse some elisp to replicate the functionality or have the impl use some thing other than curl that does it for us.
Fair point, provided the fn name is stable. |
We could rename to
That's my assumption. Selfishly I'd like to make it easy to use aws-vault, which seems easier to do if the credential system is written in elisp 😄 |
|
I haven't looked at It looks like you're only modifying Curl processes. gptel requires parity between (non-streaming) Curl requests and url-retrieve, so the requisite changes need to be made in |
| @@ -240,9 +240,14 @@ all at once. This wait is asynchronous. | |||
| :type 'boolean) | |||
| (make-obsolete-variable 'gptel-playback 'gptel-stream "0.3.0") | |||
|
|
|||
| (defcustom gptel-use-curl (and (executable-find "curl") t) | |||
There was a problem hiding this comment.
Why rename this defcustom? If you have multiple versions of curl on your path, you can set Emacs' process environment appropriately to pick the right one.
There was a problem hiding this comment.
I went off the discussion at #145
My gut reaction as someone whose platform curl doesn't support AWS signatures is that messing with process path is kind of painful to do. I would probably just put an updated curl in ~/.local/bin and accept that curl is de facto no longer managed by my distro, but that seems kind of icky
| @@ -1556,41 +1561,43 @@ implementation, used by OpenAI-compatible APIs and Ollama." | |||
| :name (gptel-tool-name tool) | |||
| :description (gptel-tool-description tool)) | |||
| (and (gptel-tool-args tool) ;no parameters if args is nil | |||
| (list | |||
There was a problem hiding this comment.
Could you explain why you split this out into a separate function? Since this is a cl-defmethod, it's useful to keep it self-contained. gptel--tool-args-to-json-schema does not indicate that it only applies to OpenAI and Ollama, for instance.
There was a problem hiding this comment.
It's a fairly generic piece of functionality that would be useful across many backends:
- The specification for the function "convert args from the gptel format to JSON schema" makes no reference to any backend
- OpenAI, Ollama, Bedrock, and even Anthropic could use this helper (once the
:additionalPropertiesTODO is resolved). I imagine there are many other providers using JSON schema for tool definitions. (Gemini uses OpenAPI schema so can't benefit from this helper)
If there were a gptel-utils, this is the sort of thing that could live there.
There was a problem hiding this comment.
It's a fairly generic piece of functionality that would be useful across many backends:
Each backend uses a different JSON scheme. Compare this implementation of gptel--parse-tools with that in gptel-anthropic.el and gptel-gemini.el for example.
So this version of gptel--tool-args-to-json-schema is not useful when separated from its caller.
There was a problem hiding this comment.
Each backend uses a different JSON scheme
They differ in how the "top" of the tool spec is structured, but all except Gemini use the same format for the arguments. JSON schema is a standard independent of the LLM provider, which OpenAI, Ollama, Bedrock, and Anthropic have all chosen to use.
There was a problem hiding this comment.
But anyway, this isn't the hill for me to die on. If you'd rather I just copy-paste the code I can do that. LMK what you prefer
There was a problem hiding this comment.
but all except Gemini use the same format for the arguments. JSON schema is a standard independent of the LLM provider, which OpenAI, Ollama, Bedrock, and Anthropic have all chosen to use.
I don't trust them to follow the standard. When I was adding tool support, I saw a large amount of ill-defined behavior and variance between the backends in what they accept as tool arguments. Tool use for every backend is an under-specified mess right now. So even though they look similar, each backend's tool argument parser is tested specifically for that backend type, and there will be many edge cases if we assume we can reuse gptel--tool-args-to-json-schema across backends.
So please leave the current version unaltered and copy what you need into gptel-bedrock's implementation of gptel--parse-tools. I will revisit this issue later this year after both the upstream tools API and gptel's tool support have had more time to stabilize.
| @@ -151,7 +151,8 @@ Throw an error if there is no match." | |||
| (:copier gptel--copy-backend)) | |||
| name host header protocol stream | |||
| endpoint key models url request-params | |||
| curl-args) | |||
| curl-args | |||
| (coding-system nil :documentation "Can be set to `binary' if the backend expects non UTF-8 output.")) | |||
There was a problem hiding this comment.
The documentation for this can be more general. Perhaps something like:
"The coding system used by the backend, defaults to utf-8-unix. Does not apply to Windows systems."
There was a problem hiding this comment.
Currently we only handle the case where (eq (gptel-backend-coding-system backend) 'binary). If we want to support any coding system, do we want to support separate encoding and decoding systems? Or alternatively we could rename this field to something like use-binary-coding to keep the options as utf8 or binary
There was a problem hiding this comment.
You're right. Let's leave it at coding-system and with your current documentation. Supporting any coding system is a messy affair because of Windows.
|
@felipeochoa Here's where we are on merging this:
|
|
I rebased this PR on master and things still work. While doing that, I also thought about |
This is useful for the bedrock implementation since some distros' version of curl doesn't have support for the --aws-sigv4 argument
This will allow the bedrock implementation to accept responses using vnd.amazon.event-stream
Discussion here. LMK how you want to proceed
Done. I put in
Will do as soon as we decide on point 1 |
| (const t :tag "Use the system curl") | ||
| (string :tag "Path to the curl executable"))) | ||
|
|
||
| (defsubst gptel-curl-path () |
There was a problem hiding this comment.
Rename to gptel--curl-path to signal that this is internal.
| :endpoint "" ; Url is dynamically constructed based on other args | ||
| :stream stream | ||
| :coding-system (and stream 'binary) | ||
| :curl-args (lambda () (gptel-bedrock--curl-args region)) |
There was a problem hiding this comment.
This will mean that the user can't add any more backend-specific Curl arguments. For example, if they want to specify --max-time or --keepalive-time time for this backend.
|
@felipeochoa gentle ping about the last few remaining issues to merge this. |
Sorry been quite busy at work. I'll probably get to this later this month |
|
@karthink @felipeochoa I would like to thank gptel and this PR. I think that gptel is the most emacs way client in various other clients for emacs. I want to feed privacy information to gptel via AWS Bedrock, and this PR enables this. A few things I noticed:
|
|
Thank you @karthink @felipeochoa for your efforts on adding AWS Bedrock support, this is very useful feature! Something I thought I'd share, in case it's helpful for others too:
|
Closes #379
Use it with this:
I've manually tested with streaming on and off. Tool use, media, and context all work