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

Provide pyodide packages #285

Closed
twiecki opened this issue May 4, 2023 · 19 comments · Fixed by #918
Closed

Provide pyodide packages #285

twiecki opened this issue May 4, 2023 · 19 comments · Fixed by #918

Comments

@twiecki
Copy link
Member

twiecki commented May 4, 2023

Description

Would be great if we could pip install pytensor on pyodide. We can build without the cython extension and it works, however, we should create proper pyodide packages https://pyodide.org/en/0.19.1/development/new-packages.html.

@lucianopaz
Copy link
Contributor

@twiecki, the pypi workflow already builds wheels and uploads them to pypi. Maybe the issue is that you need a special kind of wheel build for pyodide? I don't really know how that works, but I'll edit this issue's title accordingly

@lucianopaz lucianopaz changed the title Publish wheels on PyPI Publish wheels for pyodide on PyPI May 5, 2023
@twiecki
Copy link
Member Author

twiecki commented May 5, 2023

They need to be non-binary / universal as I found out. But apparently this isn't possible with extensions...

@twiecki
Copy link
Member Author

twiecki commented May 5, 2023

OK, removing the Extension in setup.py allowed me to build a universal wheel:

    setup(
        name=NAME,
        version="2.11.2", #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()],
        #    ),
        #],
    )

And then:
python setup.py bdist_wheel --universal
Followed by twine upload dist/pytensor-2.11.2-cp310-cp310-macosx_11_0_arm64.whl.

Would be nice to somehow automate that process, easiest would be a separate setup.py that doesn't have the Extension (which is optional in Python mode).

But with this, we get native PyMC running in the browser:
image

@lucianopaz
Copy link
Contributor

Did you just upload a universal wheel to PYPI and not to test-pypi? This is bad and we shouldn't do it. Wheels will always have precedence over source distributions. We build and supply wheels for some platforms and OS versions. If a user is using a different version of the OS or a different platform, calling pip install will grab the wheel, which will not include the compiled extensions! That means that said users will end up with an incomplete installation (it looks like they might be missing the scan with the C linker?). I strongly prefer them to get the full package but compiling it from source. That means that we should remove the universal wheel you added as soon as possible, so that users don't get faulty installations.

@twiecki
Copy link
Member Author

twiecki commented May 5, 2023

@lucianopaz Thanks for pointing that out, I deleted the wheel. Any idea for how to get non-binary wheels for pyodide only then?

@lucianopaz
Copy link
Contributor

This seems to have the instructions for building the pyodide package. I've never used it, but they say that they have a toolchain that could build the js stuff for us. I have no idea where it gets uploaded afterwards.

@twiecki
Copy link
Member Author

twiecki commented May 5, 2023

OK, building a proper pyodide package from the source you pointed to seems like the proper way to go. I gave it an honest effort but failed to make pyodide (pyodide/pyodide#3829).

@twiecki twiecki changed the title Publish wheels for pyodide on PyPI Provide pyodide packages May 5, 2023
@twiecki
Copy link
Member Author

twiecki commented May 6, 2023

OK, while I got a pyodide package to work, there is no way to serve it, so we can't pip install it. So universal wheels is the easiest and most user-friendly way to go.

I actually don't think that having non-binary wheels up on PyPI is a big deal. By default, the binary versions will get installed, and where that's not the case, the user gets a big fat warning. wdyt @lucianopaz?

@twiecki
Copy link
Member Author

twiecki commented May 6, 2023

Oh, we can also just add the cython-compiled c-file to the package as well, so it's actually not an issue! I'm going to re-upload the universal wheel, I don't think there's anything that will break.

@ricardoV94
Copy link
Member

@twiecki shall we close this issue or is there stuff we could/should do still?

@twiecki
Copy link
Member Author

twiecki commented Dec 7, 2023

I think this would still be useful to have.

@ricardoV94
Copy link
Member

What is the feature request exactly?

@twiecki
Copy link
Member Author

twiecki commented Dec 7, 2023

Ideally we figure out how to upload universal wheels when we release, but that requires a different setup.py and I haven't found a way to make that clean.

@twiecki
Copy link
Member Author

twiecki commented Dec 7, 2023

And to your question: Request is to be able to pip install pytensor in pyodide/pyscript.

@lucianopaz
Copy link
Contributor

We can't make pytensor behave as a universal wheel because it has a cython extension. What we need to do, is to add a build step that actually builds pytensor using pyodide. The documentation for that is here and here, and they reference numpy's build action for emscripten. I don't have time to look into this, and it seems challenging, but doable.

@twiecki
Copy link
Member Author

twiecki commented Dec 7, 2023

It worked without the cython extension.
The other "proper" path is a bit cumbersome as we first need to build it, and then add it to emscripten.

@twiecki
Copy link
Member Author

twiecki commented Jul 8, 2024

Another solution could be to revert #77. I think that would be easiest and most stable. That PR made Cython non-optional which is a down-side, especially as we switch away from C.

How do people feel about that? CC @ricardoV94

@ricardoV94
Copy link
Member

ricardoV94 commented Jul 8, 2024

Is there a way to have a pip install pytensor[no_c] which is not default and does not require Cython? And would that work for pyodide?

@twiecki
Copy link
Member Author

twiecki commented Jul 8, 2024

I think that should work for pyodide. I'm not sure how to configure that, however.

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