jupyterlite / xeus-python-kernel

xeus-python in JupyterLite
https://xeus-python-kernel.readthedocs.io
BSD 3-Clause "New" or "Revised" License
30 stars 19 forks source link

Issue when using the `itables` package #126

Closed jtpio closed 1 year ago

jtpio commented 1 year ago

Description

The itables package allows for making pandas dataframes more interactive: https://github.com/mwouts/itables

It seems to be working fine with the Pyodide kernel:

image

However xeus python has issues importing the package with import itables:

FileNotFoundError: [Errno 44] No such file or directory: '/tmp/xeus-python-kernel/envs/jupyterlite-tutorial-demo/lib/python3.10/site-packages/itables/html/itables.css'

image

While checking the directory content looks fine with:

import os
os.listdir()

Reproduce

Create a new environment with itables as a dependency.

Expected behavior

itables should work in the xeus python kernel.

At first it looked like it could have been an issue with the service worker not able to find the files.

But digging into this more it seems that the html folder is missing:

image

Context

This might be similar to https://github.com/jupyterlite/xeus-python-kernel/issues/120

jtpio commented 1 year ago

Or maybe it's an issue with empack filtering the html folder?

martinRenou commented 1 year ago

Or maybe it's an issue with empack filtering the html folder?

I'm almost certain this is it. We may want to add an entry for itables in https://github.com/emscripten-forge/empack/blob/main/config/empack_config.yaml

jtpio commented 1 year ago

Ah this is where the config file is located, nice.

Not sure this would still be an issue with the new empack==3.0.0? Maybe this was changed in the new version? cc @DerThorsten

jtpio commented 1 year ago

I'm almost certain this is it. We may want to add an entry for itables in https://github.com/emscripten-forge/empack/blob/main/config/empack_config.yaml

In the meantime happy to try that and see if it fixes the issue.

jtpio commented 1 year ago

Trying the fix from https://github.com/emscripten-forge/empack/pull/68 by modifying the share/empack/empack_config.yaml directly seems to do the trick:

image

martinRenou commented 1 year ago

Note that it's also possible to provide your own config through a jupyterlite-xeus-python addon option.

jtpio commented 1 year ago

Note that it's also possible to provide your own config through a jupyterlite-xeus-python addon option.

Ah indeed, looks like it's defined here:

https://github.com/jupyterlite/xeus-python-kernel/blob/c7e7c48c5c1092ba46494e4457638e78c306f716/jupyterlite_xeus_python/env_build_addon.py#L48-L53

It would be great to add that to the documentation so it's more visible: https://xeus-python-kernel.readthedocs.io/en/latest/configuration.html

It could be handy for fixing packages quickly without requiring a new empack release.

martinRenou commented 1 year ago

I haven't documented it yet because I've not tested this feature so much yet :) But a bit of testing and documentation would be good indeed!

jtpio commented 1 year ago

Right, for example does it handle merging the user provided config with the default empack config?

martinRenou commented 1 year ago

I am not entirely sure, but that would be great indeed.

DerThorsten commented 1 year ago

Right, for example does it handle merging the user provided config with the default empack config?

nope, because I think its ambiguous what that means

edit: maybe we could just always "let the user config win" in case we have entries for one pkg in both configs

jtpio commented 1 year ago

edit: maybe we could just always "let the user config win" in case we have entries for a pkg in both

Sounds good. The docs could also link to the default config used in empack for reference.

In practice we probably don't expect users to make heavy use of it anyway?

jtpio commented 1 year ago

In practice we probably don't expect users to make heavy use of it anyway?

So for example this could be documented as an advanced configuration option.

DerThorsten commented 1 year ago

In practice we probably don't expect users to make heavy use of it anyway?

Correct: The actual reason we made it configurable is, that there might be a non-python wasm ecosystem, like a hypothetical c++-wasm which would need to include a different set of files for the same package. For instance for such a c++ wasm thingy we would like to include all *.h files

DerThorsten commented 1 year ago

@jtpio / @martinRenou actually I already implemented the merging =) https://github.com/emscripten-forge/empack/blob/4b388b67035ee3fe47f9efad2c5cc57fe8593116/empack/file_patterns.py#L99

jtpio commented 1 year ago

Ah nice, thanks for providing the extra context :+1: