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 async dependencies optional. #2858

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tomchristie
Copy link
Member

See the "Version 1.0, Minimising project dependancies" discussion for background, as well as the proposed PR for httpcore version 1.0.

This pull request would changes our default install footprint from...

  • httpcore
    • certifi
    • h11
    • sniffio
    • anyio
  • idna

To this...

  • httpcore
    • certifi
    • h11
  • idna

We do also have scope to make idna support optional, but this pull request doesn't address that.

@tomchristie
Copy link
Member Author

Tests currently blocked on httpcore 1.0.

@tomchristie tomchristie mentioned this pull request Sep 20, 2023
@tomchristie
Copy link
Member Author

Tests currently blocked on httpcore 1.0.

Resolved

@tomchristie tomchristie marked this pull request as ready for review October 10, 2023 12:47
@tomchristie tomchristie requested a review from a team October 10, 2023 12:57
Comment on lines 47 to 58
http2 = [
"h2>=3,<5",
"httpcore[http2]",
]
socks = [
"socksio==1.*",
"httpcore[socks]",
]
asyncio = [
"httpcore[asyncio]"
]
trio = [
"httpcore[trio]"
]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is interesting... currently we're including the httpcore dependencies explicitly.

Eg. pip install httpx[socks] uses the dependency socksio==1.* instead of the dependency httpcore[socks].

The second style makes more sense, because that's what we're actually trying to effect... but I'm a bit hesitant that some Python install tooling might incorrectly fail to install httpcore[socks] as an optional dependency when the httpcore dependancy is already included. Is my hesitancy well founded here, or are we okay to use this new style throughout?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should always work but you're right that it's tool dependent and the Python specification around this does not provide guarantees.

README.md Outdated Show resolved Hide resolved
tomchristie and others added 2 commits October 12, 2023 10:42
Co-authored-by: T-256 <132141463+T-256@users.noreply.github.com>
@uuip
Copy link

uuip commented Nov 15, 2023

remove anyio.
Shall we must use anyio to support asyncio? redundant anyio

#2726

@abersheeran
Copy link
Member

If async is optional, should we improve the unit test so that it can skip some tests when certain optional dependencies are not installed? And can we consider conducting separate tests in CI for both scenarios: when optional dependencies are not installed and when they are already installed?

@tomchristie
Copy link
Member Author

@abersheeran I don't know. We don't have any need for that in CI, but perhaps some folks would appreciate fast runs in local dev. Perhaps spin it out into a new discussion?

@zanieb
Copy link
Contributor

zanieb commented Dec 21, 2023

We don't have any need for that in CI

It's probably worthwhile to make sure the project works as intended in some basic form during CI without the optional dependencies.

@tomchristie
Copy link
Member Author

😀 💬

@zanieb
Copy link
Contributor

zanieb commented Dec 23, 2023

I'm not sure it's fair to say we should have a separate discussion when we need something like it for reasonable coverage of the changes introduced in this pull request.

I think "skip tests when the dependencies are not available for better developer experience" makes sense as a new discussion, but "ensure the library works without the async dependencies installed" is critical here. Otherwise, we're just relying on manual testing to ensure the default installation of the library isn't broken. If you have a strong preference for a follow-up pull request, I can get behind that, but I think we need automated testing before release.

@tomchristie
Copy link
Member Author

@zanieb Ah interesting. I've perhaps been missing your intent here.

I had "should we ensure the tests can run without all dependencies installed?".
I didn't have "should we run the tests without all dependencies installed (in CI)?".

@zanieb
Copy link
Contributor

zanieb commented Dec 28, 2023

👍

I didn't have "should we run the tests without all dependencies installed (in CI)?".

Yeah I think this is the idea. We should run some subset of the tests (i.e. the synchronous API) in CI with the default install dependencies to ensure things are working as intended now (and in the future).

@tomchristie
Copy link
Member Author

tomchristie commented Jan 8, 2024

@zanieb Do we want to block this on "run the tests without all dependencies installed" or not?
(My take would probably be... we could consider it but as a follow-up... since we already have some optional dependencies)

There's a chunk of work there that I'd suggest would need to be taken on incrementally if we want to deal with it.

docs/async.md Outdated Show resolved Hide resolved
@zanieb
Copy link
Contributor

zanieb commented Jan 8, 2024

@zanieb Do we want to block this on "run the tests without all dependencies installed" or not? (My take would probably be... we could consider it but as a follow-up... since we already have some optional dependencies)

There's a chunk of work there that I'd suggest would need to be taken on incrementally if we want to deal with it.

I'm okay with merging this and addressing as a follow-up!

@tomchristie tomchristie added the 1.0 proposal Pull requests proposing 1.0 API changes label Jan 11, 2024
Copy link
Contributor

@T-256 T-256 left a comment

Choose a reason for hiding this comment

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

Let's add changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 proposal Pull requests proposing 1.0 API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants