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

merging xeus-python-kernel into this repo #596

Closed
DerThorsten opened this issue Nov 7, 2023 · 8 comments
Closed

merging xeus-python-kernel into this repo #596

DerThorsten opened this issue Nov 7, 2023 · 8 comments

Comments

@DerThorsten
Copy link
Member

DerThorsten commented Nov 7, 2023

Atm we have two repositories for the xeus-python project:

1 https://github.com/jupyter-xeus/xeus-python
2 https://github.com/jupyterlite/xeus-python-kernel

That leads to the following problems:
To test a single change in the xeus-python project, we need to jump through a lot of hoops.

  • create a new tag / version in the xeus-python project.
  • release the new tag / version on emscripten-forge (or do a local build of an emscripten-forge package)
  • update the xeus-python version in the xeus-python-kernel project to use the new version.
  • build the xeus-python-kernel project.
  • see if the changes work

The testing/ci on the xeus-python project is non-existent (wrt wasm). While on the xeus-python-kernel project, we
have the mechanism to run ui tests for the xeus-python-kernel on each commit.

Therefore it would be beneficial to merge the xeus-python and xeus-python-kernel projects into a single project.
The benefits are:

  • we can test the xeus-python project on each commit
  • we can iterate faster on the xeus-python project.
  • more robust

The downsides:

  • The repo will be bigger.

I believe the benefits outweigh the downsides massively.

Furthermore, In the long-run I think this merging of xeus-<lang> and xeus-<lang>-kernel projects should be done for all
xeus-<lang> projects. This also allows to have a single cookiecutter / template repo for new xeus-<lang> projects.
ATM I beieve no "outsider" can create a new xeus-<lang> lite kernel, because the process is too complicated.

Therefore, I suggest that we create a folder lite in the xeus-python project, where we put the xeus-python-kernel project.
We need to adapt the npm build system to build from source. We maybe want to keep the ability to build the lite kernel from an emscripten-forge pre-built xeus-python package.

Any suggestions on that? @martinRenou @JohanMabille @SylvainCorlay @jtpio

@martinRenou
Copy link
Member

Thank for starting this discussion @DerThorsten. I would be in favor of this 👍🏽 💯 Especially that it would make the jupyterlite-xeus-python kernel more robust by catching issues earlier.

@JohanMabille
Copy link
Member

I'm in favor of this merge too, with the following remarks:

  • the generation of the lite repo should be optional in the cookiecutter, since not all kernel have their dependencies available for WASM yet (I know this sounds obvious, but better to say it than to forget it)
  • The idea behing having two separate repos was that xeus-python can be used as a building block for downstream projects (including the wasm kernel). If we merge the two repos, I don't think we should maintain a pre-built xeus-python package on emscripten-forge. Or that means that we need to "split" the package on conda-forge too, to have a libxeus-python and a xeus-python kernel packages.
  • A previous idea was to have a cookiecutter for all the lite kernels, to avoid code duplication (e.g. xeus-python index.ts and xeus-lua index.ts which are identical except for the kernel spec, or the declaration files which are the same). On the long run it would be nice to factorize this code somewhere.

@DerThorsten
Copy link
Member Author

DerThorsten commented Nov 7, 2023

I'm in favor of this merge too, with the following remarks:

  • the generation of the lite repo should be optional in the cookiecutter, since not all kernel have their dependencies available for WASM yet (I know this sounds obvious, but better to say it than to forget it)

Yes, I believe we already have a flag to add / not add wasm-support in the cookiecutter

  • The idea behing having two separate repos was that xeus-python can be used as a building block for downstream projects (including the wasm kernel). If we merge the two repos, I don't think we should maintain a pre-built xeus-python package on emscripten-forge.

I would be fine with that! No need to maintain packages on emscripten forge just for the fun of it.
But in theory, there could be another kernel depending on libxeuspython.

Or that means that we need to "split" the package on conda-forge too, to have a libxeus-python and a xeus-python kernel packages.

The plain conda package (not the emscripten-forge one) should not care at all about this merging whatsoever.
So I do not see a need for any action on conda-forge.

  • A previous idea was to have a cookiecutter for all the lite kernels, to avoid code duplication (e.g. xeus-python index.ts and xeus-lua index.ts which are identical except for the kernel spec, or the declaration files which are the same). On the long run it would be nice to factorize this code somewhere.

I am against more templating. I would rather have nice APIs/ Baseclasses / Common js/ts functions.
On the typescript side, we should have smth. like a XeusLiteWorkerKernelBase. On the emscripten-cpp side, we can have a more generic initialization by implementing a pure js async_initialize function in each kernel post.js file.
Requiring templates to get it nice is a code-smell.
But I think this should be the second step.

The first step would be to merge the repos for all lite kernels.

@JohanMabille
Copy link
Member

The plain conda package (not the emscripten-forge one) should not care at all about this merging whatsoever.
So I do not see a need for any action on conda-forge.

It's more a matter of consistency: either libxeus-python can be used as a building block for other kernels, and we should split the package on both conda-forge and emscripten-forge, or it's not, and we shoud have kernel packages only on both platforms. (And should we package xeus-python for another platform / package manager, we should follow the same policy).

I am against more templating. I would rather have nice APIs/ Baseclasses / Common js/ts functions.

Totally agree with you, my point was to factorize out the code only. I'm happier with clean architecture rather than templating and code generation.

@jtpio
Copy link
Member

jtpio commented Nov 13, 2023

How do you envision making releases from this repo, now that #599 hsa been merged? Ideally we should still be able to to use the releaser instead of making manual releases.

#599 still needs a few iterations to be fully complete (see comments in #599). Should we keep track of them here?

What about https://github.com/jupyterlite/xeus-python-kernel? Should we start moving issues here and plan to archive https://github.com/jupyterlite/xeus-python-kernel soon?

@martinRenou
Copy link
Member

Ideally we should still be able to to use the releaser instead of making manual releases.

Yes it'd be great to continue using the releaser. We discussed internally that making the 1.0 release here publishing both xeus-python and jupyterlite-xeus-python 1.0 from there would be the way to go.

#599 still needs a few iterations to be fully complete (see comments in #599). Should we keep track of them here?

Yes 👍🏽 The documentation has been merged in #601 though it still needs iterations on #600 so that the readthedocs build uses the main branch for building xeus-python.

What about https://github.com/jupyterlite/xeus-python-kernel? Should we start moving issues here and plan to archive https://github.com/jupyterlite/xeus-python-kernel soon?

Yes and yes :)

@jtpio
Copy link
Member

jtpio commented Nov 13, 2023

We discussed internally that making the 1.0 release here publishing both xeus-python and jupyterlite-xeus-python 1.0 from there would be the way to go.

Fine for going with 1.0 and versioning the two packages together if that can simplify the setup 👍

As an alternative, we could also still use the releaser for making the release, but manually create PRs to bump versions, a bit like with Lumino: https://github.com/jupyterlab/lumino. Although this could create confusion as to which version to use in the GitHub release if we don't want a date based version like 2023.11.05.

@DerThorsten
Copy link
Member Author

jupyter-xeus/xeus-lite#7 seems to be the better approach

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

No branches or pull requests

4 participants