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

Hardcode support for platform pyodide_wasm32 #159

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Nov 22, 2024

Also cahnge some Generators to Tuple, as it is not the first time that while debugging I exaust the generator when printing logs; and this make micropip installing the package to fail.

I don't like it; this feels like a hack, but it works, and I woudl be happy to get more guidance.

Also one weird thing, is if I installl pywavelets nightly, it installs numpy stable; while the index does have a numpy nightly;

And with the same config, if I ask it to install numpy; it does install numpy nightly.

So there is something wonkey in recursive dependency handling of indexes.

@Carreau
Copy link
Contributor Author

Carreau commented Nov 22, 2024

You can test this patch with :

import logging
log = logging.getLogger('micropip')
log.setLevel(10)
import micropip
await micropip.install('pywavelets', index_urls=['https://cors.carreau.workers.dev/scientific-python-nightly-wheels/simple'])

(installs numpy stable, and pywavelets nightly)

import micropip
await micropip.install('numpy', index_urls=['https://cors.carreau.workers.dev/scientific-python-nightly-wheels/simple'])

(installs numpy nightly)

@Carreau
Copy link
Contributor Author

Carreau commented Nov 22, 2024

(I probably need to add a test, but feedback first would be welcome).

@ryanking13
Copy link
Member

Also one weird thing, is if I installl pywavelets nightly, it installs numpy stable; while the index does have a numpy nightly;
And with the same config, if I ask it to install numpy; it does install numpy nightly.
So there is something wonkey in recursive dependency handling of indexes.

Hmm, It must be a bug.

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, I left some comments. Could you also update the changelog?

releases_compatible = {
version: cls._compatible_wheels(files, version, name=name)
releases_compatible_x = {
version: tuple(cls._compatible_wheels(files, version, name=name))
Copy link
Member

Choose a reason for hiding this comment

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

I would like to keep the generator if possible, as checking compatibility of a long list of versions is redundant.

Comment on lines +217 to +218
if filename.endswith("wasm32.whl") and sys.platform == "emscripten":
return True
Copy link
Member

Choose a reason for hiding this comment

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

Maybe sys.platform check is redundant, as we never expect micropip to be used in the non-Emscripten environment.

Copy link
Member

Choose a reason for hiding this comment

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

At least not for installation. For making lock files, it would make sense.

Copy link
Member

Choose a reason for hiding this comment

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

For making lock files

Do you mean using it under pyodide venv environment? I think it would still have sys.platform == emscripten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well; at least some test locally runs under sys.platform != emscripten, and are useful for fast iteration.

new_tags = []
for tag in sys_tags_orig():
if "emscripten" in tag.platform:
new_tags.append(Tag(tag.interpreter, tag.abi, "pyodide_2024_0_wasm32"))
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use

import sysconfig
abi_version = sysconfig.get_config_var("PYODIDE_ABI_VERSION")
tag = f"pyodide_{abi_version}_wasm32"

instead, as abi version is expected to be changed when Pyodide ABI changes.

I don't like it; this feels like a hack, but it works, and I would be
happy to get more guidance.

Also one weird thing, is if I installl pywavelets nightly, it installs
numpy stable; while the index does have a numpy nightly;

And with the same config, if I ask it to install numpy; it does install
numpy nightly.

So there is something wonkey in recursive dependency handling of
indexes.
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. Codewise looks good to me. But could you please add a testcase for the updated sys_tags function?

There is a test function test_check_compatible but it runs under the host environment, hence no PYODIDE_ABI_VERSION config var exists.

So I would recommend to make a new test function that runs inside the Pyodide.

@run_in_pyodide
def test_check_compatible_in_pyodide(selenium_standalone_micropip):
    import micropip
    # run test against check_compatible()

@Carreau
Copy link
Contributor Author

Carreau commented Nov 26, 2024

I've added a single test that the pywavelet wheel that was not installing is marked as compatible when run in pyodide; is that sufficient ?

@ryanking13
Copy link
Member

I've added a single test that the pywavelet wheel that was not installing is marked as compatible when run in pyodide; is that sufficient ?

Yes, I think it is sufficient for now. Probably we can update the testcase when we update the emscripten or Python version in Pyodide.

@ryanking13 ryanking13 merged commit 5f01db3 into pyodide:main Nov 26, 2024
5 checks passed
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