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

Spit the package into voici-core and voici #106

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Jan 18, 2024

This PR splits the repo into two packages:

  • voici-core: provides the CLI and the jupyterlite addon
  • voici: a meta-package that ships both voici-core and jupyterlite-xeus-python (will switch to using jupyterlite-xeus in a separate PR)

Copy link
Contributor

lite-badge 👈 Try it on ReadTheDocs

@martinRenou martinRenou added the enhancement New feature or request label Jan 18, 2024
pyproject.toml Show resolved Hide resolved
@martinRenou martinRenou force-pushed the big_split branch 11 times, most recently from 448e698 to 5a6c80f Compare January 19, 2024 16:05
@martinRenou
Copy link
Member Author

check-release is driving me nuts, using some system yarn for some reason

@jtpio
Copy link
Member

jtpio commented Jan 22, 2024

check-release is driving me nuts, using some system yarn for some reason

Were you able to find the reason a system-wide yarn would be picked up?

@martinRenou martinRenou marked this pull request as ready for review January 22, 2024 08:57
@trungleduc
Copy link
Member

check-release is driving me nuts, using some system yarn for some reason

Were you able to find the reason a system-wide yarn would be picked up?

Because jupyterlab/maintainer-tools/.github/actions/base-setup@v1 installs yarn 1 by default.

@@ -25,13 +25,6 @@ You can also deploy Voici as a standalone application so it can be hosted anywhe

First install Voici by following the [installation instructions](install.md).

You also need to install a kernel for the language you want to use. For example, to use Python, you can install the `jupyterlite-xeus-python` package.
Copy link
Member

@jtpio jtpio Jan 22, 2024

Choose a reason for hiding this comment

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

Should the docs mention the two voici-core and voici packages?

For reference the JupyterLite docs try to stick to mentioning jupyterlite-core because this is what provides the CLI: https://jupyterlite.readthedocs.io/en/stable/

Also the jupyterlite metapackage removed the dependency on jupyterlite-pyodide-kernel in jupyterlite/jupyterlite#1147.

If the voici metapackage depends on jupyterlite-xeus, it would be nice to explain why.

@jtpio
Copy link
Member

jtpio commented Jan 22, 2024

Also it may be useful to provide a migration guide, so folks who are already currently using the voici package in their deployments with a different kernel (for example jupyterlite-pyodide-kernel) know why they would start pulling jupyterlite-xeus-python when they update to a newer version.

@martinRenou
Copy link
Member Author

Thank you for the review!

I'd like to work on improving the documentation when the move to jupyterlite-xeus is done. Let's do that in a separate PR. Does it sound good @jtpio ?

@trungleduc
Copy link
Member

the package/voici package should live outside of python/voici-core?

@martinRenou
Copy link
Member Author

martinRenou commented Jan 22, 2024

the package/voici package should live outside of python/voici-core?

Should it? It seems to me this package only builds an index.js that is shipped by voici-core.

Otherwise we'd need two package.json in the repo, one for packages/voici and one for voici-core which depends on the first. I'm not sure I see the benefit of this?

@trungleduc
Copy link
Member

the package/voici package should live outside of python/voici-core?

Should it? It seems to me this package only builds an index.js that is shipped by voici-core.

Otherwise we'd need two package.json in the repo, one for packages/voici and one for voici-core which depends on the first. I'm not sure I see the benefit of this?

maybe not. It's just the general layout of a mono-python/js repo

@jtpio
Copy link
Member

jtpio commented Jan 22, 2024

I'd like to work on improving the documentation when the move to jupyterlite-xeus is done. Let's do that in a separate PR. Does it sound good @jtpio ?

Sure. Maybe this would also require addressing jupyterlite/xeus#17 first.

Just trying to think of ways for making this internal change as smooth as possible for existing Voici users.

@trungleduc
Copy link
Member

Just trying to think of ways for making this internal change as smooth as possible for existing Voici users.

We can release this change in a major version? Maybe starting 0.6.0 alphas with this PR

@martinRenou
Copy link
Member Author

So do you guys think there are blockers for this PR? What do you think about merging and we iterate upon it?

@martinRenou martinRenou changed the title Starting the split Spit the package into voici-core and voici Jan 23, 2024
@trungleduc
Copy link
Member

I think it's good to go

@martinRenou martinRenou merged commit f3a0b55 into voila-dashboards:main Jan 23, 2024
8 checks passed
@martinRenou martinRenou deleted the big_split branch January 23, 2024 09:04
@trungleduc
Copy link
Member

Thanks @martinRenou !

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