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

Add vega extension, basic support for altair #113

Merged
merged 9 commits into from May 31, 2021
Merged

Add vega extension, basic support for altair #113

merged 9 commits into from May 31, 2021

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented May 30, 2021

References

Incremental improvement for #110

Code changes

Add the vega5-extension to the main application bundle like in JupyterLab

TODO

  • Add @jupyterlab-vega5-extension
  • Add example vega file to the demo site examples
  • Add to retro
  • Add altair example notebook

User-facing changes

image

Basic support for altair:

altair-example.mp4

Backwards-incompatible changes

None

@bollwyvl
Copy link
Collaborator

Awesome, vega is a great first step for viz stuff.

While writing vega by hand isn't super fun, would it be worth having the magic MIME string(s) encoded within Vega and/or VegaLite display types in pyolite?

@jtpio
Copy link
Member Author

jtpio commented May 30, 2021

While writing vega by hand isn't super fun, would it be worth having the magic MIME string(s) encoded within Vega and/or VegaLite display types in pyolite?

I think so yes. Ideally we can get Altair to work without too many patches.

@jtpio
Copy link
Member Author

jtpio commented May 30, 2021

Example using @lrowe code snippet from: https://github.com/jtpio/jupyterlite/issues/110#issuecomment-850916083 and snippets from the altair examples:

altair.mp4

@jtpio jtpio added this to In progress in MVP via automation May 30, 2021
@jtpio jtpio marked this pull request as ready for review May 30, 2021 19:01
@jtpio jtpio changed the title Add vega extension Add vega extension, basic support for altair May 30, 2021
@jtpio
Copy link
Member Author

jtpio commented May 30, 2021

Users still need the bootstrap code mentioned in https://github.com/jtpio/jupyterlite/issues/110#issuecomment-850916083:

# code snippet below from @lrowe: https://github.com/jtpio/jupyterlite/issues/110#issuecomment-850916083
import micropip
# Work around https://github.com/pyodide/pyodide/issues/1614 which is now fixed in pyodide
await micropip.install('Jinja2')
micropip.PACKAGE_MANAGER.builtin_packages['jinja2'] = micropip.PACKAGE_MANAGER.builtin_packages['Jinja2']
# Last version of jsonschema before it added the pyrsistent dependency (native code, no wheel)
await micropip.install("https://files.pythonhosted.org/packages/77/de/47e35a97b2b05c2fadbec67d44cfcdcd09b8086951b331d82de90d2912da/jsonschema-2.6.0-py2.py3-none-any.whl")
await micropip.install("altair")
import sys
sys.setrecursionlimit(255)

Maybe that's something that could be added as a "helper patch" in pyolite to make it more convenient for now.

@lrowe
Copy link

lrowe commented May 30, 2021

Maybe that's something that could be added as a "helper patch" in pyolite to make it more convenient for now.

Breaking down the steps of my bootstrap code:

# Work around https://github.com/pyodide/pyodide/issues/1614 which is now fixed in pyodide
await micropip.install('Jinja2')
micropip.PACKAGE_MANAGER.builtin_packages['jinja2'] = micropip.PACKAGE_MANAGER.builtin_packages['Jinja2']

Fix is being actively worked on here: pyodide/pyodide#1615

# Last version of jsonschema before it added the pyrsistent dependency (native code, no wheel)
await micropip.install("https://files.pythonhosted.org/packages/77/de/47e35a97b2b05c2fadbec67d44cfcdcd09b8086951b331d82de90d2912da/jsonschema-2.6.0-py2.py3-none-any.whl")

I believe the right way to fix this is to build pyrsistent as a pyodide package: https://pyodide.org/en/stable/development/new-packages.html

Was going to look at this but haven't managed to get pyodide to build on my ARM Mac.

import sys
sys.setrecursionlimit(255)

This is to avoid running into the recursion limit while rendering with altair. sys.getrecursionlimit() returns 127 in jupyterlite on Chrome for me which seems very low. At https://pyodide.org/en/stable/console.html it returns 220 on Chrome and 912 on Safari (jupyterlite doesn't seem to work properly on Safari for me) and 1000 on my regular python 3.9.5 interpreter.

Is the lower recursion limit set by jupyterlite or calculated somehow by pyodide? Seems reasonable to increase it a bit.

@bollwyvl
Copy link
Collaborator

Is the lower recursion limit set by jupyterlite or calculated somehow by pyodide?

The recursion limit stuff is derived from limitations of the WASM stack, as pyodide frames are waaaay heavier than JS (much less WASM) ones... it's a bit of a heuristic, derived from how the upstream works.

Last version of jsonschema before it added the pyrsistent dependency (native code, no wheel)

Yeah, this is going to be a killer in a bunch of places.

@jtpio
Copy link
Member Author

jtpio commented May 30, 2021

Thanks @lrowe for the extra context 👍

Is the lower recursion limit set by jupyterlite or calculated somehow by pyodide? Seems reasonable to increase it a bit.

Looks like pyodide has some logic to choose a value for the recursion limit here: https://github.com/pyodide/pyodide/blob/2e0475227255166269d910f260d138274c02d5fc/src/js/pyodide.js#L118-L143

JupyterLite doesn't explicitly set the limit. Although we could probably place the sys.setrecursionlimit(255) in https://github.com/jtpio/jupyterlite/blob/main/packages/pyolite-kernel/py/pyolite/pyolite/__init__.py.

@lrowe
Copy link

lrowe commented May 30, 2021

I just plucked the 255 figure out of the air. It seems that 170 is sufficient for the altair chart I tested. Anything much above that and I get a pyodide fatal error in the JS console (RangeError: Maximum call stack size exceeded) with the following code:

import sys; sys.setrecursionlimit(180) # <-- EDIT 180 for fatal error 
def test(i=0):
    print(i)
    test(i+1)
test()

It's uncomfortably close to the limit either side though, 160 and altair cannot render, 180 and I see pyodide fatal error. Guess the following might be okay for now:

import sys
sys.setrecursionlimit(max(170, sys.getrecursionlimit())

Note: The maximum stack size will likely vary by platform (and browser). My numbers are from Chrome 91 on ARM Mac.

@jtpio
Copy link
Member Author

jtpio commented May 31, 2021

Guess the following might be okay for now

Added as default in jtpio@6bdf4f1 for now.

@jtpio
Copy link
Member Author

jtpio commented May 31, 2021

Let's get this in then, we can always improve and iterate in follow-up PRs.

Thanks all!

@jtpio jtpio merged commit 720f5b9 into main May 31, 2021
MVP automation moved this from In progress to Done May 31, 2021
@jtpio jtpio deleted the vega branch May 31, 2021 12:05
@jtpio jtpio added the enhancement New feature or request label Jun 22, 2021
This was referenced Jul 12, 2021
@jtpio jtpio added this to the 0.1.0 milestone Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
MVP
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants