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

Use unasync #3130

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

Use unasync #3130

wants to merge 5 commits into from

Conversation

karpetrosyan
Copy link
Member

Discussed in: #3046

In order to avoid duplicating code for both the async and sync versions, this PR includes a script that converts certain async sections of the code to sync sections. By doing this, problems like these will be avoided: #3120, #3042

In order to make the review process easier, I chose to divide the massive refactoring into smaller, more manageable pieces. We can unasync client, transport, and tests in separate pull requests.

@karpetrosyan karpetrosyan added enhancement New feature or request refactor Issues and PRs related to code refactoring labels Mar 1, 2024
@karpetrosyan karpetrosyan requested a review from a team March 1, 2024 16:20
@tomchristie
Copy link
Member

Oh interesting thanks! If we're using the unasync approach here then we probably also want to drop BaseClient completely...

We don't need to subclass the common parts of the class anymore, we can just replicate them, right?

@karpetrosyan
Copy link
Member Author

Oh interesting thanks! If we're using the unasync approach here then we probably also want to drop BaseClient completely...

We don't need to subclass the common parts of the class anymore, we can just replicate them, right?

Actually yes, but we can probably keep once it's already isolated.

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 2, 2024

This will break Starlette.

@karpetrosyan
Copy link
Member Author

This will break Starlette.

I guess it will be easy to resolve

@kristjanvalur
Copy link

kristjanvalur commented Mar 4, 2024

Just pimping my own asynkit here, specifically await_sync()

@Kludex Kludex added the do not merge PRs that should not be merged label Mar 16, 2024
@Kludex
Copy link
Sponsor Member

Kludex commented Mar 16, 2024

Please do not merge without a plan or resolution on Starlette's side.

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.

Ok, overall, it seems great to me.

It seems we have internal-breaking-change by renaming _client.py file to _clients folder.

Also, I think using USE_CLIENT_DEFAULT instead of None can be counted as breaking change(?) So I think we need minor release for this reason.

And for last thing: Do you have any reason to not use same sync/async files structure as httpcore?

Comment on lines +1 to +2
from ._async_client import *
from ._sync_client import *
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these really need to be imported all members? I think each of them only needs to expose one member.

Suggested change
from ._async_client import *
from ._sync_client import *
from ._async_client import AsyncClient
from ._sync_client import Client

@@ -43,6 +43,28 @@ class UnsetType:
pass # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please also add docstring here instead of pass like other classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's focus on one thing. Little docstring changes could be done in a separate PR ( I guess it's not the only place we could add a docstring to make things more simple ).

Copy link
Member

Choose a reason for hiding this comment

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

Let's focus on one thing.

+1 to that, yep.

@@ -14,7 +14,7 @@
from ._urls import URL
from ._utils import get_ca_bundle_from_env

__all__ = ["Limits", "Proxy", "Timeout", "create_ssl_context"]
__all__ = ["Limits", "Proxy", "Timeout", "create_ssl_context", "USE_CLIENT_DEFAULT"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not exposing already implemented UNSET instead of creating new constant?

@@ -134,3 +134,6 @@ async def __aiter__(self) -> AsyncIterator[bytes]:

async def aclose(self) -> None:
pass


AnyByteStream = Union[SyncByteStream, AsyncByteStream]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's usecase of this?

@@ -100,6 +100,8 @@ replacement = 'src="https://raw.githubusercontent.com/encode/httpx/master/\1"'
select = ["E", "F", "I", "B", "PIE"]
ignore = ["B904", "B028"]

exclude = ["httpx/_clients/_sync_client.py"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Revealment PR to avoid exvluding on httpcore: encode/httpcore#887
(I'm fine to use it at first step. we can pop it separately)

@@ -12,3 +12,4 @@ set -x
${PREFIX}ruff format $SOURCE_FILES --diff
${PREFIX}mypy $SOURCE_FILES
${PREFIX}ruff check $SOURCE_FILES
${PREFIX}python scripts/unasync.py --check
Copy link
Contributor

Choose a reason for hiding this comment

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

polish: final newline

@@ -10,3 +10,4 @@ set -x

${PREFIX}ruff --fix $SOURCE_FILES
${PREFIX}ruff format $SOURCE_FILES
${PREFIX}python scripts/unasync.py
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

@T-256 T-256 Apr 13, 2024

Choose a reason for hiding this comment

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

Is this file formatted?
Perhaps we could also include scripts to $SOURCE_FILES in lint and check.

@tomchristie
Copy link
Member

tomchristie commented Apr 15, 2024

Hi @karpetrosyan - Thanks, and sorry I've been paused and slow to review on this. Have been a bit stuck on it because... I'm not sure it's a great trade-off?

Resolves a point of inconsistency that we've bumped into in the past, but it's also a relatively large amount of churn and in this context I think we're maybe making things more complicated, when we could instead aim at getting-reviews-correct on PRs that touch _client.py.

@karpetrosyan
Copy link
Member Author

Hi @karpetrosyan - Thanks, and sorry I've been paused and slow to review on this. Have been a bit stuck on it because... I'm not sure it's a great trade-off?

Resolves a point of inconsistency that we've bumped into in the past, but it's also a relatively large amount of churn and in this context I think we're maybe making things more complicated, when we could instead aim at getting-reviews-correct on PRs that touch _client.py.

Hey! I disagree with you on this point:(

I don't think getting-reviews-correct on PRs is any more dependable than this solution; even though we have changed a lot in this PR, I believe it was a necessary refactoring.

@tomchristie
Copy link
Member

I disagree with you on this point

No problem, let's go with it then.

@T-256 suggested "Do you have any reason to not use same sync/async files structure as httpcore?", which I guess is a valid alternative here, eg...

  • _async/_client.py
  • _sync/_client.py

I've probably got a marginal preference for that, but I don't think it's a blocker.

Thoughts?

@karpetrosyan
Copy link
Member Author

Yes, that is more in line with our style, but I have run into some problems with the file structure.
This PR was opened about two months ago, so I had forgotten what type of problem there was, so I tried again, and there are coverage issues with the clients base functionality.

Some of the tests were developed specifically for Async or Sync clients, therefore we need to clean up testing before using the file structures _async and _sync.

@tomchristie
Copy link
Member

coverage issues

Ah yeah that figures. Maybe we should start with some test refactoring so that we've got properly mirrored sync/async tests that do have 100% coverage onto the client module.

@karpetrosyan karpetrosyan mentioned this pull request Apr 21, 2024
13 tasks
@karpetrosyan
Copy link
Member Author

Maybe we should start with some test refactoring so that we've got properly mirrored sync/async tests that do have 100% coverage onto the client module.

Actually, yes. I think we should have started with refactoring the tests.
But if we decide to refactor tests first, I think this PR will become stale, and it will be very difficult to keep it in sync. (This PR will silently overwrite changes that were made in the client code since it was opened.)

So I think holding this MR for a long time has(?) certain risks.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs that should not be merged enhancement New feature or request refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants