Skip to content

Conversation

karpetrosyan
Copy link
Contributor

closes #892 and #537

@karpetrosyan karpetrosyan requested a review from a team as a code owner June 13, 2025 09:10
Copy link
Collaborator

@dtmeadows dtmeadows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall LGTM @karpetrosyan! Could we just remove the workflow change before merging?

@@ -7,6 +7,8 @@ on:
- 'integrated/**'
- 'stl-preview-head/**'
- 'stl-preview-base/**'
pull_request:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind removing this before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@karpetrosyan
Copy link
Contributor Author

@dtmeadows fyi, seems like I can't merge this

@dtmeadows
Copy link
Collaborator

@michael-cohen-io mind reviewing and merging this when you get a sec?

…o karpetrosyanpy/sdk-2760-anthropic-python-correctly-infer-bedrock-region-from-aws
try:
import boto3

session = boto3.Session()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually do you know if this can make fs calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand what 'making fs calls' means. It has access to the standard library, which I assume it uses to read the AWS config file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, yeah I meant "does it call blocking IO functions" which if it does we should try and call it asynchronously...

but that might be difficult?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely does, but I think it's okay. It won't block the event loop repeatedly—just once during initialization

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users may instantiate the client in their business logic inside async functions so I think it is somewhat important unfortunately.

if wrapping this in async is too annoying, I think for now we could cache the boto3.Session object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if wrapping this in async is too annoying, I think for now we could cache the boto3.Session object?

In that case, users with an async codebase will still be blocked the first time they call it — am I getting that right? Honestly, I don't think it's worth doing anything about. Many clients block for a few milliseconds during initialization. For example, httpx.AsyncClient takes around 50-150ms to create (which means the event loop is blocked for that time), while boto3.Session() blocks for just 5ms. Programmers should avoid creating such objects inside a (for/while) loop — that would kill performance anyway (they're supposed to be created once). So if it's done only once, is a 5ms delay really that critical?

you can check setup time with this script:

# /// script
# requires-python = ">=3.9"
# dependencies = [
#     "boto3",
#     "httpx",
# ]
# ///

import boto3
import time
import httpx

t1 = time.monotonic_ns()
boto3.Session()
t2 = time.monotonic_ns()

print(f"boto3 Time taken: {(t2 - t1) / 1_000_000} ms")

t1 = time.monotonic_ns()
httpx.Client()
t2 = time.monotonic_ns()

print(f"httpx Time taken: {(t2 - t1) / 1_000_000} ms")

@RobertCraigie RobertCraigie merged commit f648e09 into anthropics:next Jul 1, 2025
4 checks passed
@stainless-app stainless-app bot mentioned this pull request Jul 1, 2025
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.

4 participants