Add building of pyodide universal wheels

[twiecki]

Closes #285 and #360.

These require pure-python wheels which does not work if we ship a cython-version of scan, so this build does not include this extension module.

[maresb]

Looks pretty straightforward. Does anyone know offhand it a platform-specific wheel takes precedence over a universal one? I'd hope it does.

[twiecki]

Looks pretty straightforward. Does anyone know offhand it a platform-specific wheel takes precedence over a universal one? I'd hope it does.

Yes, they take precedence. And since we have wheels for all major platforms, the universal one should really only ever be installed when installed through pyodide.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.69%. Comparing base (7fffec6) to head (19cf45e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #918   +/-   ##
=======================================
  Coverage   81.69%   81.69%           
=======================================
  Files         182      182           
  Lines       47585    47585           
  Branches    11584    11584           
=======================================
  Hits        38875    38875           
- Misses       6518     6520    +2     
+ Partials     2192     2190    -2     

see 2 files with indirect coverage changes

[ricardoV94]

To clarify, did this PR addressed the concerns here: #285 (comment) ?

[maresb]

We can't make pytensor behave as a universal wheel because it has a cython extension.

I believe the idea here is to sidestep the issue by omitting the cython extension. (I don't understand the implications of omitting it.)

[twiecki]

To clarify, did this PR addressed the concerns here: #285 (comment) ?

I think so, yes. Ultimately, having a cython-extension really only makes sense if using the c-backend. For the python backend there shouldn't be any dependency that's related to compilation.

[twiecki]

Thanks for the assist @maresb!

[maresb]

I don't think we're testing the universal dist. Probably we should in order to avoid potentially uploading corrupt wheels.

Also, while trying to determine whether or not we're testing, I see that we're using the sloppy old versions for artifact upload/download, see #560, #580.

[maresb]

@lucianopaz, I'm very interested to get your thoughts on this approach. Is it viable to simply omit the extension when building a universal wheel?

[lucianopaz]

As far as I understand, the code here should work. It will produce a universal wheel without the cython extension for scan. I think that the setup.py could be improved so that it doesn't rely on environment variables but that is more of a detail.
My bigger concern is the same that I said originally: building a universal wheel with reduced capabilities will do more harm than good in the long run. As pointed out here, universal wheels will be preferred by pip over platform specific wheels. This means that, any installation will need to require a platform specific version of pytensor manually to avoid getting the universal wheel. The correct way to deal with this is to get pytensor built using the pyodide build toolchain so that we have a platform specific version that can run on the browser.

[lucianopaz]

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?

[maresb]

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.

[maresb]

My reading of the stack overflow answer is that the priority is:

platform-specific compatible wheel > universal wheel > sdist

[twiecki]

As pointed out here, universal wheels will be preferred by pip over platform specific wheels.

I don't think that's the case. And I have confirmed that non-pyodide installs work the same across all major platforms.

I don't think the risk is that high to give this a try. We can test to make sure we don't break anything on existing platforms.

[lucianopaz]

My reading of the stack overflow answer is that the priority is:

platform-specific compatible wheel > universal wheel > sdist

Oh! You’re right, I misread the last sentence

This is not all that common however, as a universal wheel would be preferred over a source distribution!

So platform specific wheels will be preferred, then universal wheels and finally source distributions. I take back what I said about the risks then. This should be mostly fine.

[twiecki]

Thanks @lucianopaz. Can we get an approval?

[maresb]

I'm a bit uncomfortable that we're not testing the universal wheel. Maybe I can finish off #560 right now...

[twiecki]

FAILED tests/test_config.py::test_config_pickling - AssertionError: Regex pattern did not match.
 Regex: "Can't pickle local object"
 Input: "Can't get local object 'test_config_pickling.<locals>.<lambda>'"
= 1 failed, 807 passed, 69 skipped, 4 xfailed, 33 warnings in 248.68s (0:04:08) =
Error: Process completed with exit code 1.

what might this be about?