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.

Sign up for GitHub

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

Jump to bottom

Add building of pyodide universal wheels #918

Merged
merged 10 commits into from
Aug 10, 2024
+48 −7
Merged

Add building of pyodide universal wheels #918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6d1546c
Add building of pyodide universal wheels
twiecki Jul 10, 2024
2417282
precommit
twiecki Jul 10, 2024
42b333e
Fix precommit. Readd comment.
twiecki Jul 10, 2024
29b59bc
Fix precommit2
twiecki Jul 10, 2024
cc39616
Minor improvement to ext_modules conditional definition
maresb Jul 11, 2024
5e3bab6
Bump Python version so that tomllib is included
maresb Jul 11, 2024
b61d048
Add wheel package to build dependencies
maresb Jul 11, 2024
1eb1586
Update .github/workflows/pypi.yml
twiecki Aug 9, 2024
68e5338
Revert unnecessary
twiecki Aug 9, 2024
19cf45e
ruff
twiecki Aug 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .github/workflows/pypi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,31 @@ jobs:
name: wheels-${{ matrix.platform }}
path: ./wheelhouse/*.whl

build_universal_wheel:
name: Build universal wheel for Pyodide
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: '3.11'

- name: Install dependencies
run: pip install numpy versioneer wheel

- name: Build universal wheel
run: |
PYODIDE=1 python setup.py bdist_wheel --universal

- uses: actions/upload-artifact@v4
with:
name: universal_wheel
path: dist/*.whl

check_dist:
name: Check dist
needs: [make_sdist,build_wheels]
Expand Down Expand Up @@ -103,6 +128,11 @@ jobs:
path: dist
merge-multiple: true

- uses: actions/download-artifact@v4
with:
name: universal_wheel
path: dist

- uses: pypa/[email protected]
with:
user: __token__
Expand Down
25 changes: 18 additions & 7 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#!/usr/bin/env python
import os

import numpy
import versioneer
from setuptools import Extension, setup
Expand All @@ -11,17 +13,26 @@

NAME: str = dist.get_name() # type: ignore

# Check if building for Pyodide
is_pyodide = os.getenv("PYODIDE", "0") == "1"
maresb marked this conversation as resolved.
Show resolved Hide resolved

if is_pyodide:
# For pyodide we build a universal wheel that must be pure-python
# so we must omit the cython-version of scan.
ext_modules = []
else:
ext_modules = [
Extension(
name="pytensor.scan.scan_perform",
sources=["pytensor/scan/scan_perform.pyx"],
include_dirs=[numpy.get_include()],
),
]
Comment on lines +19 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on an environment variable to conditionally include an extension is very unorthodox and I don't think that it will be robust in the future. As far as I know, there is no canonical way to conditionally include an extension or not when using pyproject.toml or the older setup.py. However, there is a way to get platform conditional requirements following this pep. Maybe we could look at the sys information to identify that python is running from the browser instead of having is_pyodide conditioned on the environment?

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 extremely pro-declarative config, so I'd really like to do this better.

Unfortunately after taking a brief look through the environment markers, I don't see any particular variables that can help in our situation. This feels not so surprising to me since "universal" vs "platform-specific" seems more like a "build" parameter than an "environment" parameter, making it something to be handled at the build-backend (setuptools) level rather than the project-config level. On the same note, the pyproject.toml standard doesn't seem to support ext_modules. So I don't have any ideas for how we could do this better.

Also, unfortunately using ext_modules seems to marry us to setuptools which I loathe, but that's out-of-scope for this PR.


if __name__ == "__main__":
setup(
name=NAME,
version=versioneer.get_version(),
cmdclass=versioneer.get_cmdclass(),
ext_modules=[
Extension(
name="pytensor.scan.scan_perform",
sources=["pytensor/scan/scan_perform.pyx"],
include_dirs=[numpy.get_include()],
),
],
ext_modules=ext_modules,
)
Loading