Skip to content

GH-41910: [Python] Add support for Pyodide - #37822

Merged
kou merged 140 commits into
apache:mainfrom
joemarshall:emscripten_python_changes
Jul 5, 2024
Merged

GH-41910: [Python] Add support for Pyodide#37822
kou merged 140 commits into
apache:mainfrom
joemarshall:emscripten_python_changes

Conversation

@joemarshall

@joemarshalljoemarshall commented Sep 21, 2023

Copy link
Copy Markdown
Contributor

pyarrow knows about ARROW_ENABLE_THREADING and doesn't use threads if they are not enabled in libarrow.

Split from #37696

@joemarshall

Copy link
Copy Markdown
ContributorAuthor

@kou And these are the python changes

@jorisvandenbosschejorisvandenbossche changed the title GH23221 - python changes for pyodide buildGH-23221: [Python] python changes for pyodide buildSep 25, 2023
@apacheapache deleted a comment from github-actionsBotSep 25, 2023
@jorisvandenbossche

Copy link
Copy Markdown
Member

@joemarshall thanks for the PR!

We might want to expose is_threading_enabled() in pyarrow publicly (it might be useful for downstream packages as well?), in __init__.py

2. pyarrow sets defaults for inclusion of submodules based on their inclusion in the arrow build. e.g. pyarrow.parquet is built only if ARROW_PARQUET is set. This makes it possible to build in situations where you don't have access to set the build environment variables (e.g. in cross compiling situations like pyodide).

This part is not actually included here? (or I don't understand the sentence)
(and it might also make sense to leave that for a third PR since it is changing the build process beyond emscriptem?)

@joemarshall

Copy link
Copy Markdown
ContributorAuthor

@joemarshall thanks for the PR!

We might want to expose is_threading_enabled() in pyarrow publicly (it might be useful for downstream packages as well?), in __init__.py

  1. pyarrow sets defaults for inclusion of submodules based on their inclusion in the arrow build. e.g. pyarrow.parquet is built only if ARROW_PARQUET is set. This makes it possible to build in situations where you don't have access to set the build environment variables (e.g. in cross compiling situations like pyodide).

This part is not actually included here? (or I don't understand the sentence) (and it might also make sense to leave that for a third PR since it is changing the build process beyond emscriptem?)

Sorry, I missed out putting in the setup.py changes. They're in now.

About is_threading_enabled(), it is currently in pyarrow.lib.is_threading_enabled(). Does it need to be top-level?

@joemarshall

joemarshall commented Sep 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Oh and for now I have blocked the auto-setting of PYARROW_* to happen only on emscripten - I don't know if that makes sense or not, but it isn't possible to build for emscripten without that change or something similar right now.

Comment threadpython/CMakeLists.txt Outdated
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 3, 2023
Comment threadpython/pyarrow/io.pxi Outdated
Comment threadpython/pyarrow/lib.pyx
Comment threadpython/setup.py Outdated
Comment threadpython/pyarrow/parquet/core.py Outdated
Comment threadpython/pyarrow/pandas_compat.py Outdated
Comment threadpython/CMakeLists.txt Outdated
@github-actionsgithub-actionsBot removed the awaiting committer review Awaiting committer review label Oct 3, 2023
@kou

kou commented Jul 3, 2024

Copy link
Copy Markdown
Member

@jorisvandenbossche@pitrou Do you want to review the Python part?

@ianmcook

Copy link
Copy Markdown
Member

@kou I believe Joris and Antoine are mostly away this week. Joris reviewed this earlier and communicated that he would like this to be merged in time for 17.0.0 if possible. Let's please not block waiting for additional reviews of the Python parts, unless you have concerns.

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK. I've reviewed the Python part too.

Comment threadpython/pyarrow/_parquet.pyx Outdated
Comment threadpython/pyarrow/conftest.py Outdated
Comment threadpython/pyarrow/error.pxi Outdated
Comment threadpython/pyarrow/io.pxi
Comment threadpython/pyarrow/io.pxi
Comment threadpython/scripts/run_emscripten_tests.py Outdated
Comment threadpython/scripts/run_emscripten_tests.py Outdated
Comment threadpython/scripts/run_emscripten_tests.py Outdated

print("Load pyodide in browser")
driver.load_pyodide(dist_dir)
print("Done\n")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is \n needed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@joemarshall I see the print("Done") is removed entirely now. Is that what you intended?

Comment threadpython/scripts/run_emscripten_tests.py Outdated
@ianmcook

Copy link
Copy Markdown
Member

@joemarshall if you're able to react to Kou's review in the next day or so, then I think we can get this merged in time for the 17.0.0 release

@joemarshall

Copy link
Copy Markdown
ContributorAuthor

@joemarshall if you're able to react to Kou's review in the next day or so, then I think we can get this merged in time for the 17.0.0 release

Brilliant, I'll get onto it either later on today or first thing tomorrow

joemarshalland others added 4 commits July 5, 2024 09:36
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@joemarshall

Copy link
Copy Markdown
ContributorAuthor

@ianmcook changes merged in, running tests locally right now, could probably fire off the emscripten tests here to be sure also

@joemarshall

Copy link
Copy Markdown
ContributorAuthor

Tests pass here, so hopefully we're ready to merge.

@ianmcook

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-conda-python-emscripten

@github-actions

Copy link
Copy Markdown

Revision: fa0e497

Submitted crossbow builds: ursacomputing/crossbow @ actions-e5764a79dd

TaskStatus
test-conda-python-emscriptenGitHub Actions

@ianmcook

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g wheel

@github-actions

Copy link
Copy Markdown

Revision: fa0e497

Submitted crossbow builds: ursacomputing/crossbow @ actions-a1c5cd96da

TaskStatus
wheel-macos-big-sur-cp310-arm64GitHub Actions
wheel-macos-big-sur-cp311-arm64GitHub Actions
wheel-macos-big-sur-cp312-arm64GitHub Actions
wheel-macos-big-sur-cp38-arm64GitHub Actions
wheel-macos-big-sur-cp39-arm64GitHub Actions
wheel-macos-catalina-cp310-amd64GitHub Actions
wheel-macos-catalina-cp311-amd64GitHub Actions
wheel-macos-catalina-cp312-amd64GitHub Actions
wheel-macos-catalina-cp38-amd64GitHub Actions
wheel-macos-catalina-cp39-amd64GitHub Actions
wheel-manylinux-2-28-cp310-amd64GitHub Actions
wheel-manylinux-2-28-cp310-arm64GitHub Actions
wheel-manylinux-2-28-cp311-amd64GitHub Actions
wheel-manylinux-2-28-cp311-arm64GitHub Actions
wheel-manylinux-2-28-cp312-amd64GitHub Actions
wheel-manylinux-2-28-cp312-arm64GitHub Actions
wheel-manylinux-2-28-cp38-amd64GitHub Actions
wheel-manylinux-2-28-cp38-arm64GitHub Actions
wheel-manylinux-2-28-cp39-amd64GitHub Actions
wheel-manylinux-2-28-cp39-arm64GitHub Actions
wheel-manylinux-2014-cp310-amd64GitHub Actions
wheel-manylinux-2014-cp310-arm64GitHub Actions
wheel-manylinux-2014-cp311-amd64GitHub Actions
wheel-manylinux-2014-cp311-arm64GitHub Actions
wheel-manylinux-2014-cp312-amd64GitHub Actions
wheel-manylinux-2014-cp312-arm64GitHub Actions
wheel-manylinux-2014-cp38-amd64GitHub Actions
wheel-manylinux-2014-cp38-arm64GitHub Actions
wheel-manylinux-2014-cp39-amd64GitHub Actions
wheel-manylinux-2014-cp39-arm64GitHub Actions
wheel-windows-cp310-amd64GitHub Actions
wheel-windows-cp311-amd64GitHub Actions
wheel-windows-cp312-amd64GitHub Actions
wheel-windows-cp38-amd64GitHub Actions
wheel-windows-cp39-amd64GitHub Actions

@ianmcook

Copy link
Copy Markdown
Member

@joemarshall could you please sync your fork with upstream main? I think that will resolve the CI failures.

@joemarshall

Copy link
Copy Markdown
ContributorAuthor

@joemarshall could you please sync your fork with upstream main? I think that will resolve the CI failures.

Try now

@raulcd

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g wheel

@github-actions

Copy link
Copy Markdown

Revision: 71a2f6a

Submitted crossbow builds: ursacomputing/crossbow @ actions-8f6301a1d5

TaskStatus
wheel-macos-big-sur-cp310-arm64GitHub Actions
wheel-macos-big-sur-cp311-arm64GitHub Actions
wheel-macos-big-sur-cp312-arm64GitHub Actions
wheel-macos-big-sur-cp38-arm64GitHub Actions
wheel-macos-big-sur-cp39-arm64GitHub Actions
wheel-macos-catalina-cp310-amd64GitHub Actions
wheel-macos-catalina-cp311-amd64GitHub Actions
wheel-macos-catalina-cp312-amd64GitHub Actions
wheel-macos-catalina-cp38-amd64GitHub Actions
wheel-macos-catalina-cp39-amd64GitHub Actions
wheel-manylinux-2-28-cp310-amd64GitHub Actions
wheel-manylinux-2-28-cp310-arm64GitHub Actions
wheel-manylinux-2-28-cp311-amd64GitHub Actions
wheel-manylinux-2-28-cp311-arm64GitHub Actions
wheel-manylinux-2-28-cp312-amd64GitHub Actions
wheel-manylinux-2-28-cp312-arm64GitHub Actions
wheel-manylinux-2-28-cp38-amd64GitHub Actions
wheel-manylinux-2-28-cp38-arm64GitHub Actions
wheel-manylinux-2-28-cp39-amd64GitHub Actions
wheel-manylinux-2-28-cp39-arm64GitHub Actions
wheel-manylinux-2014-cp310-amd64GitHub Actions
wheel-manylinux-2014-cp310-arm64GitHub Actions
wheel-manylinux-2014-cp311-amd64GitHub Actions
wheel-manylinux-2014-cp311-arm64GitHub Actions
wheel-manylinux-2014-cp312-amd64GitHub Actions
wheel-manylinux-2014-cp312-arm64GitHub Actions
wheel-manylinux-2014-cp38-amd64GitHub Actions
wheel-manylinux-2014-cp38-arm64GitHub Actions
wheel-manylinux-2014-cp39-amd64GitHub Actions
wheel-manylinux-2014-cp39-arm64GitHub Actions
wheel-windows-cp310-amd64GitHub Actions
wheel-windows-cp311-amd64GitHub Actions
wheel-windows-cp312-amd64GitHub Actions
wheel-windows-cp38-amd64GitHub Actions
wheel-windows-cp39-amd64GitHub Actions

@ianmcook

Copy link
Copy Markdown
Member

Checks look good to me. The CI failures are unrelated.

@kou do you think this is good to merge now? If so please go ahead.

kou
kou approved these changes Jul 5, 2024

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2de8008.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 31 possible false positives for unstable benchmarks that are known to sometimes produce them.

@jorisvandenbossche

Copy link
Copy Markdown
Member

Thanks @joemarshall for the amazing effort and everyone for the reviews!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@joemarshall@jorisvandenbossche@ianmcook@kou@bitsondatadev@cpcloud@raulcd@pitrou