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

First attempt to setup GitHub actions #345

Merged
merged 42 commits into from
Sep 10, 2020
Merged

First attempt to setup GitHub actions #345

merged 42 commits into from
Sep 10, 2020

Conversation

krassowski
Copy link
Member

References

Trying to migrate to GitHub actions #311. It will be trial and error I guess. Not sure If I can do this without pushing to master though...

Code changes

User-facing changes

None

Backwards-incompatible changes

Chores

  • linted
  • tested
  • documented
  • changelog entry

activate-environment: jupyterlab-lsp
auto-update-conda: false

- name: testing dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll likely want to revisit this, and generate a known environment, rather than doing the solves multiple times.

@krassowski
Copy link
Member Author

@bollwyvl Would you like to take over to make conda update/install work? I am not an advanced conda user and it is getting more trial and error. I removed generation from the template as thought it would be easier, but I am confused on when to create the env ad how (note: I am happy for the generation from the template to be restored, I guess it would safe some run time).

@@ -27,7 +22,6 @@ dependencies:
- chktex
# test tools
- pytest-asyncio
- pytest-azurepipelines
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really is too bad... I haven't found anything to replace it yet on github actions (short of a dedicated ci bot that publishes gists)

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's this:
https://pypi.org/project/pytest-github-actions-annotate-failures/

that's a heckuva package name, but looks useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

(could not find a conda package, hence pip)

Copy link
Collaborator

Choose a reason for hiding this comment

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

great. i'll start up the conda-forge process for it, the lockfile approach can't handle pip deps... yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -5,11 +5,6 @@ channels:
- defaults

dependencies:
# runtime dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that's fine... I'm more and more thinking we want to just drive down the runtime cost of all these env shenanigans... something along the lines of conda/conda-lock#42

@bollwyvl
Copy link
Collaborator

bollwyvl commented Sep 8, 2020

Solider on! Let me know if you need any support!

I'm still investigating my issues on #328, but shouldn't be stepping on each others' toes.

@krassowski

This comment has been minimized.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Sep 9, 2020

I was able to spend a little time working the conda-lock piece, but it's not ready to push:

  • offline, solve all the environments we need based on
    • on the actions matrix
    • the requirements/*.yml
  • check lock files in (right now in .github/conda.locks
  • use setup-miniconda to handle the "plumbing" (setting shell variables etc) for the env -n jupyerlab-lsp (with just e.g. python)
  • conda create -n from the lockfile, derived from the matrix
    • use mamba on linux/osx, conda on windows
  • otherwise, should stay the same

Right now, these are the locks its generating with mamba on linux in about three minutes:

conda.ubuntu-16.04-3.6-10-2.2.lock
conda.ubuntu-16.04-3.6-13-2.2.lock
conda.ubuntu-16.04-3.7-10-2.2.lock
conda.ubuntu-16.04-3.7-13-2.2.lock
conda.ubuntu-16.04-3.8-10-2.2.lock
conda.ubuntu-16.04-3.8-13-2.2.lock
conda.macos-10.14-3.6-10-2.2.lock
conda.macos-10.14-3.6-13-2.2.lock
conda.macos-10.14-3.7-10-2.2.lock
conda.macos-10.14-3.7-13-2.2.lock
conda.macos-10.14-3.8-10-2.2.lock
conda.macos-10.14-3.8-13-2.2.lock
conda.vs2017-win2016-3.6-10-2.2.lock
conda.vs2017-win2016-3.6-13-2.2.lock
conda.vs2017-win2016-3.7-10-2.2.lock
conda.vs2017-win2016-3.7-13-2.2.lock
conda.vs2017-win2016-3.8-10-2.2.lock
conda.vs2017-win2016-3.8-13-2.2.lock

@krassowski
Copy link
Member Author

Looking at 12fe40d to backport it here...

@krassowski
Copy link
Member Author

offline, solve all the environments we need based on

Ideally we would have a GitHub action which would create a PR with the lock files I guess... It could trigger on push and then check if any of the requirements files has changed.

@krassowski
Copy link
Member Author

Or, could we just cache the solved environments?

@krassowski
Copy link
Member Author

krassowski commented Sep 9, 2020

This seems so cool: https://github.com/marketplace/actions/lint-action (auto fix and annotations!). What do you think @bollwyvl?

We could probably keep the old scripts for local use, and run both (first the github action, then the scripts); also because robot is not supported in the above.

@bollwyvl
Copy link
Collaborator

GitHub action which would create a PR with the lock files

I dunno... we don't do this with yarn.lock (but could), and there will be less tooling on this. Frankly, what I've liked doing (for both) is refreshing both locks during an "epoch" of development (usually a release) or when something "important" comes up... if it gets too automated, it can be hard to catch what's going on.

cache the solved environments?

Environments are not generally movable on disk. If they go back to the exact same path, it can work however. conda-pack can create an archive which needs one command to run (conda-unpack) after uncompressing to get over most of this (and works with more than just conda).

So a run would be like:

  • attempt to uncache the pack, based on the entropy of the lockfile
    • if that fails, build and cache the pack
    • if it succeeds do nothing, unpack and install

linter... auto fix and annotations

We shouldn't rely on anything unique on CI for anything, as then it gets really hard to reproduce locally. if the tool in question can respect our settings and not generate lots of false positives, that's great, and saves us work that we have decided to do.

I have found that a single-command workflow (e.g. make, or doit because windows), used locally and on CI, is more effective than investing more in exotic third-party CI-only gizmos... locally, it would just always lint before building and testing, whereas on CI, it could skip the lint if that's not that job's... job.

@bollwyvl
Copy link
Collaborator

bollwyvl commented Sep 10, 2020

The conda-lock approach is down to ~6 minutes of conda stuff on windows, and I think i can shave another minute off...

@krassowski
Copy link
Member Author

conda.vs2017-win2016-3.8-13-2.2.lock

We should use Node 14 if possible, 13 is EOL I think. The CI running right now should tell us if we can bump the version easily.

@krassowski
Copy link
Member Author

Looking at finalising this... Any preferences on coverage uploads? I am looking at https://github.com/marketplace/actions/codecov (no annoying comments, just uploads to the third-party servers). I am also happy to upload the xmls as artifacts if you need this.

@krassowski krassowski closed this Sep 10, 2020
@krassowski krassowski reopened this Sep 10, 2020
@bollwyvl
Copy link
Collaborator

It's fine if you want to land it... i've been tinkering with mine over here, not yet ready to go, and would not merge cleanly with the pip dep:

bollwyvl#6

I can PR on top of this, if you keep it open, or we can hit it later: once again, it pulled the new black, which inflates the delta some.

Any preferences on coverage uploads?

No opinion really: python side of the house is already pinned at 100, so it fails... npm side... well... if you're not failing things based on coverage, then it's just window dressing, and the jest tests aren't using real browsers... so i might not even care.

We should use Node 14 if possible,

have started that on above branch, appears to be working, so i'd say all the warnings are just... scary.

@krassowski
Copy link
Member Author

Merging as is, looking forward to even faster Windows builds and the conda package for pytest-github-actions-annotate-failures!

@krassowski krassowski merged commit e97324d into master Sep 10, 2020
@krassowski krassowski deleted the github-actions branch December 13, 2020 13:03
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