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

Binder and yarn tweaks #3795

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Binder and yarn tweaks #3795

merged 8 commits into from
Jul 31, 2023

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jun 30, 2023

(Separate) commits in this PR:

  1. Upgrade jupyterlab in Binder from 3.x to 4.0.2

  2. Run 'yarn set version stable' in Binder, to use yarn 3

  3. Remove yarn-deduplicate, which is only compatible with yarn 1. (It's unnecessary with yarn 2+, anyway.)

  4. Adds packageExtensions: configs to the .yarnrc.yml, to silence warnings about undeclared dependencies in imported packages. (Except for the fundamental incompatibility of istanbul-instrumenter-loader with webpack 5.) See Setting up repo with yarn 3 fails #3797 for more info.

    (Note: There's also a remaining undeclared react dependency in jupyterlab/settingregistry, but adding that to the packageExtensions makes react a dependency of the project as well, which was a road I decided not to go down.)

  5. Create a .prettierignore file in the repo root, and add .yarnrc.yml to it.
    Add a comment,

    # prettier-ignore

    to the section of .yarnrc.yml that Yarn will edit on its own. (Using a .prettierignore file didn't work, because the scripting is already passing --ignore-file .gitignore on the command line, and Prettier doesn't support multiple ignore files.)

    Yarn edits the .yarnrc.yml file automatically, and its own style rules conflict with Prettier's. The two are in constant tension if Prettier is also making changes. It's best to consider the file's contents as managed by Yarn.

    Additional # prettier-ignore comments may be needed as the file grows. The comment only disables Prettier for the single block of code immediately following it.

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch ferdnyc/ipywidgets/patch-1

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 30, 2023

Well, that did nothing. 3.6.4 is already preinstalled, so it's kept as-is.

@ferdnyc ferdnyc marked this pull request as draft June 30, 2023 07:44
@martinRenou
Copy link
Member

which is now a bad thing as ipywidgets wants 4.x.

ipywidgets works with JupyterLab 3 and 4

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 30, 2023

@martinRenou

Ah! Well, the explicitly not-matching versions of both jupyterlab and yarn seem bad, here...

Checking JupyterLab (assuming JupyterLab >=4)... 3.6.4
Installing and building all yarn packagesyarn install v1.21.1
[1/5] Validating package.json...
error @: The engine "yarn" is incompatible with this module. Expected version ">=3". Got "1.21.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

@martinRenou
Copy link
Member

martinRenou commented Jun 30, 2023

Oh yes. It is indeed more subtle and binder may need JupyterLab 4 if it uses a dev version of ipywidgets.

ipywidgets now requires to be built against JupyterLab 4. Once built and published on PyPi/conda-forge, it will work with both JupyterLab 3 and JupyterLab 4.

Before #3752, ipywidgets required to be built against JupyterLab 3, but that does not mean the built and published package did not work with JupyterLab 4 (apart from a couple small issues it is working).

So you're right, we should update the binder setup to use JupyterLab 4 if binder builds its own version from dev.

@martinRenou
Copy link
Member

I would be in favor of not using a dev version of ipywidgets on Binder though, and instead use the latest published version on either PyPi or conda-forge.

Because it may be surprising to users to not get the same features in their local setup as in the binder.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 30, 2023

@martinRenou

Binder from pull requests should probably build ipywidgets from the PR's source branch, though, no? Otherwise, that automatic comment with a link to a Binder notebook... running ipywidgets without any of the code from the PR... seems not entirely helpful/useful.

@ferdnyc ferdnyc marked this pull request as ready for review July 2, 2023 05:46
Copy link
Contributor Author

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

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

Speaking of yarn invocations, apparently with Yarn 2+ it's... well, debatably a good idea, but at the very least "not wrong" (according to them, anyway) to check into the repo either:

  1. The Yarn version in use, so that everyone is running the same tools, or
  2. The entire dependency cache, iff it's in the form of compressed package archives rather than a giant, exploded node_modules hierarchy. That requires switching to using their pnp linker, instead of the node-modules one that's the default for projects coming from yarn 1.

Not necessities by any stretch, but apparently options possibly worth considering.

.binder/postBuild Outdated Show resolved Hide resolved
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jul 3, 2023

The test failure is just a change in font selection, here's the expected:
8748451373021fa6d781e7340c226aa3b04b8212

And the actual:
13803ac72b8a2a5a1ff6f4a05e97b811f37659da

....Aand, somehow that magically cleared itself up. Weird.

@ferdnyc ferdnyc changed the title Binder: Unpin jupyterlab from version 3.x Binder and yarn tweaks Jul 3, 2023
Yarn will reformat the file as it sees fit, so having prettier edit
it as well creates a constant back-and-forth of style changes.
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry for the late review

@martinRenou martinRenou merged commit 4153f16 into jupyter-widgets:main Jul 31, 2023
@ferdnyc ferdnyc deleted the patch-1 branch July 31, 2023 12:11
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

Successfully merging this pull request may close these issues.

2 participants