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

run CI with Pyodide 0.26.4 and 0.27.2, update docs for Pyodide 0.27, and miscellaneous maintenance updates #146

Merged
merged 31 commits into from
Feb 22, 2025

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Feb 18, 2025

Description

  • version 3 for the upload-artifact and download-artifact GitHub Actions have been sunset. This pull request fixes the CI failure by switching to v4.
  • I updated the docs to reference Pyodide 0.27 instead of older releases (there's a chance I might have missed something, though).

As noticed in #145.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks! It seems like some part of the CI is still broken, but probably we can fix it one by one.

@agriyakhetarpal
Copy link
Member Author

Thanks for the review! Could we drop 0.24.1 and 0.25.1, as they are more than a year old at this point? I'm working on fixing pre-commit at the moment, but I can take a look at the rest of the issues as well.

@ryanking13
Copy link
Member

Thanks for the review! Could we drop 0.24.1 and 0.25.1, as they are more than a year old at this point? I'm working on fixing pre-commit at the moment, but I can take a look at the rest of the issues as well.

Yes, feel free to drop old versions. You can update the version tested in CI and update the table in compatibility.md accordingly.

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Feb 18, 2025

The tests on Safari fail with this message:

ModuleNotFoundError: No module named 'pathlib._local'; 'pathlib' is not a package

But I don't see any uses of pathlib._local in our codebase or in the tests. Do you have an idea about it?

@agriyakhetarpal
Copy link
Member Author

And Chrome hasn't started running the tests. I expect that it will time out.

@agriyakhetarpal
Copy link
Member Author

Never mind – it didn't time out, but we have a test that fails as such:

@run_in_pyodide
def test_jspi(selenium_jspi):
    from js import WebAssembly

    assert hasattr(WebAssembly, "Suspender")

which I think we should revisit, as the JSPI API was changed: https://v8.dev/blog/jspi-newapi. @hoodmane previously raised an issue about the use of WebAssembly.Suspender: WebAssembly/js-promise-integration#21. I traced this to GoogleChromeLabs/wasm-feature-detect@b8f4668.

@agriyakhetarpal
Copy link
Member Author

I'm not too sure how to go about debugging the other test:

ERROR examples/test_install_package.py::test_install_from_pypi[chrome] - selenium.common.exceptions.TimeoutException: Message: timeout
from no such execution context: frame does not have execution context
  (Session info: chrome=133.0.6943.53)

@ryanking13
Copy link
Member

ryanking13 commented Feb 18, 2025

But I don't see any uses of pathlib._local in our codebase or in the tests. Do you have an idea about it?

Since it is a testcase using hypothesis, I guess hypothesis is passing random host Python object to the Pyodide environment.

And I also found that Python 3.13 is installed in the host environment for the Safari test (maybe the GHA macos runner has been updated).

So, probably the error happens because Python 3.13's pathlib object is incompatible with Python 3.12. It is not an error IMO, I think we need to fix GHA to use the same Python version instead.

@ryanking13
Copy link
Member

ERROR examples/test_install_package.py::test_install_from_pypi[chrome] - selenium.common.exceptions.TimeoutException: Message: timeout
from no such execution context: frame does not have execution context
  (Session info: chrome=133.0.6943.53)

For this one, I don't understand why it timeouts, but maybe let's just remove that test from the example directory. I don't think we need a test that queries PyPI.

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Feb 18, 2025

But I don't see any uses of pathlib._local in our codebase or in the tests. Do you have an idea about it?

Since it is a testcase using hypothesis, I guess hypothesis is passing random host Python object to the Pyodide environment.

And I also found that Python 3.13 is installed in the host environment for the Safari test (maybe the GHA macos runner has been updated).

So, probably the error happens because Python 3.13's pathlib object is incompatible with Python 3.12. It is not an error IMO, I think we need to fix GHA to use the same Python version instead.

That makes sense! I will try to fix that! I confirmed and your observation is in line with what I observe locally with Python 3.13 and Python 3.12:

Python 3.13.1 (main, Dec  3 2024, 17:59:52) [Clang 16.0.0 (clang-1600.0.26.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pathlib
>>> pathlib._local
<module 'pathlib._local' from '/opt/homebrew/Cellar/[email protected]/3.13.1/Frameworks/Python.framework/Versions/3.13/lib/python3.13/pathlib/_local.py'>
Python 3.12.7 | packaged by conda-forge | (main, Oct  4 2024, 15:57:01) [Clang 17.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pathlib
>>> pathlib._local
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'pathlib' has no attribute '_local'

@agriyakhetarpal
Copy link
Member Author

Apparently, GHA is adamant and keeps running the Safari tests with Python 3.13 instead, even after requesting Python 3.12.

@ryanking13
Copy link
Member

Thanks for handling the issues Agriya.

I guess this comment is related to out situation. Maybe we need to set the default shell and remove -l option from bash?

@agriyakhetarpal
Copy link
Member Author

Ah, that's interesting, thanks, @ryanking13. I would never have thought of that being a problem. 🙈

I think we are close to fixing all the issues here. One problem is that the fix I noted in #146 (comment) doesn't yet work (maybe because we are using an older Node version or something). Another problem is that Chrome has started to time out after six hours, as noted in the last few CI runs.

@ryanking13
Copy link
Member

(maybe because we are using an older Node version or something).

Yeah, it is probably the opposite. The Node version installed in the CI has probably updated to a newer version (with a newer JSPI), which makes the test break. I think we can xfail that test for now. It is not a critical test for pytest-pyodide IMO.

@ryanking13
Copy link
Member

For chrome issue, I think we had a same issue in micropip. For now, we pin the chrome version in micropip as a workaround. I think we can do the same thing. (Please also update the install_browser action version to v2 . v1 has a bug that it ignores the version specified in the parameter).

@agriyakhetarpal
Copy link
Member Author

Oh yes, I remember that one. I was thinking of doing that as a last resort, but I'm okay with doing so to get the CI passing here, as other PRs rely on this change.

@ryanking13
Copy link
Member

Oh, you need to fix this file

https://github.com/pyodide/pytest-pyodide/blob/main/utils/build_test_matrix.py

to update the versions used in the CI.

@agriyakhetarpal
Copy link
Member Author

It looks like Chrome 131 is going to time out at some point – it shouldn't take this long to pass the tests. Should we go lower?

@agriyakhetarpal agriyakhetarpal changed the title chore: use download-artifact@v4 for CI, update docs for Pyodide 0.27 run CI with Pyodide 0.26.4 and 0.27.2, update docs for Pyodide 0.27, and miscellaneous maintenance updates Feb 21, 2025
@hoodmane
Copy link
Member

For stack switching we should be able to check for either the old way or the new way and I'd hope that'd work.

@ryanking13
Copy link
Member

It looks like Chrome 131 is going to time out at some point – it shouldn't take this long to pass the tests. Should we go lower?

Yeah, maybe let's use 125 just same as micropip for now.

Co-Authored-By: Gyeongjae Choi <[email protected]>
@agriyakhetarpal
Copy link
Member Author

Okay, I think it should be ready now. I'll double-check and merge it as other PRs are blocked on this. Thanks for the guidance, @ryanking13 and @hoodmane!

@agriyakhetarpal agriyakhetarpal merged commit e26e780 into pyodide:main Feb 22, 2025
14 checks passed
@agriyakhetarpal agriyakhetarpal deleted the fix/ci branch February 22, 2025 08:57
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.

3 participants