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

[squash] Jupyterlab3 CI updates #454

Merged
merged 26 commits into from
Jan 4, 2021

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Jan 3, 2021

References

Code changes

Packaging/metdata

  • moves more files in-tree in packages
    • was getting some weird stuff when installed as-packaged
  • updates install.json and jupyterlab-lsp/package.json to point at the new package name

CI

  • adds a build step to do the all the distribution builds, uploads for downstream jobs
    • nice side-effect: two commands can upload everything from CI-built assets 🎉
    • +~4min before any atest jobs start
  • uses --find-links and --no-index to install whls without knowing the version
    • removes this check from integrity
  • skips js builds altogether on atest
    • windows: -~3min
  • moves js unit tests to lint (since it already has to do a lot with js stuff)
  • uses mambaforge
    • windows: -~2min
  • tries to cache node_modules, falls back to .yarn-packages
    • windows: -~4min
  • removes the jedi and tectonic cache
    • these weren't doing anything positive because we overload $HOME
      • bringing them back would require more research into how to specify these locations with env vars or config, but would very probably reduce some robot timeouts
  • updates binder postBuild to (probably) work
    • haven't tested

timings above are:

User-facing changes

n/a

Backwards-incompatible changes

n/a

Chores

  • linted
  • tested
  • documented
  • changelog entry

@krassowski
Copy link
Member

I was thinking about making release workflow as separate from test, with some fancy trigger.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 4, 2021

I was thinking about making release workflow as separate from test,

I think the actual uploading is a separate concern. With this approach, I'm just ensuring we are testing exactly what we would be shipping. The release step could pull from this artifact, though I still am not super-excited about putting my pypi/npm creds into CI, no matter the encryption, etc. Another approach is to create github "draft" releases from tags, and then

with some fancy trigger.

If you do want to put creds in, a nice approach is posting a "draft" github release, which you can then Push The Button to promote it, and have that make the magic happen.

@krassowski
Copy link
Member

krassowski commented Jan 4, 2021

Right. Though it does not that bad - no need for full credentials, just API token which may be limited in scope to only one project, as per https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/

Edit: also missed one lint issue earlier - sorry!

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 4, 2021

Right, but I like those creds to only be available to the release job, and not to every random npm post-install script and github action.

@bollwyvl bollwyvl mentioned this pull request Jan 4, 2021
10 tasks
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 4, 2021

Calling it for the day. I updated the pr description with all the stuff I've been fiddling with. This is probably ready to be merged into #452, if there's nothing overly-offensive happening, but haven't retargeted as:

  • i'll happily fix anything you don't like
  • I don't know if re-targeting would kill the currently-running jobs which would have the most updated timings

Still no update on conda-forge/jupyter_server-feedstock#34 for py36 builds, maybe the PR fairy will come over night 🧚 .

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 4, 2021

Timings look good... getting close to 10 minutes saved on the slowest windows excursion, and inching us back from the dreaded hour.

Future versions of setup-miniconda will obviate a bunch of the hackery this introduces, as well.

Without doing more entropy plumbing with make/doit (e.g. pre-solved lock files), I don't think I can get it down much more... I guess the build could use setup-node and setup-python, which could shave another minute. Robot waiting Until Keyword Suceeds is definitely the long pole: I've found pabot to be awesome on linux/osx (up to about 4 processes on CI, basically linear speedup, though the server gets stood up more) but windows can't seem to handle it, so wouldn't move the needle much.

Will re-target, and will be off open source stuff (but occasionally checking email) for the next $DAY_JOB worth of hours... I've no idea what started on fire over the holidays!

@bollwyvl bollwyvl changed the base branch from master to jupyterlab3 January 4, 2021 13:42
@bollwyvl bollwyvl closed this Jan 4, 2021
@bollwyvl bollwyvl reopened this Jan 4, 2021
@bollwyvl bollwyvl changed the title [wip] Jupyterlab3 CI updates [squash] Jupyterlab3 CI updates Jan 4, 2021
"packageName": "jupyter_lsp",
"uninstallInstructions": "Use your Python package manager (pip, conda, etc.) to uninstall the package jupyter_lsp"
"packageName": "jupyterlab_lsp",
"uninstallInstructions": "Use your Python package manager (pip, conda, etc.) to uninstall the package jupyterlab_lsp"
Copy link
Member

Choose a reason for hiding this comment

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

Good catch

@@ -130,7 +129,7 @@
"discovery": {
"server": {
"base": {
"name": "jupyter-lsp"
"name": "jupyterlab_lsp"
Copy link
Member

Choose a reason for hiding this comment

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

Well, if I publish another npm release and someone installs from npm, jupyter-lsp would suffice. Otherwise they would not see this anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, who knows. just thought of it while fixing the other one...

acceptance:
runs-on: ${{ matrix.os }}-latest
name: ${{ matrix.os }} py${{ matrix.python }} node${{ matrix.nodejs }}
name: atest ${{ matrix.os }} py${{ matrix.python }}
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of atest abbreviation, but we already use it so it can stay I guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can call it whatever you like! but shorter is better so we can get some info off the UI. robot, browser, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if they could be emoji, i'd be all for 🤖

@krassowski
Copy link
Member

Great work, I like the idea with building once! I restarted the CI to make sure it works with 3.6.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 4, 2021

omg so green 🤯

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