-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ensure that the SymPy version always remains up to date #26
Ensure that the SymPy version always remains up to date #26
Conversation
Hi @oscarbenjamin, this enhancement might be of interest to you based on our recent correspondence. Could you please take a look when you have time? Thank you! |
check_sympy_version.py
Outdated
def main(): | ||
pyodide_version = get_pyodide_lock() | ||
pypi_version = get_pypi_version() | ||
|
||
print(f"SymPy version in Pyodide: {pyodide_version}") | ||
print(f"Latest PyPI SymPy version: {pypi_version}") | ||
|
||
should_download = Version(pypi_version) > Version(pyodide_version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why all of this is needed. Would it not make sense to just run pip download sympy
and always use the latest final release?
I'm not sure why the pyodide version would be relevant at all or is there some advantage in using it from pyodide rather than from PyPI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me, I will refactor the script. TIL that a pip download
command exists!
The idea is to add the %pip install sympy
command only when it is needed, as it is redundant otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to add the
%pip install sympy
command only when it is needed, as it is redundant otherwise.
I don't understand when/where that command is being run. Is it running in the end users browser? Or is it just run locally as part of building the site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first one – the command runs in the end user's browser, when the REPL is instantiated.
So, we now have two options: A. 3bfc047, which goes halfway and adds the Which one would you prefer? B is much nicer, I feel. |
Do these options have any implications for performance or anything else? Generally my preference is to use the latest release of SymPy always and not worry about the version in pyodide. |
Note: please see the "Tip" admonition below for a TL;DR. Good question – I don't think performance will be impacted at all, at least at a level that will be glaringly noticeable, as the SymPy wheel will be shipped alongside JupyterLite's static files and not downloaded externally from Pyodide's CDN. I did try to benchmark this in the dev console, and the installation for the wheel for Pyodide 0.26.4 (where I loaded SymPy from the local download) was almost instant, coming at 96.98 ms from time of pressing Enter without browser throttling enabled. Getting SymPy's wheel from the jsDelivr CDN took 161.27 ms. Fetching SymPy from PyPI and not locally took 299.53 ms. By enabling throttling on a "Slow 4G" network, the local/hosted wheel took 583.83 ms to install, and the one from the CDN took 50.81 ms (not sure why, perhaps my request hit a cache – I wasn't able to do this experiment reliably). Fetching from PyPI on this throttled network took 45.17 ms, and I was unable to evict the cache for some JupyterLite niceties/annoyances. In all cases, One thing to note is that Pyodide's SymPy wheel/recipe has the GitHub Pages will use the Tip Hence, I can't offer a concrete answer for this, unfortunately, and I can't thoroughly verify if my experiment above was empirical enough to be insightful without having a hosted GitHub Pages site, but I would prefer this order (option B), and I hope it clears your concerns somewhat: The local wheel, as it requires no external bandwidth connection and can be cached by JupyterLite's storage on subsequent runs by a user >= jsDelivr CDN/Pyodide, which has a smaller wheel size + better compression > PyPI, which has a similar speed as that of jsDelivr and wheels can be cached by JupyterLite, but they are larger in size. However, the jsDelivr CDN might not come with the latest SymPy; hence, it's not a viable option at the moment. I just performed the experiment as it made sense to compare all options, especially when Pyodide will complete unvendoring recipes, and we could think about it again at a later time. Hence, choosing option B—where we always have a local and up-to-date wheel—shouldn't cause any adverse side effects right now, and we can ask @ivanistheone to run https://bit.ly/sympyjstest over it once we merge this. However, if this warrants a closer inspection, I would be open to deploying SymPy Live on GH Pages via my fork, and we could run those tests targeting my site instead of the upstream one. P.S. Since we are downloading SymPy from PyPI anyway, I could undoubtedly port the test unvendoring to a new reusable tool, but I don't think doing so is worth the effort as JupyterLite and |
I don't follow all the details so this is my main takeaway:
I just wanted to know if there was some reason why the previous code attempted to use pyodide first before then using PyPI. It sounds like there isn't any important reason to do that rather than just always install from PyPI. The main thing from my perspective is just that I want this to be always the latest version because e.g. we get bug reports from people who are not using the latest version and they do often test against the live website. The next thing from my perspective is that it would be great if we had a latest dev version that people could test e.g. to see if some issue is fixed already or try out new features. It would also be good if the interface makes it clear what version is being used because I think many users will not think to print |
Yes, I did write a lot here, and that's the main takeaway, indeed. FWIW, it's going to serve as a self-note for me in the future :)
Ah, that comes from the long-standing issue in Pyodide: pyodide/pyodide#2580, where removing SymPy and other pure Python packages was previously discussed. The gist is that it could be a bad idea to remove them, as:
Actually, I noticed when typing this comment that the code we have for unvendoring the tests is not too complicated: https://github.com/pyodide/pyodide-build/blob/7ebf9a274de346477910ae9b6885a5aed0b98fff/pyodide_build/buildpkg.py#L690-L741. This opens two options for an alternative measure, where the benefit is that we have a reduced wheel size to ship:
We would have to add the Otherwise, I think installing from PyPI could be tried out too, as the wheel caching is quite reliable based on my experiments.
Yes, that makes sense to me – this PR should be in line with those thoughts – barring the delay in which a new SymPy version is released and the nightly deployment hasn't been built yet. A cron job has a maximum granularity of five minutes, but that's overkill and someone will need to do a manual trigger of the deployment workflow.
That is a good idea, and it makes sense that we will have the necessary machinery for that after this PR – all we will need for a "dev" version of the shell is to
I agree. I've opened #23 for this recently. I haven't looked at a solution so far, as it might require writing a kernel of our own based on the design for |
Yes, I mean all these ideas sound good to me. I am somewhat out of my depth in understanding the details. In general I would actually like to just remove the tests from the installed sympy package and have them be a separate package but that's another bunch of work.
That is not an issue. If the live website is updated even weekly then that is fine. Anything faster than yearly beats the current situation.
I'm not sure what can or can't be done. In my mind the ideal thing would be something like a drop down box where you select the sympy version and maybe which other packages like python-flint you want. I have no idea whether that is an easy thing or something that is much harder than it sounds though. |
No worries; thanks for your input! I'll add these changes. Yes, moving tests away into a package of their own is a tricky scenario. In Meson land, this can be done using Install tags, which one can use to separate what files end up being included, and build two wheels with one command:
Sounds good. In that case, I think we should match the release schedule of https://github.com/sympy/sympy/blob/master/.github/workflows/nightly-wheels.yml, so daily sounds like a good bet as per my previous commit. A weekly build would also be fine with me.
That was what I had in mind, too, but I self-rejected the idea :D So, I spent a bit of time thinking about it, and I found that we can do this with the same JupyterLite distribution itself – multiple wheels can be added to the index: ![]() So it should be possible, but we need to add a file |
This is ready for review now; thank you @oscarbenjamin, for continually pinging me with questions about all of this in general and for responding to mine! I added a script to remove all test-related files from the wheel, and it reduces the SymPy 1.13.3 wheel size by 33.4%, from 6.2 MiB to 4.13 MiB (while retaining the compression level from PyPI). TODO items for me after this is merged:
|
Thanks @agriyakhetarpal for all your work on this. I'll didn't have time to follow all the discussions today, but I'll reserve some time for to look at the PR tomorrow afternoon/evening. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read in details and it looks good. Ready to ship!
Shall I press the green button or do you want to do it?
Thanks for the review, @ivanistheone! Unfortunately (or rather fortunately), I think I've discovered an upstream bug here as a part of testing my changes building up on this branch for a "dev' REPL – if I were to install the dev version here through Tap to show error message
This means that once SymPy 1.14 is released sometime later this year and The other thing to note here as to why this occurs is because statements like |
@agriyakhetarpal Can you try running the code form 26efec5 with I added the |
Ah, interesting, TIL, and thank you so much, @ivanistheone! That works quite well and I confirmed with I don't know if this behaviour that I noticed is a bug then, as it thus has an easy workaround. Either way, we should be good to merge this in this case. I've just added ec640fa to bump to https://github.com/jupyterlite/pyodide-kernel/releases/tag/v0.5.2, and will press the button. |
Also, I think I should rewrite history a bit here, as I believe squashing isn't too helpful because I've also added a few files. Could you please post some benchmarks from https://bit.ly/sympyjstest after this? It would be helpful to see that this doesn't cause significant downsides, as there is indeed a lack of CDN compression when downloading SymPy. |
ec640fa
to
15c149b
Compare
I did a quick test from my old phone (iOS), the loading and code execution is pretty fast. Like so fast it would be difficult to do exact timing since it's less than 3 seconds... I can do more tests from friend's Android phones later, but overall it's looking good. |
Meson (and meson-python) has no problems with being used to build pure-python packages. :) |
Sounds good, thanks for the information @eli-schwartz! I am willing to do such work for SymPy, though it would be on the discretion of @oscarbenjamin and @ivanistheone :) |
Description
This PR adds machinery to the shell's configuration to ensure that the SymPy version is always up to date. This change would become redundant with Pyodide 0.28 where package recipes will be unvendored from its runtime and multiple versions of a package will be available to install for a particular Pyodide version, but that release and the subsequent stability of that system are quite a few months away for us as of now – and hence this change should stay relevant till that time.
Changes made
jupyter_lite_config.json
file for this, as the CLI currently doesn't work (Ship additional Pyodide wheels at build time
does not work jupyterlite/jupyterlite#1502).unvendor_tests_from_wheel.py
has been added, based onpyodide-build
.index.html
page generation script has been updated to automatically add the%pip install sympy
command to the REPL initialisation code, so that SymPy is always up to date in the REPL.requirements.txt
, which is used by JupyterLite to manage its environmentblack
over the Python files)generateindex.py
togenerate_index.py
custom_wheels/
directory that is to be kept in version controlPlan of action
The version of SymPy in Pyodide 0.27.1 is 1.13.3 at the time of writing. When SymPy 1.x.y (where either x > 13 or y > 3 or both) gets released upstream, we will use
pip download
to place the wheel incustom_wheels/
, and rununvendor_tests_from_wheel.py
to remove all test-related files from the wheel (like how it is in the Pyodide distribution). Then,generate_index.py
will notice that a wheel exists in that directory, and it will embed the%pip install sympy
command in the REPL initialisation code. This will ensure that the SymPy version is always up to date.Additional context
%pip install sympy
does not work, but%pip install sympy==<sympy_new_version
does. I've filed an upstream issue here: Installing newer versions of packages that are included in the Pyodide distribution already jupyterlite/jupyterlite#1557pip
-installing packages in that case. I am happy to switch if warranted, as the feature for corresponding functionality in the Pyodide kernel has not had progress yet: Configure the kernel to have packages automatically installed jupyterlite/pyodide-kernel#60.%pip install
command could be misleading for users. However, I think it is fine for the REPL to include, as theimport sympy
command, too, runspiplite
underneath to install it before it can be imported, and JupyterLite-specific code is included in other contemporary projects using JupyterLite, such asscikit-learn
's notebooks.mpmath
are both pure Python packages helps a lot with this workflow here, as we don't need to build WASM wheels for them. However, for the inclusion of other packages with compiled extensions that SymPy uses via its optional dependencies, such aspython-flint
or Matplotlib (that have been proposed in Display the SymPy version in the shell prompt #23) – we'll have to think about it, as it means that we have to build them out-of-tree (xref tracking issue forpython-flint
: Nightly wheels and WASM/pyodide builds flintlib/python-flint#234 and issues/PRs for Matplotlib: [ENH]: out-of-tree Pyodide builds in CI for Matplotlib matplotlib/matplotlib#27870, Add wasm CI matplotlib/matplotlib#29093). The requirement depends on whether SymPy upstream moves to a version for them that is higher than what is available in Pyodide.