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

Upgrade to Pyodide 0.21.3 #887

Merged
merged 3 commits into from Oct 27, 2022
Merged

Conversation

JeffersGlass
Copy link
Member

Upgrade to Pyodide 0.21.3 as the default. The changelog from 0.21.2 -> 0.21.3 is mostly bug fixes (including a couple regressions in 0.21.2) - the only breaking changes involves how the soupsieve package loads.

The rest of the fixes - including loading packages in a more sensible order and being able to access the Pyodide version without having to use loadPyodide() seem minor but potentially useful.

@tedpatrick
Copy link
Contributor

tedpatrick commented Oct 26, 2022

odd test-ts error

(node:46904) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[Error: ENOENT: no such file or directory, open '/Users/ted/Dev/forks/JeffersGlass/pyscript/pyscriptjs/node_modules/src/js/pyodide_py.tar'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/ted/Dev/forks/[JeffersGlass](https://github.com/JeffersGlass/pyscript/pyscriptjs/node_modules/src/js/pyodide_py.tar'
}
 FAIL  tests/unit/runtime.test.ts

@JeffersGlass
Copy link
Member Author

Huh! Interesting… will investigate.

@JeffersGlass
Copy link
Member Author

It's curious to me that the error can't find a file in .../node_modules/src/js/... , which doesn't seem right... surely that should be some kind of path relative to an actual module?

From the main branch currently, if you do npm install, then check out this PR branch and build PyScript without reinstalling the node packages, it builds and tests fine, and runs with the Pyodide 0.21.3 runtime. Which maybe implies to me there's an upstream issue with how Pyodide is building its npm package currently, that was introduced after 0.21.2? I'm not fully confident in my ability to describe the issue though, so I'm trying to produce a minimal example.

I've been looking for an excuse to set up a Pyodide build environment, so investigating that too, as a way of digging into this.

@antocuni
Copy link
Contributor

this very vaguely rings a bell to me. I think that @madhur-tandon had problems when migrating to 0.21.2 because pyodide changed the way it located certain files.
Maybe this is the same kind of problem, or maybe completely unrelated

@madhur-tandon
Copy link
Member

@madhur-tandon had problems when migrating to 0.21.2

Yes, this is possible it's the same issue. Basically, the unit tests run inside node while integration tests run through selenium. The unit tests here use the node package of pyodide which should be the main culprit I guess.

CC: @JeffersGlass

@antocuni
Copy link
Contributor

while integration tests run through selenium.

I guess you mean playwright here

@madhur-tandon
Copy link
Member

madhur-tandon commented Oct 27, 2022

I guess you mean playwright here

Yeah, I guess I typed in hurry 😓
And that's probably because pyodide uses selenium while we use playwright.

@madhur-tandon
Copy link
Member

@JeffersGlass I did something similar in my previous attempt -- using the indexURL property, I referred to the same place you linked in the comments i.e.
https://github.com/pyodide/pyodide/blob/7dfee03a82c19069f714a09da386547aeefef242/src/js/pyodide.ts#L179
However, for me, this affected loading a local pyodide build at that time. So, if the test

# The default pyodide version is 0.21.2 as of writing
# this test which is newer than the one we are loading below
# (after downloading locally) -- which is 0.20.0
# The test checks if loading a different runtime is possible
# and that too from a locally downloaded file without needing
# the use of explicit `indexURL` calculation.
def test_runtime_config(self, tar_location):
unzip(
location=tar_location,
extract_to=self.tmpdir,
)
self.pyscript_run(
"""
<py-config type="json">
{
"runtimes": [{
"src": "/pyodide/pyodide.js",
"name": "pyodide-0.20.0",
"lang": "python"
}]
}
</py-config>
<py-script>
import sys, js
pyodide_version = sys.modules["pyodide"].__version__
js.console.log("version", pyodide_version)
display(pyodide_version)
</py-script>
""",
)
assert self.console.log.lines == [self.PY_COMPLETE, "version 0.20.0"]
version = self.page.locator("py-script").inner_text()
assert version == "0.20.0"
passes for you, then we should be good to go I guess.

Copy link
Contributor

@tedpatrick tedpatrick left a comment

Choose a reason for hiding this comment

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

Perfect

Copy link
Member

@madhur-tandon madhur-tandon left a comment

Choose a reason for hiding this comment

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

Seems like the test for loading a local pyodide passes, approving 🗡️

@JeffersGlass
Copy link
Member Author

JeffersGlass commented Oct 27, 2022

Y'all were exactly right, the issue was in differences between running in Jest vs in selenium/browser.

It turns out Pyodide's method of determining the base URL/location to load additional files from is a bit of a hack, though a clever one - if you don't provide an indexURL parameter to loadPyodide, it throws an error, catches it, grabs the file path from the error, and throws away everything after the last '/'. The filename of the error is different when the tests are running in Jest, which breaks indexURL. The fix is to set indexURL specifically for the test.

I believe this broke in 0.21.3 build (and not 0.21.2) because some files (including pyodide_py.tar) are now distributed as separate files, while I think they which they weren't in 0.21.2, and Jest didn't know how to locate them.

Edit: @madhur-tandon Beat me to posting! Thanks again for pointing me in the right direction.

@tedpatrick
Copy link
Contributor

Yours to merge @JeffersGlass

@JeffersGlass JeffersGlass merged commit 4850f39 into pyscript:main Oct 27, 2022
@JeffersGlass JeffersGlass deleted the pyodide-21-3 branch October 27, 2022 20:16
@madhur-tandon
Copy link
Member

I believe this broke in 0.21.3 build (and not 0.21.2)

What is strange is that it broke in 0.21.1 too I guess, so 0.21.2 was the only one which worked IIRC xD
apart from earlier ones such as 0.20.

You can refer some of my previous conclusions here: #734 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants