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

changes for jupyterlite-kernel #119

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

DerThorsten
Copy link
Member

@DerThorsten DerThorsten commented Feb 3, 2022

  • added new target for jupyterlite kernel
  • added FindSqlite3.cmake in dedicated cmake_emscripten folder:
    • this is needed since we use a non-custom installation of sqlite for the wasm build
    • cmake_emscripten is only included in wasm build
  • Added new magic commands for wasm build:
    • FETCH <URL> <FILENAME> : download a file / sqlite database and store it at the filename
    • SET_IDBFS_DIR <DIR>: set the directory which is "cached" / "persisted" to the IndexedDB
    • PUSH_TO_IDBFS : 'flush' content of persisted directory to IndexedDB
  • tiny bugfix in create since the index of the used token was off by one

changed find sqlite
@DerThorsten DerThorsten changed the title changes for sqlite-kernel changes for jupyterlite-kernel Feb 3, 2022
Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

curious to know why did you have to add a FindSQLite file. find_package(SQLite3 REQUIRED) didn't work?

@DerThorsten
Copy link
Member Author

@marimeireles the sql installation comes from the SQLiteCpp - installation.
This gives us the library and the headers, but is does not ship a proper cmake integration, this is why we need that here.
We could very easy write this FindSqlite thingy from scratch instead of using the copy pasted one since it only needs to work for the emscripten case and nothing else

@DerThorsten
Copy link
Member Author

but @marimeireles we can also wait a bit with mering this since we are currently changing xeus itself a bit

@marimeireles
Copy link
Member

but @marimeireles we can also wait a bit with mering this since we are currently changing xeus itself a bit

Not a problem for me, we can merge it now and then do a new PR once the changes upstream are in.

This gives us the library and the headers, but is does not ship a proper cmake integration,

I see. Cool. Was just wondering! :)
Thanks for your work here @DerThorsten.

@psychemedia
Copy link

psychemedia commented Feb 8, 2022

This looks really useful :-) But if the database is large it means the browser will also take quite a hit memory wise, and network / download time wise.

An alternative approach for accessing large SQLite databases from in-browser SQLite engines is described in Hosting SQLite databases on Github Pages. To quote that post, it uses "a virtual file system that fetches chunks of the database with HTTP Range requests when SQLite tries to read from the filesystem: sql.js-httpvfs."

@marimeireles
Copy link
Member

@psychemedia, thanks for the input :)

I'll merge this PR and fix the CI on a next one.

@psychemedia
Copy link

@DerThorsten Do you have an example of using the magics to download and query data from a sqlite db located at a particular URL?

If I just try to run the %FETCH magic using this repo's Binder link, I get an error:

Error: Load a database to run this command.

If I create a database, the %FETCH runs without delay (too short a time for it to be blocking-running as it waits for the file to download) and then when I try the %LOAD I get an Error: The path doesn't exist.

@DerThorsten
Copy link
Member Author

DerThorsten commented Feb 11, 2022

@psychemedia this feature only works in case of a wasm build (ie JupyterLite )
Also it is not yet integrated in the jupyterlite kernel repo

But should not be too complicated to make the changes in the kernel repo by changing the Dockerfile
https://github.com/jupyterlite/xeus-sqlite-kernel/blob/c5fc770610453f0af80b6043fd430d96f9453a0d/Dockerfile#L188

@psychemedia
Copy link

Ah, ok, thanks... I must have misread the other thread that referred to this one..

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.

3 participants