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 and add built-in async support #186

Closed
wants to merge 19 commits into from

Conversation

Ousret
Copy link

@Ousret Ousret commented Oct 23, 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 prawcore automatically. If you want to.

Why make the switch?

I believe this will benefit you and your users:

  • Lower burden on maintaining library by having both sync/async in a single core package with a single code base.
  • Faster HTTP, multiplexed connection, HTTP/3 ready, etc..

disclaimer: I maintain Niquests.

@Ousret
Copy link
Author

Ousret commented Oct 23, 2024

We need to be able to pass PYTEST_DISABLE_PLUGIN_AUTOLOAD to the environment. Currently GH disallow doing this with reusable workflow.

@bboe
Copy link
Member

bboe commented Oct 23, 2024

This PR will take some thought as this is the first I've heard of Niquests. I'm generally a little weary of pulling in new packages, especially ones owned by individuals. I have a few questions: How many people have admin access to the repo, and what governs them? Who all can publish packages?

@LilSpazJoekp
Copy link
Member

Same concerns that @bboe has.

From a technical standpoint, PRAW will also need to pass all of its tests with this as well. It's unlikely we will switch http clients unless our concerns are resolved and it makes maintaining the async version significantly easier. The async changes to PRAW would also need to be a drop in replacement for Async PRAW.

@Ousret
Copy link
Author

Ousret commented Oct 23, 2024

Hello there,

Thanks for your fast response.

This PR will take some thought as this is the first I've heard of Niquests.

Naturally.

I have a few questions: How many people have admin access to the repo, and what governs them? Who all can publish packages?

  • Today, only my wife and I retain admin access. The roles are as follow, I develop, she manage the schedule, sponsors, fiscal, etc... Moreover, we have instructions left in case of "issues" left to each other so that the projects will be passed to a capable developer.
  • Only me can publish today.

We partnered up with Tidelift to ensure a source of income, also they handle the security part. They are reliable enough.
At current speed of thing, we reasonably expect mid-2025 to have enough Subscriber at Tidelift so that we can invite someone and share incomes. The plan is to reach 4 maintainers at Jawah (org) compensated with Tidelift. With this, we have in mind to offer bounties to freelancer every now and then.

We have gain enough trust (I hope) by managing project like charset-normalizer for example.

If we partner up on this, we will be able to move closer to the said goals.

It's unlikely we will switch http clients unless our concerns are resolved and it makes maintaining the async version significantly easier.

Maintaining the async part will be completely automatic after this PR. If you are willing to pursue this, I will manage a follow up PR that automatically generate this part like I have done in https://github.com/grafana-toolbox/grafana-client/blob/main/script/generate_async.py

I am sure that this will make your life easier, and additionally be of service to the broader community.

The async changes to PRAW would also need to be a drop in replacement for Async PRAW.

I will look into it, but it should be.

Regards,

@Ousret Ousret force-pushed the feat-niquests branch 2 times, most recently from c60e91e to 9d14800 Compare October 24, 2024 05:51
.coveragerc Outdated Show resolved Hide resolved
prawcore/requestor.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@LilSpazJoekp
Copy link
Member

We'll need to see if PRAW works with this (all tests pass) before proceeding as well.

@Ousret
Copy link
Author

Ousret commented Nov 6, 2024

Do you need anything else from my part?

regards,

@LilSpazJoekp
Copy link
Member

Would like to see it work with the async side of PRAW:

  • The generation of the async code
  • Works with exisiting Async PRAW code

@Ousret
Copy link
Author

Ousret commented Nov 10, 2024

Works with exisiting Async PRAW code

Done. praw-dev/asyncpraw#289

The generation of the async code

As I already stated, generating the async code AND the async tests are really challenging as per the 100% coverage constraint. I can generate the async code using a script, for sure, but I am less confident about the test suite to be generated along. Moreover, the async generation script is going to be doable in prawcore, not in praw or asyncpraw (due to the feature diff, asyncpraw isn't ISO with praw). In the long term, you could deprecate asyncpraw and generate the async part in praw instead.

I definitely want to propose another PR, dedicated to the async generation so that I can solely focus on it.

The work done in the three PRs should be a good starter point. WDYT?

Regards,

@Ousret
Copy link
Author

Ousret commented Nov 14, 2024

I updated praw to do the same as asyncpraw regarding websocket.
The projects are now closer than ever.

I will wait for further instruction & reviews from your side.

regards,

@LilSpazJoekp
Copy link
Member

This definitely looks promising! Give me time to review all the changes and discuss with @bboe about moving forward with this. I would like to see how the scripts to generate the async parts as that what will make this migration worthwhile.

@Ousret
Copy link
Author

Ousret commented Nov 19, 2024

Hello again,

I have just pushed an "experimental" async code generator in tools/

python tools/generate_async.py --help

usage: generate_async.py [-h] [-v] [-f] [-s] [-a]

Asynchronous Code Generator for PRAW

options:
  -h, --help        show this help message and exit
  -v, --verbose     Enable DEBUG logging
  -f, --fix         Applies the newer version of generated modules
  -s, --sync-diff   Render the differences between the synchronous parts and the newly generated asynchronous ones
  -a, --async-diff  Render the differences between the old async and newest async generated modules

It is not completed as we speak. But very close to be.

I am proposing this preview so that you can decide whether you (maintainers) want to pursue this integration (aka. async generation) or not.
So that I don't spend more time on it if it is not something you'd agree to.

I am waiting for your appreciations and feedback if any.

Regards,

@Ousret
Copy link
Author

Ousret commented Nov 20, 2024

The generator is complete for prawcore & integrated smoothly in pre-commit.
This project is one of the very few having this.

@LilSpazJoekp
Copy link
Member

Thanks for doing that! Give me some time to review this.

@Ousret
Copy link
Author

Ousret commented Jan 1, 2025

Hello,

I know the PRs can be difficult to review, that is why if you want to we could schedule a meet to work live together if you want to.

Regards,

@LilSpazJoekp
Copy link
Member

LilSpazJoekp commented Jan 1, 2025

Other than the possible decrease in async conversion burden, what benefits does niquests offer?

Development on PRAW has slowed due to less changes happening with the public API so the benefits offered from less async conversion burden isn't going to change much in the long run. In fact, it is going to cause more work in the short term do consolidate the sync and async packages.

At the moment, Bryce and I are pretty busy with life so it might be a while before we can decide to fully commit to niquests. If there isn't any benefits outside of less async burden, then I'm leaning towards sticking with requests and aiohttp.

Lastly, nothing against you (I have to consider this with every PR to PRAW) but in general I'm apprehensive about switching to or adding a new dependency when the author is driving hard for its adoption (hence my question about the benefits). Given the recent uptick in supply-chain attacks, it's throwing a red flag for me.

@Ousret
Copy link
Author

Ousret commented Jan 2, 2025

Hard to believe. But I am not shocked either.

Other than the possible decrease un async conversion burden, what benefits does niquests offer?

I suppose you meant "What does Niquests brings to PRAW itself other than the lower async burden?"
If the extensive work done at Niquests shows no interest here I should not say anything further. It's like the persons who ear the fellow saying: "I am buying a new electric car, so happy for my upcoming purchase!" And say: "I don't get it, your crappy car did get you from A to B before? What does this really benefit you aside from better tech?"

[...] In fact, it is going to cause more work in the short term do consolidate the sync and async packages.

It is not true. Yes there is work to do to achieve lower maintenance burden. Clearly deprecating two packages (asyncpraw, and asyncprawcore) will surely ends up in easier maintenance for sure.

I explicitly said numerous times that I am willing to contribute and not throw you the remaining tasks solely in your hands.

[...] when the author is driving hard for its adoption

There's no winning with this kind of argumentation. It is just weird in OSS to say something like this in a negative way.
So you have someone answering every single bit of your demands, and it ends up being perceived negatively.

Anything else than "is driving hard for its adoption" would be perceived negatively also. Figure.

Given the recent uptick in supply-chain attacks, it's throwing a red flag for me.

Don't take this the wrong way but you are clearly misinformed about the risks. Did you thought that Requests, or aiohttp or PRAW were safe? Or even better, GitHub itself.
You mention "recent events", but did we see the same thing where massively popular library got hijacked...? Am I dreaming? XZ c lib, pypi Ultralytics, ...
So raising this as a "red flag" when there is no possible way to mark something or someone as "riskier" is difficult for me to understand. And it is not like I am not already in your dependency chain..

Today, from praw and asyncpraw you have 23 (unique) dependencies in the tree. And using Niquests instead would bring it down to 13. (counting asyncprawcore gone) What is weird is that it goes against your perception.


I can see that you changed your mind. We passed from:

  • It is definitely looking promising

to:

  • than the possible decrease un async conversion burden
  • it is going to cause more work
  • is driving hard for its adoption
  • Given the recent uptick in supply-chain attacks

Just, for a favour, don't do that to anyone else in the future. You clearly let the door open, and with time you asked things to be done your way on three different projects knowing fully the amount of work required (which I have fulfilled completely) when it seems like your last message makes it like you've just discovered the PRs. This is discouraging plenty of good people to work in OSS.

It is me who is seeing the red flag now. I don't know what I can say more.
I am closing the PRs, it will make everything easier.

Regards.

@Ousret Ousret closed this Jan 2, 2025
@Ousret Ousret deleted the feat-niquests branch January 2, 2025 05:42
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.

3 participants