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 files from jupyterlite-xeus #4

Merged
merged 23 commits into from
Dec 12, 2023

Conversation

DerThorsten
Copy link
Collaborator

this is building ontop of the PR #2

@DerThorsten
Copy link
Collaborator Author

DerThorsten commented Dec 11, 2023

in the working repo #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
Copy link
Collaborator Author

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

jupyter lite build --XeusAddon.environment_file=<REPO_DIR>/env.yaml

package.json Outdated Show resolved Hide resolved
@DerThorsten
Copy link
Collaborator Author

@trungleduc still the same :/

@trungleduc
Copy link
Collaborator

@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
Copy link
Collaborator

Or you can try the second solution renaming the output of Webpack to another name #4 (comment)

@DerThorsten
Copy link
Collaborator Author

DerThorsten commented Dec 12, 2023

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
Copy link
Collaborator

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

@DerThorsten
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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

env.yaml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jtpio
Copy link
Member

jtpio commented Dec 12, 2023

@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.

Co-authored-by: Jeremy Tuloup <[email protected]>
@DerThorsten
Copy link
Collaborator Author

DerThorsten commented Dec 12, 2023

@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
Copy link
Member

jtpio commented Dec 12, 2023

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
Copy link
Collaborator Author

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 jtpio added the enhancement New feature or request label Dec 12, 2023
@jtpio
Copy link
Member

jtpio commented Dec 12, 2023

OK so I guess we can look into this separately.

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

@DerThorsten DerThorsten merged commit 8790d87 into jupyterlite:main Dec 12, 2023
5 of 6 checks passed
@DerThorsten DerThorsten deleted the xeus-loader branch December 12, 2023 13:54
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants