-
Notifications
You must be signed in to change notification settings - Fork 30
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
Allow to install extra packages from extra indexes from jupyter-lite.json #158
base: main
Are you sure you want to change the base?
Conversation
@@ -394,5 +395,6 @@ export namespace PyodideKernel { | |||
* The Jupyterlite content manager | |||
*/ | |||
contentsManager: Contents.IManager; | |||
extraPackagesAndIndexes: Array<{ package: string; indexes: string[] | null }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make that Array<{ package: string[]; ... }>;
/** | ||
* List of extra url/wheel paths to load, and extra associated indexes urls (if not a wheel url) | ||
*/ | ||
extraPackagesAndIndexes: Array<{ package: string; indexes: string[] | null }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
} | ||
} else { | ||
console.info('no extra packages and indexes'); | ||
} | ||
|
||
// get piplite early enough to impact pyodide-kernel dependencies | ||
await this._pyodide.runPythonAsync(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lower in initKernel the
for (const pkgName of toLoad) {
if (!preloaded.includes(pkgName)) {
scriptLines.push(`await piplite.install('${pkgName}', keep_going=True)`);
}
}
Could be modified to be a single call to piplite.install(list)
, instead of repeated install calls.
|
if (indexes === null) { | ||
installCmd = `import micropip\nawait micropip.install('${pkg}', keep_going=True)`; | ||
} else { | ||
installCmd = `import micropip\nawait micropip.install('${pkg}', index_urls=${JSON.stringify(indexes)}, keep_going=True)`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both cases, if pkg ends with whl, prepend baseurl,
and from testing, check the index urls do not end in /
, otherwise it redirects and there are CORS issues.
and cc @gabalafou |
Hmmmm, while I take no issue with the indexes (indeed, maybe a As of So my feeling would be: we could add hooks to allow another extension to load extra packages, run dynamic startup code, configure environment variables, write files on disk, make telemetry posts, etc. and all the other things that folk have asked for, but well after the core plumbing has loaded so that when errors happen, those messages appear, with attribution, somewhere. |
Sure that would be fine with me, what Im really hopping is to allow most of the Python Ecosystem to be able to get interactive documentation without having to rebuild a pyodide distribution. I'm fine if extra dependencies and breakage are up to the maintainers' using this configuration option. I'm also happy to move this to another place in the launching process. |
crosslinking to #60 |
Would you agree if this were change to only look at locally available wheels ? Basically when using For me including wheels in a jupyterlite build should make them immediately available to the end user, or it feels weird. I'm ok if wheels needs to be referred to in two locations. |
I would agree with this, as this is one of the points that I've made in sympy/live#26 and it would be nicer to see that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As promised, I gave this a look and it looks good to me.
It seems that your comment about getting TypeScript to work was based on a previous commit and that you've resolved the issues with the TypeScript compiler, is that correct?
…json<F48> This is in progress, but the idea is to be able to point to dev wheels on rtd or similar without having to make a custom pyidide distribution. For example: ``` // jupyter-lite.json { "jupyter-config-data": { "litePluginSettings": { "extraPackagesAndIndexes": [ { "indexes": [ "https://cors.carreau.workers.dev/scientific-python-nightly-wheels/simple", "https://pypi.org/simple" ], "package": "xarray" } ] } } } ``` Package could also be a simple url to a whl (I need to check how it works if it's a local URL), and indexes can be ommited. WIP as I'm stuggling to have the typescript build process work.
Co-authored-by: gabalafou <[email protected]>
It seem to be that this asking way more than just having packages ready to use. Right now usually all the file and setup are ready, the only step missing is having pre-installed package. I understand the need to not wanting to download and extra URL, but there is already the --piplite-wheels so we only need to make them available at import. Now if you really want a different extension fine, we can do tham but assuming I provide the necessary patches for |
There is a big difference between blocking the kernel being available, with proper error reporting, on:
There was a previously-rejected PR (don't see it here, maybe was before the SplitLite) that did the scut work to make that happen (discovering top-level names, etc.), by patching the as-loaded For those reasons, I've been pursuing upstream means to make this more predictable, such as:
...and downstream means to make this work in Maybe once all of that works, every time, without npm/nodejs or a full browser at build time, we can consider making something that isn't too bad in this repo. |
This is in progress, but the idea is to be able to point to dev wheels on rtd or similar without having to make a custom pyidide distribution.
For example:
Package could also be a simple url to a whl (I need to check how it works if it's a local URL), and indexes can be ommited.
WIP as I'm stuggling to have the typescript build process work.