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

Pyodide Conversion: fix requirements handling #6859

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

flxmr
Copy link

@flxmr flxmr commented May 22, 2024

This fixes: #6787 AND #4164 and adjusts the documentation

Why both? adding custom wheels as requirements requires making those available to the Pyodide environment at runtime. One could place them into the output structure as wheels to be served with the app, but this might clutter it a lot if there are many wheels (and also clash with any bundling system to be implemented). Also semantics with this solution seem clearer: you convert a panel app requiring custom wheels, you get a zip with dependencies and you distribute the app and the zip-file.

As the infrastructure for fixing #4164 was in place with this solution, I then also added a --resources-flag to the convert-command to include custom resources (such as template files/data files/...) required by the custom-python-portion of the panel app. Everything is packed into a single zipfile, because that's easiest to use with the pyscript config (which only allows to uncompress one zip to a specific wildcard-location); also I bumped the pyscript version, because that feature only got added with one of the small releases.

Currently BROKEN: on main, someone updated the version number in package.json which is used for getting the CDN_DIST-url. I think PY_VERSION in panel/io/convert.py (which references a python __version__ should also be updated accordingly and possibly also be acquired from a single-source-of-truth!

Reasons for restructuring:

  • installing the dependencies one-by-one with micropip for the pyodide-worker seems clever and gives some responsiveness. However this doesn't use custom packages installed in previous calls for dependency resolution (...) and thus does not work if you want to override specific dependencies with custom-builds!
  • I split the code for finding the requirements into a function collect_python_requirements (finding the requirements or parsing the requirements-file) and some code rewriting/collecting filesystem-URLs. script_to_html now takes a micropip-ready list of package-specifications!

Small things:

  • I would put the documentation for standalone-usage of panel in WASM in front of the conversion docs. Currently this is very weird clicking through, showing a conversion tool and then showing how to manually write your code.
  • I replaced some very inconsistent usage of single and doublequotes
  • as http-patch is in the pyodide-lockfile (from which micropip installs WASM-wheels) and this only ever includes one version (for a specific version of Pyodide), it doesn't make sense to version-lock this. Also removed the MIN_VERSIONS for this reason. Once pyodide implements proper requirements handling this should be put into the requirements-file like for every other project.

@flxmr flxmr changed the title Flxmr fix requirements handling Pyodide Conversion: fix requirements handling #6787 May 22, 2024
@flxmr flxmr changed the title Pyodide Conversion: fix requirements handling #6787 Pyodide Conversion: fix requirements handling May 22, 2024
@philippjfr
Copy link
Member

This is incredible, thanks for diving into this so deeply @flxmr! I'll have to dig into this a little bit to unpack it and I'm out until next week but would like to target this for the 1.5 release in early to mid June.

});
}
self.postMessage({type: 'status', msg: `Installing packages`});
// a finegrained approach installing dependencies one after another with status updates was implemented previously
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunate but better to handle this correctly. Would be great if micropip supported hooks to report progress.

Copy link
Author

@flxmr flxmr Jun 25, 2024

Choose a reason for hiding this comment

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

ah, that was not commented properly for the review! The problem is that the dependency resolution fails for locally installed wheels if they are not in the install command (...) - probably this should be fixed in micropip. I will try to do some tests too and poke the micropip-people a bit in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

Sweet thanks! Am trying to test this PR and then get it merged in time for the Panel 1.5 release.

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.

Pyodide Conversion: requirements.txt not able to deal with wheel-urls
2 participants