Skip to content

Conversation

@dom96
Copy link
Contributor

@dom96 dom96 commented Feb 1, 2024

What this PR solves / how to test:

This PR implements wrangler deploy index.py and wrangler deploy (with a wrangler.toml that defines main = "index.py"). This means Python Workers can be deployed using Wrangler.

Tested manually via:

$ cat $PWD/index.py
from js import Response
def fetch(request):
  return Response.new('Updated Python Worker value')
$ pnpm run --filter wrangler start dev $PWD/index.py --compatibility-flag experimental

Also using tests:

$ cd packages/wrangler
$ pnpm test # added --testNamePattern=\"can run and modify python worker during dev session (local)\" --silent=false in packages.json
$ pnpm test:e2e

I included an e2e test for dev but I couldn't run it locally. Other e2e tests fail locally for me too so I guess I just have something misconfigured. Help appreciated in getting it working.

Author has addressed the following:

  • Tests
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • Included
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2024

🦋 Changeset detected

Latest commit: f5c8c9e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dom96 dom96 force-pushed the dominik/python-wrangler-deploy-support branch 2 times, most recently from 530812b to fe727ae Compare February 1, 2024 15:18
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7745519897/npm-package-wrangler-4896

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/4896/npm-package-wrangler-4896

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7745519897/npm-package-wrangler-4896 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7745519897/npm-package-create-cloudflare-4896 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7745519897/npm-package-miniflare-4896
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7745519897/npm-package-cloudflare-pages-shared-4896

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240129.0
workerd 1.20240129.0 1.20240129.0
workerd --version 1.20240129.0 2024-01-29

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@dom96 dom96 force-pushed the dominik/python-wrangler-deploy-support branch 2 times, most recently from 1058e74 to c76123b Compare February 1, 2024 17:34
@dom96 dom96 changed the title Implements wrangler deploy support for Python. Implements wrangler deploy/dev support for Python. Feb 1, 2024
@dom96 dom96 force-pushed the dominik/python-wrangler-deploy-support branch 2 times, most recently from c3eb465 to de2da45 Compare February 1, 2024 17:52
@dom96 dom96 force-pushed the dominik/python-wrangler-deploy-support branch from de2da45 to f5c8c9e Compare February 1, 2024 17:53
@dom96 dom96 marked this pull request as ready for review February 1, 2024 17:53
@dom96 dom96 requested a review from a team as a code owner February 1, 2024 17:53
}));
});

it.only("can run and modify python worker during dev session (local)", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it.only("can run and modify python worker during dev session (local)", async () => {
it("can run and modify python worker during dev session (local)", async () => {

* The type of Worker
*/
export type CfScriptFormat = "modules" | "service-worker";
export type CfScriptFormat = "modules" | "service-worker" | "python";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is quite right—a python worker should still be a modules worker, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it really depends what the meaning for this type is. I think differentiating module (JS) vs. modules (Python) makes sense since the code which reads these ultimately does a completely different thing depending on its value. Maybe renaming "python" to "modules-python" would be best?

@codecov
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (a94d46f) 70.64% compared to head (f5c8c9e) 70.66%.
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4896      +/-   ##
==========================================
+ Coverage   70.64%   70.66%   +0.01%     
==========================================
  Files         292      292              
  Lines       15178    15204      +26     
  Branches     3860     3870      +10     
==========================================
+ Hits        10723    10744      +21     
- Misses       4455     4460       +5     
Files Coverage Δ
packages/wrangler/src/config/validation.ts 89.70% <ø> (ø)
...src/deployment-bundle/create-worker-upload-form.ts 90.07% <100.00%> (+0.14%) ⬆️
...ngler/src/deployment-bundle/guess-worker-format.ts 100.00% <100.00%> (ø)
...wrangler/src/deployment-bundle/run-custom-build.ts 100.00% <ø> (ø)
packages/wrangler/src/type-generation.ts 98.85% <ø> (ø)
packages/wrangler/src/dev/miniflare.ts 61.23% <63.63%> (-0.42%) ⬇️
packages/wrangler/src/deployment-bundle/bundle.ts 88.61% <71.79%> (+0.68%) ⬆️

... and 5 files with indirect coverage changes

@penalosa
Copy link
Contributor

penalosa commented Feb 1, 2024

This should probably include a default module rule mapping *.py to python

@penalosa penalosa added the e2e Run wrangler + vite-plugin e2e tests on a PR label Feb 1, 2024
@dom96
Copy link
Contributor Author

dom96 commented Feb 1, 2024

@penalosa can you elaborate on this? Where do I add it and what effect will it have?

@penalosa penalosa mentioned this pull request Feb 2, 2024
6 tasks
@dom96
Copy link
Contributor Author

dom96 commented Feb 2, 2024

Closing in favour of #4901

@dom96 dom96 closed this Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run wrangler + vite-plugin e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants