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

Add github action running on each push #33263

Closed
tobiasdiez opened this issue Jan 31, 2022 · 32 comments
Closed

Add github action running on each push #33263

tobiasdiez opened this issue Jan 31, 2022 · 32 comments

Comments

@tobiasdiez
Copy link
Contributor

Add a github action that runs on each push. Since it uses the most recent docker image as the base, the runtime is relatively short with about 2h. The idea is to get quick feedback on PRs without the need to wait until the patchbot picks it up. After this ticket is merged, we can add a badge in the ticket similar to the linter workflow.

Example run: https://github.com/sagemath/sagetrac-mirror/actions/workflows/build.yml
(it works but one doctest and the pytests are failing; I think there are already tickets for these issues)

Depends on #33103

CC: @mkoeppe @fchapoton

Component: build

Author: Tobias Diez

Branch/Commit: 6329816

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33263

@tobiasdiez tobiasdiez added this to the sage-9.6 milestone Jan 31, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 31, 2022

Changed commit from 676799b to 1ab0755

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 31, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

1ab0755Merge branch 'develop' of https://github.com/sagemath/sage into public/build/github_build

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 31, 2022

comment:3

Why not just configure --prefix=/sage/local?

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 31, 2022

comment:4

Which jobs does it cancel?

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 31, 2022

comment:5

You may want to use #33233

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 31, 2022

comment:6

Replying to @mkoeppe:

Which jobs does it cancel?

Also perhaps the new built-in concurrency stuff can be used? https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

d27ca7fImprove based on feedback

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2022

Changed commit from 1ab0755 to d27ca7f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2022

Changed commit from d27ca7f to 4f5bcb9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

4f5bcb9Test that cancel works as desired

@tobiasdiez
Copy link
Contributor Author

comment:9

Thanks for the feedback. I've now used --prefix and concurrency (to cancel previous runs of the same workflow for the same branch).

@tobiasdiez
Copy link
Contributor Author

comment:10

Replying to @mkoeppe:

You may want to use #33233

The idea is that this workflow never fails (otherwise the branch is not merged). Thus the base line is "always green".

@tobiasdiez
Copy link
Contributor Author

comment:11

Replying to @mkoeppe:

Why not just configure --prefix=/sage/local?

But now it seems to rebuild all python packages instead of using them from the docker image as before. Is this a bug in prefix or did I use it wrong?

@tobiasdiez

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 1, 2022

comment:13

Sorry, it should be configure --prefix=/sage/local --with-sage-venv

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 1, 2022

comment:14

Replying to @tobiasdiez:

Replying to @mkoeppe:

You may want to use #33233

The idea is that this workflow never fails (otherwise the branch is not merged). Thus the base line is "always green".

The patchbot makes the same optimistic assumption, which is why it's broken so often...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

3b0e99fUse --with-sage-venv

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2022

Changed commit from 4f5bcb9 to 3b0e99f

@tobiasdiez
Copy link
Contributor Author

comment:16

Replying to @mkoeppe:

Sorry, it should be configure --prefix=/sage/local --with-sage-venv

Thanks, that worked well!

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 1, 2022

comment:17

Another idea, for a much faster build, would be to just work out of /sage, merging the current commit.

@tobiasdiez
Copy link
Contributor Author

comment:18

I think with a bit more than 1.5h runtime, it is already reasonably fast. The actual build also only takes about 35mins, the rest is spent on the tests. Further optimization in my opinion will be a trade-off between time savings and reduced reliability, because you then have to think about invalidation of the cythonize cache etc. The point of this ticket is to pursue a different approach than the patchbot and really start from a clean state. I would propose to first gain experience with this, before we implement more fancy caching mechanisms (and in this case I would also prefer github actions builtin cache mechanism).

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2022

comment:19

Sure, sounds good.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2022

comment:20

However I think it should only be run for branches on tickets set to needs_review.
After making git-trac available (see #33222), you can use something like git trac search --branch u/mkoeppe/upgrade_giac_to_1_7_0
and git trac print 33263 | grep '^Status: needs_review' for this

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2022

comment:21

Hm... no, people set "needs_review" after a push, not before

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2022

Dependencies: #33103

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2022

comment:22

OK, let's just merge it and try it out. I'd suggest to set #33103 as a dependency and already put dev instead of 9.5 as image tag

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 2, 2022

Reviewer: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

6329816Use dev docker tag

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2022

Changed commit from 3b0e99f to 6329816

@tobiasdiez
Copy link
Contributor Author

comment:24

Replying to @mkoeppe:

OK, let's just merge it and try it out. I'd suggest to set #33103 as a dependency and already put dev instead of 9.5 as image tag

Thanks for the review. It now uses dev and I set the ticket to positive review now (please intervene if I misunderstood you).

@tobiasdiez
Copy link
Contributor Author

comment:25

Increasing priority as the badge is already added to the trac ticket (so that I don't have to login in the VM and change the config multiple times).

@vbraun
Copy link
Member

vbraun commented Feb 16, 2022

Changed branch from public/build/github_build to 6329816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants