Skip to content
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

Make ListObject.auto_paging_iter() implement AsyncIterator #1247

Merged
merged 8 commits into from
Feb 23, 2024

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Feb 22, 2024

Summary

This PR gets rid of .auto_paging_iter_async() and makes .auto_paging_iter() capable of asynchronous iteration.

Not included: the analog for SearchResult. Want feedback on this approach before I apply it everywhere.

Motivation

We got some feedback from internal users that it error-prone figuring out how to migrate sync code that used .auto_paging_iter, e.g.

- print(", ".join([cus.id for cus in stripe.Customer.list().auto_paging_iter()]))
+ print(", ".join([cus.id async for cus in stripe.Customer.list_async().auto_paging_iter_async()]))

There are 3 things you have to change, and so 2^3 - 2 = 6 ways to incorrectly/partially migrate this (minus one for the correct migration, minus one for making no change at all). Here are 3:

# This throws
print(", ".join([cus.id async for cus in stripe.Customer.list_async().auto_paging_iter()]))
# This succeeds but will make synchronous calls
print(", ".join([cus.id for cus in stripe.Customer.list_async().auto_paging_iter()]))
# This succeeds but makes a synchronous call.
print(", ".join([cus.id async for cus in stripe.Customer.list().auto_paging_iter_async()]))

I experimented a little bit with introducing ListObjectAsync and SearchResultAsync, this added a lot of complexity and would have resulted in

# This throws and gives a type error
print(", ".join([cus.id async for cus in stripe.Customer.list_async().auto_paging_iter()]))
# This throws and gives a type error
print(", ".join([cus.id for cus in stripe.Customer.list_async().auto_paging_iter()]))
# This succeeds but makes a synchronous call.
print(", ".join([cus.id async for cus in stripe.Customer.list().auto_paging_iter_async()]))

This PR, on the other hand, makes it so that it is not a mistake to use async for along with .auto_paging_iter(). So it reduces the possible ways to migrate incorrectly to 2^2 - 2 = 2.

# This succeeds but makes synchronous calls
print(", ".join([cus.id for cus in stripe.Customer.list_async().auto_paging_iter()]))
# This succeeds but makes a synchronous call.
print(", ".join([cus.id async for cus in stripe.Customer.list().auto_paging_iter()]))

As a follow-up, we could consider adding (and recommending) an option like stripe.default_http_client = stripe.HTTPXClient(disable_sync=True) that would cause these to begin throwing exceptions if the user wanted.

@richardm-stripe richardm-stripe marked this pull request as draft February 22, 2024 21:17
@richardm-stripe richardm-stripe requested review from a team and pakrym-stripe and removed request for a team February 22, 2024 21:28
@richardm-stripe richardm-stripe marked this pull request as ready for review February 22, 2024 21:28
@anniel-stripe
Copy link
Contributor

 # This succeeds but makes synchronous calls
 print(", ".join([cus.id for cus in stripe.Customer.list_async().auto_paging_iter()]))

Do we want this to succeed? If the user wanted to use all async methods, this seems like something they could easily miss and unintentionally introduce synchronous calls.

@richardm-stripe
Copy link
Contributor Author

@anniel-stripe

Do we want this to succeed? If the user wanted to use all async methods, this seems like something they could easily miss and unintentionally introduce synchronous calls.

I think ideally, no. But the only way to get this to fail is via an approach like https://github.com/stripe/stripe-python/compare/richardm-async-list-object?expand=1 which has a lot of downsides, basically you have to have 2 separate versions of ListObject (which is kind of at odds with the design decision we've made not to have mirror versions of resources), and you also have to flow a prefer_async_versions property through the entire request-making infrastructure in order to persist the knowledge of whether a particular list_object was obtained through the result of an asynchronous request or not.

So instead of that, for users who want to make sure that they've eliminated all synchronous requests, I propose a bigger hammer

stripe.default_http_client = stripe.HTTPXClient(disable_sync=True)

this unfortunately won't help the typechecker find synchronous method calls but it could cause runtime errors when they run tests.

@anniel-stripe
Copy link
Contributor

anniel-stripe commented Feb 22, 2024

Ahh, I didn't parse the follow-up about stripe.default_http_client = stripe.HTTPXClient(disable_sync=True).

So if I'm understanding correctly:

# No migration, all synchronous calls
print(", ".join([cus.id for cus in stripe.Customer.list().auto_paging_iter()]))
# 1 asynchronous call (through `list_async` method) + synchronous pagination calls
print(", ".join([cus.id for cus in (await stripe.Customer.list_async()).auto_paging_iter()]))
# 1 synchronous call (through the `list` method) + asynchronous pagination calls
print(", ".join([cus.id async for cus in stripe.Customer.list().auto_paging_iter()]))
# Correct migration, all asynchronous calls
print(", ".join([cus.id async for cus in (await stripe.Customer.list_async()).auto_paging_iter()]))

Overall, I do think this change would give a better experience!

@richardm-stripe richardm-stripe force-pushed the richardm-unify-async-sync-auto-paging-iter branch from 61844dd to 163adc4 Compare February 23, 2024 22:22
@richardm-stripe
Copy link
Contributor Author

Ok @anniel-stripe, since the last time you reviewed:

  • I added support to search_result too
  • I renamed the SyncAsyncIterator to AnyIterator and moved to a central location. I also added an exception if you try and do both sync and async iteration.
  • I had to make some codegen changes to fix the search. Apparently every searchable API resource (unlike listable resources) defines a .auto_paging_iter and .auto_paging_iter_async directly on the top level.

@richardm-stripe richardm-stripe enabled auto-merge (squash) February 23, 2024 22:58
@richardm-stripe richardm-stripe merged commit b077eea into beta Feb 23, 2024
14 checks passed
@xavdid-stripe xavdid-stripe deleted the richardm-unify-async-sync-auto-paging-iter branch May 10, 2024 03:36
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.

None yet

2 participants