Skip to content

AWS bedrock-update #1053

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Sep 27, 2025
Merged

AWS bedrock-update #1053

merged 9 commits into from
Sep 27, 2025
+322 −296

Conversation

akssri
Copy link
Contributor

@akssri akssri commented Aug 30, 2025 •
edited
Loading

There are multiple-PRs that either duplicate each other or have merge-conflicts.
This is a synthesis of all of these: #895 #925 #996 #1007 #983

What does this do ?

If the authors of these PRs can test these out, we can push this in.
(@liangfu @leezu @lgfang @andykuszyk )

This was referenced Aug 30, 2025
Closed
Closed
Closed
Closed
Closed
@karthink
Copy link
Owner

@akssri Thanks for piecing everything together! I'll wait for testing and feedback from the authors to merge this.

@karthink karthink mentioned this pull request Aug 30, 2025
Closed
lgfang
lgfang reviewed Aug 30, 2025
View reviewed changes
@andykuszyk
Copy link

@akssri I've tested this with Bedrock API keys (which was the subject of my initial PR), and it works for me 👍

Personally, I would prefer to be able to provide the API key directly in my Elisp configuration (e.g. with :token <api-key>), rather than having to set an environment variable. That being said, this PR works just fine with Bedrock API keys set via environment variables, so I would be in favour of merging it 🙂

@akssri
Copy link
Contributor Author

akssri commented Sep 5, 2025

@andykuszyk Thanks. You can set it the API directly with :aws-bearer-token argument.

@andykuszyk
Copy link

@andykuszyk Thanks. You can set it the API directly with :aws-bearer-token argument.

Ah right! I missed that! 😅

I've tested that too, and it works fine 👍

Thanks for pulling all these different approaches together into a single PR 🙏

@kotatsuyaki
Copy link

While I am not one of the authors of the PRs, I’ve tested with :aws-bearer-token and it works ;)

@leezu
Copy link
Contributor

leezu commented Sep 18, 2025

Thank you, @akssri for rebasing and merging the PRs. I've validated that both :aws-profile and AWS_BEARER_TOKEN_BEDROCK still work as expected.

@CsBigDataHub
Copy link

@karthink, friendly ping. thanks.

waiting on this for couple of weeks. thanks @akssri

@akssri
Copy link
Contributor Author

akssri commented Sep 25, 2025 via email

karthink and others added 8 commits September 26, 2025 18:19
@karthink
gptel-request: Remove gptel--wrap-user-prompt
4e947fa 
`gptel--wrap-user-prompt' is a vestige of an older context
wrapping implementation in gptel.

When `gptel-use-context' is set to 'user', we used to construct
the full messages array, and then concat the context into the last
user prompt in this array.

Post 0.9.7, gptel pastes the context text into the prompt
construction buffer instead, and lets the usual
`gptel--parse-buffer' routines handle constructing the messages
array with the context included.

So `gptel--wrap-user-prompt' has been superfluous for a while,
except for an edge case: It is used to include media (binary)
files added to gptel's global context (via `gptel-add-file', etc),
and these media files were always included with the first user
prompt in the array.

We do this even if `gptel-use-context' is set to system, since not
all backends uniformly support including documents/images in the
system prompt.

This use is regrettably still unavoidable, because we don't have a
convenient way of including an image in the prompt construction
buffer that isn't major-mode specific (such as via Org links).

In keeping with its only remaining use, rename
`gptel--wrap-user-prompt' to `gptel--inject-media' in all backend
implementations, simplify it and remove the unused code branch in
the function.

* gptel-anthropic.el (gptel--wrap-user-prompt):
(gptel--inject-media):
* gptel-bedrock.el (gptel--wrap-user-prompt):
(gptel-bedrock--inject-media-context):
(gptel--inject-media):
(gptel-bedrock--inject-text-context):
* gptel-gemini.el (gptel--wrap-user-prompt):
(gptel--inject-media):
* gptel-kagi.el (gptel--wrap-user-prompt):
(gptel-make-kagi):
* gptel-ollama.el (gptel--wrap-user-prompt):
(gptel--inject-media):
* gptel-openai.el (gptel--wrap-user-prompt):
(gptel--inject-media):
* gptel-request.el (gptel--realize-query):
(gptel--wrap-user-prompt):
@karthink
gptel-context: Change how context is specified
5e7d47f 
A common complaint abotu gptel is that the context handling does
not have a good API for programmatic usage, or for buffer-local
context management (see karthink#459, karthink#475, karthink#481, karthink#548, karthink#984 and several
more).  To make the context handling more ergonomic, we introduce
several changes to `gptel-context--alist' in this commit.

The main advantage is that context can now be added by pushing
files and buffers (objects, not buffer names) to this alist.  This
variable can be made buffer local, etc.  The full API will be
described in the next commit, after renaming
`gptel-context--alist' to a user option.

This commit fixes the plumbing required for this change,
`gptel-context--wrap' is now a pure function -- it returns a
sanitized version of `gptel-context--alist' that can be fed to the
context wrapper (`gptel-context-string-function').  Since
`gptel-context--alist' is intended to be a user option, we modify
it less frequently than before to avoid user surprise.
Unfortunately we still need to do this a few times for gptel's
menus to function as intended.

* gptel-context.el (gptel-context-string-function):
(gptel-context-remove):
(gptel-context-remove-all):
(gptel-context--collect-media):
(gptel-context--collect):
(gptel-context--insert-buffer-string):
(gptel-context--string):
(gptel-context--buffer-setup):
(gptel-context-confirm):

* gptel-transient.el (gptel--describe-infix-context): Don't
run `gptel-context--wrap' when showing `gptel-menu'.  Use
the (possibly out of date) gptel-context--alist here.

* gptel.el (gptel-mode): Adjust header-line display.

The effect of this staleness is yet to be gauged.
@karthink
gptel: Add gptel-context user option to specify context
445d655 
Rename `gptel-context--alist' to `gptel-context', and make it a
user option.  This goes some way towards addressing the requests
for buffer-local contexts, programmatic context specification and
context via gptel presets.  (See karthink#459, karthink#475, karthink#481, karthink#548, karthink#984 and
several more for details.)

`gptel-context' can contain file paths or buffers, which covers
the most common cases. More involved context
specification (overlays, files with mime types etc) is also
possible, which see.

* NEWS (New features and UI changes): Mention change
* gptel-anthropic.el (gptel--wrap-user-prompt):
* gptel-bedrock.el (gptel--wrap-user-prompt):
* gptel-context.el (gptel-context--add-text-file):
(gptel-context--add-binary-file):
(gptel-context--add-directory):
(gptel-context-remove):
(gptel-context-remove-all):
(gptel-context--make-overlay):
(gptel-context--collect-media):
(gptel-context--collect):
(gptel-context--string):
* gptel-gemini.el (gptel--wrap-user-prompt):
* gptel-ollama.el (gptel--wrap-user-prompt):
* gptel-openai.el (gptel--wrap-user-prompt):
* gptel-request.el (gptel-context--alist):
(gptel-context):
(gptel--transform-add-context):
(gptel--realize-query):
* gptel-transient.el (gptel--describe-infix-context):
(gptel--describe-suffix-send):
(gptel--infix-context-remove-all):
(gptel--suffix-context-buffer):
* gptel.el (gptel-mode):
@karthink
gptel-context: Fix indentation and formatting
98553a8 
* gptel-context.el (gptel-context-add):
(gptel-context-add-file):
(gptel-context--wrap):
(gptel-context--add-region):
(gptel-context--insert-buffer-string):
(gptel-context--string):
@leezu @karthink
gptel-bedrock: aws profile keyword argument
8a8dd78 
This extends the previous AWS_PROFILE based get-credentials
implementation to allow explicit setting of the profile

gptel-bedrock: swap argument / env var precedence
@akssri @karthink
gptel-bedrock: incorporate static-IAM cred
afe701c 
- give higher precedence to profile arg over other env. variables
@leezu @karthink
gptel-bedrock: AWS_BEARER_TOKEN_BEDROCK support
61d09c8 
AWS has introduced Bearer token based authentication for Bedrock
https://docs.aws.amazon.com/bedrock/latest/userguide/api-keys.html
@akssri @karthink
gptel-bedrock: add bearer-token argument to backend
0737508
@karthink karthink force-pushed the akssri/bedrock-update branch from eb2971f to 0737508 Compare September 27, 2025 07:49
@karthink
gptel-bedrock: Linting
b31731a 
* gptel-bedrock.el (gptel-bedrock--fetch-aws-profile-credentials):
(gptel-bedrock--curl-args): Fix linter warnings.
@karthink karthink merged commit b31731a into karthink:master Sep 27, 2025
@karthink
Copy link
Owner

Merged, thanks everyone.

I only took a cursory look at the changes -- AWS auth looks quite complicated to me from this distance! Not sure if there's a way to simplify the auth method in gptel-make-bedrock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@lgfang lgfang lgfang left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@akssri @karthink @andykuszyk @kotatsuyaki @leezu @CsBigDataHub @lgfang