ci: Refactor workflows so that they are usable by forks #981

thomass-dev · 2024-12-18T10:17:35Z

This PR addresses several issues concerning the CI in PR from forks:

Refactor workflows so that they are usable in every PR
- assignee can be added to every PR
- coverage comments can be send to every PR
Add workflow to serve documentation preview and send a comment with the corresponding link in PR
Add workflow to clean documentation preview and GH artifacts after closing a PR

glemaitre · 2024-12-31T15:20:40Z

.github/workflows/backend.yml

+        if: ${{ ! matrix.coverage }}
+        timeout-minutes: 10
+        working-directory: skore/
+        run: python -m pytest -n auto --no-cov src/ tests/


Something that I wondered: why not always testing with the coverage? In scikit-learn, we have some build jobs that don't do so to speed-up the CI but because we are at almost 25 minutes otherwise.

So if you are still in a reasonable time, I would not bother and always collect coverage.

glemaitre · 2024-12-31T16:00:14Z

.github/workflows/pr-cleanup.yml

+          HEAD_REPOSITORY_ID: ${{ github.event.pull_request.head.repo.id }}
+          HEAD_BRANCH: ${{ github.head_ref }}
+
+  clean-documentation-preview:


It looks like you use this to clean the buckets. Would it be easier to set up a time-based purge like in Azure or CircleCI that delete your preview after a week or so. In this way you don't have to maintain this script?

glemaitre · 2024-12-31T16:01:20Z

.github/workflows/pr-cleanup.yml

+      - name: Clean documentation preview
+        run: rclone --config rclone.configuration purge "${PROVIDER}:${BUCKET}/${PULL_REQUEST_NUMBER}"
+        env:
+          PROVIDER: scaleway


I'm wondering now what is the expected cost to store the buckets given a certain activity. It is just to have an idea.

lesteve · 2025-01-06T08:05:16Z

I haven't had a detailed look 😓 but based on my personal experience with CI (and biases thereof), I would suggest the following for the doc preview rather than maintaining a custom setup:

readthedocs has built-in support for doc preview. The limitation is build time (15 minutes) and memory (7GB) see doc. We do this for joblib.
Github action + Netlify (or Vercel). We do this for the scikit-learn MOOC. Limitation is that there are some limits in Netlify but I don't remember them very well (maybe some kind of size limit if you upload too many times). Also IIRC you need to apply for an open-source plan and the rules are not super clear. IIRC you are supposed to acknowledge Netlify so I think we put links into the preview somewhere to try to follow them (I took this from another project).
CircleCI for doc preview + redirect-action for link inside CI statuses. We do this for scikit-learn.

Of course, do what you think is best for skore and very happy to chat about what you learn if you take another road. Out of curiosity did you draw for inspiration from an existing project for the doc preview?

.github/CODEOWNERS

rouk1 · 2025-01-06T14:52:38Z

.github/actions/workflow-run/context/action.yml

+  Acquire the PR number and the PR commit HEAD sha, after a "workflow_run" event.
+
+  The "workflow_run" context differs from the "pull_request" context and doesn't contain
+  PR basic information. This action intends to provide some missing information in the
+  "workflow_run" context, without having to explicitly record them in the workflow that
+  triggered the "workflow_run" event.


This comment should express the difference between "workflow_run" and other execution context.

rouk1 · 2025-01-06T14:57:33Z

.github/workflows/pr-add-assignee.yml


 on:
-  pull_request:
+  pull_request_target:


We may document here that no user code should be executed in this action.

rouk1 · 2025-01-06T15:00:33Z

.github/workflows/pr-cleanup.yml

+name: cleanup PR
+
+on:
+  pull_request_target:


You should document that no user code should be run in this PR.

glemaitre · 2025-01-08T10:19:12Z

@thomass-dev do you want to move forward on this PR. I can later help at having a spin on #916 because it looks this one will be a blocker.

# Conflicts: # .github/workflows/skore.yml

thomass-dev · 2025-01-09T14:26:59Z

@rouk1 i want to postpone the writing of the ADR in #1066. Are you okay to merge in state?

rouk1 · 2025-01-09T14:58:48Z

@rouk1 i want to postpone the writing of the ADR in #1066. Are you okay to merge in state?

ok lgtm then !

thomass-dev force-pushed the fix-ci-on-fork branch 2 times, most recently from 0436ef4 to 47a7b9f Compare December 18, 2024 10:37

thomass-dev added a commit to thomass-dev/skore-fork that referenced this pull request Dec 18, 2024

ci: Test FORK based on probabl-ai#981

51b4c1e

thomass-dev added a commit to thomass-dev/skore-fork that referenced this pull request Dec 18, 2024

ci: Test FORK based on probabl-ai#981

4ebfa51

thomass-dev force-pushed the fix-ci-on-fork branch 2 times, most recently from b85e586 to 4721ba9 Compare December 18, 2024 20:09

thomass-dev added a commit to thomass-dev/skore-fork that referenced this pull request Dec 19, 2024

ci: Test FORK based on probabl-ai#981

9b0e154

thomass-dev force-pushed the fix-ci-on-fork branch from 4721ba9 to c622923 Compare December 19, 2024 09:32

thomass-dev added a commit to thomass-dev/skore-fork that referenced this pull request Dec 19, 2024

ci: Test FORK based on probabl-ai#981

2becadd

thomass-dev force-pushed the fix-ci-on-fork branch from c622923 to 37373ef Compare December 19, 2024 09:39

thomass-dev added a commit to thomass-dev/skore-fork that referenced this pull request Dec 19, 2024

ci: Test FORK based on probabl-ai#981

ae9072c

thomass-dev self-assigned this Dec 26, 2024

thomass-dev force-pushed the fix-ci-on-fork branch 12 times, most recently from 5f7d570 to 5d6b2f5 Compare December 26, 2024 19:23

thomass-dev force-pushed the main branch from 74f7894 to 469d1e5 Compare December 26, 2024 19:37

thomass-dev force-pushed the fix-ci-on-fork branch 3 times, most recently from 0f30aa2 to 58d9cfd Compare December 27, 2024 08:31

thomass-dev force-pushed the main branch from 970eda1 to ea1de09 Compare December 27, 2024 08:56

thomass-dev force-pushed the fix-ci-on-fork branch from 1041c45 to f9a7942 Compare December 27, 2024 08:59

glemaitre reviewed Dec 31, 2024

View reviewed changes

thomass-dev added 5 commits January 3, 2025 15:13

ci: Refactor workflows so that they are usable by forks

acf2a77

ci: Add workflow to serve documentation preview in PRs (#1027)

9c7e6d0

ci: Factorize acquire-pr-context job (#1035)

d9edb1d

ci: Compute coverage only on skore, not anymore on src/ (#1036)

b25d003

ci: Fix pytest-cov and pytest-xdist bug

eaa4282

thomass-dev force-pushed the fix-ci-on-fork branch from 14889c5 to eaa4282 Compare January 3, 2025 14:13

thomass-dev added 2 commits January 3, 2025 16:49

Merge branch 'main' into fix-ci-on-fork

747f6c3

Merge branch 'main' into fix-ci-on-fork

d532328

rouk1 reviewed Jan 6, 2025

View reviewed changes

.github/CODEOWNERS Show resolved Hide resolved

rouk1 reviewed Jan 6, 2025

View reviewed changes

thomass-dev linked an issue Jan 7, 2025 that may be closed by this pull request

Having documentation preview in PR would be really handy for reviewing #958

Closed

thomass-dev marked this pull request as draft January 8, 2025 11:19

thomass-dev force-pushed the fix-ci-on-fork branch from d150c2f to 35857f3 Compare January 9, 2025 13:45

Merge branch 'main' into fix-ci-on-fork

0c61a8d

# Conflicts: # .github/workflows/skore.yml

thomass-dev force-pushed the fix-ci-on-fork branch from 35857f3 to 0c61a8d Compare January 9, 2025 14:00

Remove CI badge from front page

31c9a16

thomass-dev mentioned this pull request Jan 9, 2025

docs: Make an ADR on CI architecture #1066

Open

thomass-dev marked this pull request as ready for review January 9, 2025 14:53

rouk1 approved these changes Jan 9, 2025

View reviewed changes

thomass-dev merged commit be1aec3 into main Jan 9, 2025
27 checks passed

thomass-dev deleted the fix-ci-on-fork branch January 9, 2025 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Refactor workflows so that they are usable by forks #981

ci: Refactor workflows so that they are usable by forks #981

thomass-dev commented Dec 18, 2024 •

edited

Loading

glemaitre Dec 31, 2024

glemaitre Dec 31, 2024

glemaitre Dec 31, 2024

lesteve commented Jan 6, 2025 •

edited

Loading

rouk1 Jan 6, 2025 •

edited

Loading

rouk1 Jan 6, 2025

rouk1 Jan 6, 2025

glemaitre commented Jan 8, 2025

thomass-dev commented Jan 9, 2025

rouk1 commented Jan 9, 2025

ci: Refactor workflows so that they are usable by forks #981

ci: Refactor workflows so that they are usable by forks #981

Conversation

thomass-dev commented Dec 18, 2024 • edited Loading

glemaitre Dec 31, 2024

Choose a reason for hiding this comment

glemaitre Dec 31, 2024

Choose a reason for hiding this comment

glemaitre Dec 31, 2024

Choose a reason for hiding this comment

lesteve commented Jan 6, 2025 • edited Loading

rouk1 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

rouk1 Jan 6, 2025

Choose a reason for hiding this comment

rouk1 Jan 6, 2025

Choose a reason for hiding this comment

glemaitre commented Jan 8, 2025

thomass-dev commented Jan 9, 2025

rouk1 commented Jan 9, 2025

thomass-dev commented Dec 18, 2024 •

edited

Loading

lesteve commented Jan 6, 2025 •

edited

Loading

rouk1 Jan 6, 2025 •

edited

Loading