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

Implements Request class in Python SDK. #3689

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dom96
Copy link
Contributor

@dom96 dom96 commented Mar 10, 2025

Implements a Python native Request class based on https://developer.mozilla.org/en-US/docs/Web/API/Request/Request.

The API tries to match the Response API as closely as possible.

Test Plan

$ bazel run @workerd//src/workerd/server/tests/python:sdk_development@

@dom96 dom96 requested a review from hoodmane March 10, 2025 19:04
@dom96 dom96 requested review from a team as code owners March 10, 2025 19:04
@dom96 dom96 requested a review from harrishancock March 10, 2025 19:04
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Without running web-platform tests (where @npaun can help), I don't think we should land this, as it is. It would be extremely hard to review how compliant we are to the spec.

Tests can be found at: https://github.com/web-platform-tests/wpt/tree/983adc74e3c0a2faa1f3e3275f45c05aa5419c53/fetch/api/request

@jasnell
Copy link
Member

jasnell commented Mar 10, 2025

@anonrig ... given that this is a Python implementation of the class, conformance with the wpt is a bit secondary.

@anonrig
Copy link
Member

anonrig commented Mar 10, 2025

@anonrig ... given that this is a Python implementation of the class, conformance with the wpt is a bit secondary.

Even if it's a Python implementation we might introduce a major behavior difference between the spec and our implementation that I'm afraid we might have to live with for a really long time.

@dom96
Copy link
Contributor Author

dom96 commented Mar 11, 2025

Admittedly I don't have a lot of context on WPT, but my understanding is that they are all JS tests for the browser (or other web platform), so I don't think they can be run on a Python worker easily. But the Python code is mostly wrapping the JS anyway, so behaviour should be spec compliant in those cases, where we don't simply wrap we try to make the API more idiomatic to Python which WPT wouldn't have test coverage for.

In the long-term we'll probably want to create standards for Python on the web platform and create a test suite specifically for it, maybe that can even become part of WPT, but for now I don't think we should worry about it.

Copy link
Contributor

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

I made some comments but it generally looks good to me.

@dom96 dom96 force-pushed the dominik/python-sdk-request branch 2 times, most recently from 4ee8945 to be5f56f Compare March 11, 2025 15:41
@anonrig
Copy link
Member

anonrig commented Mar 11, 2025

Admittedly I don't have a lot of context on WPT, but my understanding is that they are all JS tests for the browser (or other web platform), so I don't think they can be run on a Python worker easily. But the Python code is mostly wrapping the JS anyway, so behaviour should be spec compliant in those cases, where we don't simply wrap we try to make the API more idiomatic to Python which WPT wouldn't have test coverage for.

In the long-term we'll probably want to create standards for Python on the web platform and create a test suite specifically for it, maybe that can even become part of WPT, but for now I don't think we should worry about it.

We can at least port the tests from WPT to Python to ensure we are compliant.

@dom96
Copy link
Contributor Author

dom96 commented Mar 11, 2025

We can do so, but I don't think that will give us much signal. We should focus on testing where we diverge from the JS and let the existing JS tests verify that the underlying platform that we wrap is compliant.

@dom96 dom96 force-pushed the dominik/python-sdk-request branch from be5f56f to f8bd542 Compare March 11, 2025 16:55
@anonrig
Copy link
Member

anonrig commented Mar 12, 2025

We can do so, but I don't think that will give us much signal. We should focus on testing where we diverge from the JS and let the existing JS tests verify that the underlying platform that we wrap is compliant.

For correction: I'm not blocking, but I still think we should have some of test (preferably from WPT) to validate the Request class API surface is same as the web spec dictates. It is fine if we do it later, but I strongly believe we should do it.

It is also fine not to do it, and don't be spec compliant even though we implement a web spec.

I approved the PR because it is in a good state that I'm comfortable with approving.

@danlapid
Copy link
Collaborator

I think we might be better off conforming to Python standards instead, how about https://requests.readthedocs.io/en/latest/api/#requests.Request ?
@hoodmane wdyt?

@anonrig
Copy link
Member

anonrig commented Mar 12, 2025

@danlapid thats a really good solution and a path forward in my honest opinion.

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.

5 participants