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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async and httpx support #131

Closed
HarrySky opened this issue Jan 14, 2020 · 13 comments 路 Fixed by #134
Closed

Async and httpx support #131

HarrySky opened this issue Jan 14, 2020 · 13 comments 路 Fixed by #134
Labels
enhancement New feature or improvement

Comments

@HarrySky
Copy link
Contributor

Hey, just found this library - great work!

100% tests coverage is great and 100% type-hints (but not just dicts and lists) and async support would be even better 馃槃

Did not found a project that has all three, so I written own wrapper for my pet-project with those requirements.

But it has only 3 endpoints (user/me, player/currently-playing and OAuth2.0 methods) and I don't want to invent my own wheel (API wrapper), can I help with annotations and async support?

@felix-hilden felix-hilden added the enhancement New feature or improvement label Jan 14, 2020
@felix-hilden
Copy link
Owner

Thank you and welcome! Async support has been in the back of my mind since discovering PyFy. It would certainly be a good feature to add. Initially I thought that it wouldn't have that much use, but would you like to tell me more about your application? I'd love to hear what sorts of things its good for, especially since I'm not that experienced with async itself.

What do you have in mind in terms of annotations? They are almost everywhere in our code already. The most significant problem is with documentation (#109).

@HarrySky
Copy link
Contributor Author

HarrySky commented Jan 14, 2020

About annotations - it was first assumption, I glanced at model/artist.py and saw external_urls: dict and thought there is a lot of that kind of annotations, actually everything is cool :)
Though class ExternalURL would be cool to have.

class ExternalURL:
    spotify: str

My pet-project is not working for about a year, but what it was doing is this:

  1. Let me login via OAuth2.0
  2. Get information about me (username and avatar)
  3. Get currently playing song every 5 seconds
  4. Let other users "Listen Along" (backend will set the same track with the same progress on their devices)

Year ago I started experiencing problems with backend workers timeouts, since I was using async web framework (starlette) with sync requests (when Internet was down - requests for current song took longer than 5 seconds - it was not responding). So I decided to switch to async httpx package that has almost 100% compatibility with requests.

I see you are using requests, so it should not be a challenge to add httpx support. What do you think?

@HarrySky
Copy link
Contributor Author

Frontend is still up and running - https://music.neigor.me/

@felix-hilden
Copy link
Owner

If I recall, that sort of class existed at some point. But I took it out, maybe because there were other external URLs, but I don't quite remember. I'll have to look at it once more.

That's a cool application! I'll investigate httpx support too. Let me get back to you.

@felix-hilden felix-hilden changed the title Async support and type hints Async and httpx support Jan 14, 2020
@felix-hilden
Copy link
Owner

At first glance Httpx seems like a great library! It even has a client object for automatically appending base URLs. Cool. And they are planning to release 1.0.0 in this April.

From what I've understood, the real power of Async programming is the ability to make concurrent requests. Now that's rather obvious, but correct me if I'm wrong: with sequential applications using Async it is essential to gather many requests for execution at once and await them, rather than making one request, then some code, then another request. Then servers with Async are another thing.

So if we were to implement an Async client, given my inexperience, I'd like to clarify some things. If you can help, great, if not then I'll dive in deeper myself.

  • Async client with Requests: could it (at least initially) just wrap the methods of the sync client with async keywords and additionally provide some methods for gathering the results? And is it beneficial right away?
  • What would it mean to the above implementation of an Async client to use Httpx instead of Requests? Does Requests constrain the Async nature in any way or does Httpx simply provide better support for it out of the box?
  • Would the sync client benefit from Httpx enough to warrant discarding Requests altogether?
  • Specifically, how do sessions and keepalive fit in Httpx and Async in general?

@HarrySky
Copy link
Contributor Author

Concurrent requests - yes, async gives ability to make them in parallel, because async should not block execution as it blocks it with threads. When you are writing async code - you should go full async, otherwise libraries like requests will block execution while making request, because requests is not built with async support (you need to use async defs and await keywords so that methods can run on asyncio event loop and also use it's non-blocking I/O API).

So we can't use requests for async client. Wrapping existing methods will not work since we need to use async/await keywords.

httpx supports both sync and async, but it is not as mature as requests, so it is up to you to decide whether it is okay to discard requests for sync :)

Last question I did not get, what do you mean? Sessions and keepalive should work the same as for requests.

If there is anything else you need to know - let me know

@felix-hilden
Copy link
Owner

Yes, these were exactly what I was looking for, thanks a bunch! With regards to the last question, I'm simply not aware of how the keepalive is implemented and how requests are delivered in HTTP. The fact that it can be used in async as well is enough for me.

So here's what I'm thinking. We could start developing the idea of an async client using Httpx and implementing some simple version. Then we should think about what it means for Senders that wrap around requests.Session, could they provide both async and sync "sessions" or what would be the best way of providing them for both types.

I'll take the ball first, implement something and make a PR so we can discuss the practicalities more. It may take a while though. Let's see what we can make out of this!

@HarrySky
Copy link
Contributor Author

I could try to make some simple proof of concept, but I will have time only on Friday and weekend this week

@felix-hilden felix-hilden mentioned this issue Jan 19, 2020
8 tasks
@felix-hilden
Copy link
Owner

The mentioned PR will provide initial support for Async and Httpx, but not break compatibility. Some additional concerns will remain, and they are very much worth considering when we think about releasing 2.0.

  • RetryingSender tries to instantiate a default type, but accepts only synchronous senders for obvious reasons, should we remove the defaulting behavior?
  • Should the sender module be divided into two submodules for sync and async, and if so, how should defaults be handled?
  • Should we use Httpx exclusively? I think at least not before it's stable.

@marioortizmanero
Copy link
Contributor

marioortizmanero commented Jan 19, 2020

Ah, I just saw your comment. I don't think I'm experienced enough with async to give you advice, but using only httpx would be quite nice, in my opinion. It doesn't seem as mature as requests, though, so I'm not sure if you'll have problems with it. But it seems simpler and more straightforward this way.

@HarrySky
Copy link
Contributor Author

@felix-hilden my thoughts:

  • RetryingSender can accept base class Sender and then check if AsyncSender is provided and act accordingly
  • I think all senders belong in one module, so that user can choose and switch easily
  • Let's wait for stable 1.0 release and then we can discuss dropping requests in favor of httpx

@felix-hilden
Copy link
Owner

Thanks for your input!

I meant dividing them up into submodules for just for the sake of refactoring and cleaning things up. The senders would then be imported back to the top level of sender.

@felix-hilden
Copy link
Owner

felix-hilden commented Jan 25, 2020

We can't refactor the sender module yet. But I have an idea for an improvement. It would break setting defaults though. This is beyond this issue, but I'll open up another one based on this for Tekore 2.0 when this is closed. And one for Httpx exclusivity too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants