jupyterlite / xeus

JupyterLite loader for Xeus kernels
https://jupyterlite-xeus.readthedocs.io
BSD 3-Clause "New" or "Revised" License
22 stars 11 forks source link

add files from jupyterlite-xeus #4

Closed DerThorsten closed 9 months ago

DerThorsten commented 9 months ago

this is building ontop of the PR #2

DerThorsten commented 9 months ago

in the working repo https://github.com/jupyterlite/xeus/pull/2 we have some special step for the webpack / worker https://github.com/DerThorsten/jupyterlite-xeus/blob/main/package.json#L34 and some special webpack config https://github.com/DerThorsten/jupyterlite-xeus/blob/main/worker.webpack.config.js

This is not yet present in this repo and gives strange issues when trying to start the kernel (compiled without webpack modification to see whats going on)

213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:224 Uncaught (in promise) TypeError: __webpack_modules__[moduleId] is not a function
    at __webpack_require__ (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:224:41)
    at 213 (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:8:65)
    at __webpack_require__ (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:224:41)
    at 213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:239:85
    at __webpack_require__.O (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:269:23)
    at __webpack_require__.x (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:240:54)

we get a lot of webpack code appended to the worker.js which is causing the error above

************************************************************************/
/******/    // The module cache
/******/    var __webpack_module_cache__ = {};
/******/    
/******/    // The require function
/******/    function __webpack_require__(moduleId) {
/******/        // Check if module is in cache
/******/        var cachedModule = __webpack_module_cache__[moduleId];
/******/        if (cachedModule !== undefined) {
/******/            return cachedModule.exports;
/******/        }
/******/        // Create a new module (and put it into the cache)
/******/        var module = __webpack_module_cache__[moduleId] = {
/******/            // no module.id needed
/******/            // no module.loaded needed
/******/            exports: {}
/******/        };
/******/    
/******/        // Execute the module function
/******/        __webpack_modules__[moduleId](module, module.exports, __webpack_require__);
/******/    
/******/        // Return the exports of the module
/******/        return module.exports;
/******/    }
/******/    
/******/    // expose the modules object (__webpack_modules__)
/******/    __webpack_require__.m = __webpack_modules__;
/******/    
/******/    // expose the module cache
/******/    __webpack_require__.c = __webpack_module_cache__;
/******/    
/******/    // the startup function
/******/    __webpack_require__.x = () => {
/******/        // Load entry module and return exports
/******/        var __webpack_exports__ = __webpack_require__.O(undefined, [424], () => (__webpack_require__(213)))
/******/        __webpack_exports__ = __webpack_require__.O(__webpack_exports__);
/******/        return __webpack_exports__;
/******/    };
/******/    
/************************************************************************/
DerThorsten commented 9 months ago

To try to debug this you need to build jupyterlite like this, where is the dir of this repo

jupyter lite build --XeusAddon.environment_file=<REPO_DIR>/env.yaml
DerThorsten commented 9 months ago

@trungleduc still the same :/

trungleduc commented 9 months ago

@trungleduc still the same :/

if you use the worker.ts file. You need to add the typescript loader. For example here is what we did https://github.com/jupytercad/jupytercad/blob/v0.3.3/packages/jupytercad-extension/worker.webpack.config.js

trungleduc commented 9 months ago

Or you can try the second solution renaming the output of Webpack to another name https://github.com/jupyterlite/xeus/pull/4#discussion_r1422581031

DerThorsten commented 9 months ago

So just to clarify. The webworker code is picked up by webpack and processed. But when we enter the webworker code it fails inside the worker with that (ie it fails at runtime once I open the kernel)

213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:224 Uncaught (in promise) TypeError: __webpack_modules__[moduleId] is not a function
    at __webpack_require__ (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:224:41)
    at 213 (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:8:65)
    at __webpack_require__ (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:224:41)
    at 213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:239:85
    at __webpack_require__.O (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:269:23)
    at __webpack_require__.x (213.c1afc05d321777c556aa.js?v=c1afc05d321777c556aa:240:54)

I assume this is when it tries to load the dependencies we use inside the worker, ie that stuff

import { expose } from 'comlink';

import {
  DriveFS,
  DriveFSEmscriptenNodeOps,
  IEmscriptenFSNode,
  IStats
} from '@jupyterlite/contents';
trungleduc commented 9 months ago

your build:worker step is not called anywhere? I should be a part of the build process.

DerThorsten commented 9 months ago

your build:worker step is not called anywhere? I should be a part of the build process.

I played around with that as we used to have a dedicated step for this in the other repos. But the reason to have this as a separate step was that worker.ts did not used to be part of the repository. So I hope that we can drop that again.

DerThorsten commented 9 months ago

@trungleduc ah ok, so I start to understand. The worker.webpack.config.js is not magically picked up. And If I don't call the manual webpack step for the worker it does not make sense at all to have this worker.webpack.config.js?

trungleduc commented 9 months ago

No, you can not drop that, you want to build worker.ts with webpack separately because we need to bundle all its dependencies into the worker.js file so that you won't have the import errors on run time.

DerThorsten commented 9 months ago

No, you can not drop that, you want to build worker.ts with webpack separately because we need to bundle all its dependencies into the worker.js file so that you won't have the import errors on run time.

Ok, now things start to make sense again.

(assume maximum stupidity here...I don't know what I am doing here most the time ;) )

DerThorsten commented 9 months ago

@jtpio we could merge this now. (maybe not in main?) as it is in a working state again. Still needs a lot of work to be production ready

jtpio commented 9 months ago

@jtpio we could merge this now. (maybe not in main?) as it is in a working state again. Still needs a lot of work to be production ready

Sure, if it helps iterating on it. Just left a few minor inline comments.

Also wondering if we could improve the initial blocking HTTP request for fetching the json file. Network (or the static HTTP server) can be slow, and this could increase the time it takes for the application to start.

DerThorsten commented 9 months ago

@jtpio we could merge this now. (maybe not in main?) as it is in a working state again. Still needs a lot of work to be production ready

Sure, if it helps iterating on it. Just left a few minor inline comments.

Also wondering if we could improve the initial blocking HTTP request for fetching the json file. Network (or the static HTTP server) can be slow, and this could increase the time it takes for the application to start.

I agree that its unsexy, but the json files are all so tiny that this will not be a big issue (the kernel.json for each kernel are ~200 bytes)

jtpio commented 9 months ago

Maybe there is a way to inline the kernels at jupyter lite build time? If the kernels are all known at jupyter lite build time.

DerThorsten commented 9 months ago

Maybe there is a way to inline the kernels at jupyter lite build time? If the kernels are all known at jupyter lite build time.

Yeah they are all known at jupyter lite build time. Anything else would mean downloading kernels on the fly with query parameters or smth like that -- but no, we do not do shenanigans like that so far.

jtpio commented 9 months ago

OK so I guess we can look into this separately.

And also fix the remaining CI failure in the next iteration.