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

replace Requests http client by Niquests #664

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Ousret
Copy link

@Ousret Ousret commented Oct 15, 2024

This PR effectively replace the http client Requests for Niquests.
Niquests is a drop-in replacement for Requests that is no longer under feature freeze.

This new client support HTTP/2, and HTTP/3 by default and offers both sync and async interfaces. It supports all the latest shiny features you would expect from an http client.

If you were interested on merging, I will be interested to propose a followup PR that will propose a script that generate
the async part of ytmusicapi automatically. If you want to.

disclaimer: I maintain Niquests.

Niquests is a simple, yet elegant, HTTP library. It is a drop-in replacement for Requests, which is under feature freeze.
HTTP/1.1, HTTP/2, and HTTP/3 supported.
Both sync and async interfaces are available.
@sigma67
Copy link
Owner

sigma67 commented Oct 15, 2024

Sorry, but from my perspective there are no benefits.There are no features missing in requests for the purposes of this library.

In addition a lot of risk would be introduced by switching to something less widely used than requests which has been stable for years and is PSF supported.

@sigma67 sigma67 closed this Oct 15, 2024
@sigma67
Copy link
Owner

sigma67 commented Oct 15, 2024

Also note that this broke half of all tests, seems it's not just a drop-in

https://github.com/sigma67/ytmusicapi/pull/664/checks

@Ousret
Copy link
Author

Ousret commented Oct 16, 2024

You came back to take a deeper lock.
This is somewhat interesting.

There are no features missing in requests for the purposes of this library.

That I may understand. But it is not entirely true. I would not propose this if your community wouldn't benefit from it.
Browsing your issue tracker shows horrible things like self.code = await self.hass.async_add_executor_job(self.oauth.get_code)
Native async would be a nice addition. According to me.

Then, you should know that YT admins may, without notice, kill HTTP/1 requests to make this lib useless. So, HTTP/2, and HTTP/3 would be nice to gain more confidence on that topic.

In addition a lot of risk would be introduced by switching to something less widely used than requests which has been stable for years

That I've heard many times. This thinking is tautological. Let's summarize what you said:

Old + Big usage = Better ?

So, if given a choice (for free) between a "Toyota Corolla from 2003" and a "Tesla Model X from 2023" you would pick the first by that logic. You would also choose Google Chrome over others without question. (and so on..)

Ok, so there I am not saying that Niquests is bug-free, but it's in a pretty good place right now.
We should be concerned more by the ability of someone to respond to bug than on his ability to have none from the start. And today, if you peek at Requests, it is bad.
Feel free to inspect what we have done and how we treat issues (both Requests and us)

Progress can't happen otherwise.

Also note that this broke half of all tests, seems it's not just a drop-in

You closed the PR in minutes, so I didn't get the chance of looking further.
The errors aren't bad, and they are now fixed the branch.

If you're curious, re-open and re-rerun them. hint: CaseInsensibleDict isn't made for setting storage, but only http headers. It was a mistake from Requests era to allow garbage as values.

regards,

@sigma67
Copy link
Owner

sigma67 commented Oct 16, 2024

Thank you for the eloquent and detailed response, appreciate it. To address your points:

  • I will admit that async has been a somewhat frequently requested feature for this library. But I don't yet see how niquests would solve that issue, and autogenerating code to solve it seems like a strange approach to me (albeit I have not looked into this further)
  • How much merit does the " YT admins may, without notice, kill HTTP/1 requests to make this lib useless" argument really have? Is there any indication that this may actually happen?
  • "Old + Big usage = Better ?" No, this is not my argument. Better is hard to define anyway. What matters for me only, and I will repeat that, is stability. Requests has done the job since this library was created almost 4 years ago with 0 breaking changes. In contrast, the first version of niquests was released a year ago (and you admit it is not bug free - fair - this might be one of those).
  • If requests ceases to do the job,I will swap it for a decent replacement. Keep in mind that niquests is not the only alternative and the choice must be based on what is the best fit for this library (and ideally least maintenance effort - as a fork of requests you have one up here)

I will keep your project in mind, but at this point it is too early for me to make a decision that is not prompted by a need

Edit: I will reopen the PR for now but that does not mean this will be merged anytime soon

@sigma67 sigma67 reopened this Oct 16, 2024
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.96%. Comparing base (2b5d19a) to head (22d3c41).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ytmusicapi/mixins/browsing.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #664      +/-   ##
==========================================
- Coverage   94.99%   94.96%   -0.04%     
==========================================
  Files          38       38              
  Lines        2278     2283       +5     
==========================================
+ Hits         2164     2168       +4     
- Misses        114      115       +1     
Flag Coverage Δ
unittests 94.96% <91.30%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ousret
Copy link
Author

Ousret commented Oct 17, 2024

autogenerating code to solve it seems like a strange approach to me

You are not alone on this take. But unfortunately, no other way emerge to solve this.
There is no reliable way to have a single code base and do both sync and async.

Basically here, you'll need to duplicate code and all the way down from the http calls up to the user front call, you'll need to convert async/await(able).

I've already contributed to that goal at https://github.com/grafana-toolbox/grafana-client/blob/main/script/generate_async.py
The code between async and sync doesn't change, so to relieve you of the burden of updating the async part frequently or ask contributor to do so every time, you may prefer this option.

Is there any indication that this may actually happen?

There is no official statement (figure...) but all over internet, the number of reports about bans start to grow. You have yourself some complaint about it.

Not only Google, but major provider like Cloudflare use the way you connect to them (TLS layer, http headers, protocol used, cipher, etc...) to determine how likely you are NOT a traditional browser. (see https://niquests.readthedocs.io/en/latest/user/advanced.html#tls-fingerprint-like-ja3 for more info)

This is how they target and act upon library/client like yours.

I am extremely confident that they will kill HTTP/1 soon. Some services (major) in my home country started to progressively kill HTTP/1, this was the starting point of the development for Niquests.

Requests has done the job since this library was created almost 4 years ago with 0 breaking changes.

To you, yes, no breaking. But for other major libraries out there, there are major breakage happening while they are in maintenance mode only. And there's still major fixes awaited by the community, sometimes for months or years.

this might be one of those).

You are right, this happen to be a minor issue. The Google QUIC implementation seems to be aggressive toward keepalive/ACKs.
We've applied a fix for that.

If requests ceases to do the job

Don't wait for complete "out-of-order" situation. Maybe try to be proactive about this one. Keep in mind that Requests stated that the project is already half-dead.

but at this point it is too early for me to make a decision that is not prompted by a need

I understand completely. Take your time.
If you need further collaboration, I'll be there.

Until then,

Regards,

@sigma67
Copy link
Owner

sigma67 commented Oct 17, 2024

You are right, this happen to be a minor issue. The Google QUIC implementation seems to be aggressive toward keepalive/ACKs.
We've applied a fix for that.

Could you point out the mentioned fix?

Keep in mind that Requests stated that the project is already half-dead.

Could you link the source for this please?

Additionally, since your project claims performance benefits (up to 3x faster than requests), I had a look at the test runtime. It seems worse than with requests:

Granted that most of the runtime is to blame on server-side processing. Nevertheless it still seems like a significant performance regression at first glance. Any clue?

@Ousret
Copy link
Author

Ousret commented Oct 21, 2024

Could you point out the mentioned fix?

Of course. See jawah/urllib3.future#161 and jawah/urllib3.future#162

Could you link the source for this please?

There is more, but, you will excuse me to not collect the hundreds of demonstration for it.

It seems worse than with requests:

Short story: no multiplexing used, only pure synchronous. re-do QUIC handshake hundred of times using Post-Quantum Security, don't reuse session, etc...

See https://niquests.readthedocs.io/en/latest/community/faq.html#why-http-2-and-http-3-seems-slower-than-http-1-1

To witness the speed up in CI, that's tricky, because you'll need to instantiate once the niquests.Session() and leverage pytest concurrency, then you'll see improvement for sure, up to 2 times faster. (x3 times is reserved to async or manual multiplexing aka. lazy responses)

To be clear, our claim about 3x faster is accompanied with a reproducer / specific scenario. If you use HTTP/2+ as you use HTTP/1, it will be slower because you don't leverage his potential.

On the latest run, we get:

  • ======= 1 failed, 104 passed, 1 skipped, 1 retried in 218.81s (0:03:38) ========

so, not so significant after all.

The failure here seems unrelated to us:

regards,

@Ousret
Copy link
Author

Ousret commented Nov 6, 2024

It seems that we are facing more ban from YT. The phenomena is getting more and more important.
(Looking at google recent results + GH issues mentioning YT+ban)

maybe it worth to pursue that PR and getting HTTP/3 mainstream.

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.

2 participants