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

Upgrade to Pyodide 0.23.4 (with Python 3.11) and compile dependencies #603

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

alexmojaki
Copy link
Contributor

No description provided.

@paulfitz
Copy link
Member

(@alexmojaki can you rebase this when you have a moment)

@paulfitz
Copy link
Member

From sandbox/pyodide, I ran build_packages.sh and it ran to completion, building everything needed 👍. I then tried running GRIST_SANDBOX_FLAVOR=pyodide yarn start and opening a document, but I got this error:

2023-07-31 18:03:50.301 - info: Sandbox stderr: [pyodide sandbox] [package] Loaded et_xmlfile, executing, asttokens, iso8601, lazy_object_proxy, chardet, pure_eval, python_dateutil, astroid, sortedcontainers, six, unittest_xml_reporting, stack_data, roman, wrapt, typing_extensions, openpyxl, friendly_traceback, phonenumberslite sandboxPid=692865, flavor=pyodide, command=undefined, entryPoint=(default), docId=smStzDu4B6G5AT4CmVxdua
2023-07-31 18:03:50.394 - info: Sandbox stderr: [pyodide error] PythonError: Traceback (most recent call last):
  File "/lib/python3.10/_pyodide/_base.py", line 460, in eval_code
    .run(globals, locals)
  File "/lib/python3.10/_pyodide/_base.py", line 306, in run
    coroutine = eval(self.code, globals, locals)
  File "<exec>", line 5, in <module>
  File "/grist/grist.py", line 8, in <module>
    from usertypes import Any, Text, Blob, Int, Bool, Date, DateTime, \
  File "/grist/usertypes.py", line 21, in <module>
    import six
ImportError: bad magic number in 'six': b'\xa7\r\r

I saw a pyodide version number in setup.sh that looked out of step with the rest of the PR, updating that and replacing _build/worker seemed to do the trick.

For the unrelated test failure - I made a series of changes to test/nbrowser/SelectBySummary.ts that could be worth considering for test/nbrowser/SelectByRefList.ts - can't actually say if any of them change anything, since I can't duplicate the failure, so I'm just whittling away at possibilities. The SelectBy tests have been flakey since the order of test file evaluation became less deterministic.

I'd like to run make save_packages to save the painstakingly built output, but I wonder if there needs to be some kind of overall version number in the path? Are any build files that happen to have the same name currently usable by grist-static in its current state?

@paulfitz
Copy link
Member

paulfitz commented Aug 1, 2023

I'd like to run make save_packages to save the painstakingly built output, but I wonder if there needs to be some kind of overall version number in the path? Are any build files that happen to have the same name currently usable by grist-static in its current state?

Just for clarity on this - pyodide is the default python engine for grist-electron these days. As a desktop app grist-electron gets build on a matrix of platforms, so building the wheels for it repeatedly would be a significant slowdown. So it is best if the wheels are cached. See e.g.:
https://github.com/gristlabs/grist-electron/blob/ee6e73cfe8a8464d84538c826ab3edcaa03f3c71/scripts/setup.sh#L170-L176

I think when the pyodide engine in grist-core is updated, we should probably assign it a version and cache material in a distinct path. Then grist-electron and grist-static can get updated on their own schedule.

@alexmojaki
Copy link
Contributor Author

I didn't realise the pyodide sandbox flavor was actually still being used!

I think grouping the packages by pyodide version makes sense.

@paulfitz
Copy link
Member

paulfitz commented Aug 2, 2023

I didn't realise the pyodide sandbox flavor was actually still being used!

It is a relatively new thing. I cobbled together the pyodide sandbox in order to have some kind of sandboxing option for Windows users of the desktop app. Then realized that it came close to making something like grist-static possible, so I started hacking on that. None of this was really planned, it was a rogue project.

I think grouping the packages by pyodide version makes sense.

Who should make that happen? Do you plan to do it in this PR? This PR works well for me now, the one remaining issue is the package layout. Currently if you follow the README in sandbox/pyodide, you'll end up getting packages for a different version from https://s3.amazonaws.com/grist-pynbox/pyodide/packages/

@paulfitz
Copy link
Member

paulfitz commented Aug 2, 2023

(The sporadically failing TypeChange.ntest test should be fixed on main now)

@alexmojaki
Copy link
Contributor Author

Rather than the pyodide version, let's just use a custom version number for this purpose to also account for things like changes in requirements. So e.g. we could start with s3://grist-pynbox/pyodide/packages/v2/ and then later change that to v3 etc.

Let's try to avoid accidentally deleting/replacing uploaded files if we forget to update the version number. Is there an easy alternative to aws s3 sync that fails if there's already files in that 'directory'? Otherwise, a low tech solution could be to remove save_packages from Makefile and explain what to do in the README so that people are more likely to do it carefully.

If you could take over from here I'd really appreciate that, bit overwhelmed atm

Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Let's land this as is, I'll follow up quickly with something that adds versioning. Thanks @alexmojaki.

@alexmojaki alexmojaki merged commit d72f177 into main Aug 2, 2023
12 of 13 checks passed
@paulfitz
Copy link
Member

paulfitz commented Aug 2, 2023

Rather than the pyodide version, let's just use a custom version number for this purpose to also account for things like changes in requirements. So e.g. we could start with s3://grist-pynbox/pyodide/packages/v2/ and then later change that to v3 etc.

Sounds good.

Let's try to avoid accidentally deleting/replacing uploaded files if we forget to update the version number. Is there an easy alternative to aws s3 sync that fails if there's already files in that 'directory'? Otherwise, a low tech solution could be to remove save_packages from Makefile and explain what to do in the README so that people are more likely to do it carefully.

There may be a flag, I can check. The set of people who can upload to grist-pynbox is very low though.

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.

2 participants