Skip to content

Conversation

@thomass-dev
Copy link
Collaborator

@thomass-dev thomass-dev commented Apr 2, 2025

Some changes to prepare the setup of the GH merge queue, for which we need to specify the jobs required to succeed:

  • Set filenames as workflow/job names to avoid confusion,
  • Add backend job that can be used as required 🟢 job,
  • Add sphinx job that can be used as required 🟢 job,
  • Add explicit restricted permissions to the sphinx workflow (no worries, they were already restricted to read-only).

For a job to be required, its workflow has to be executed and not skipped.
Before this PR, we skipped entire workflow via path filtering, which is incompatible with "required jobs".

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks

image

Now we always execute workflows, but skip jobs individually based on modified files.

@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch 4 times, most recently from a1e1f22 to 67f1ed4 Compare April 3, 2025 15:35
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

Coverage

Coverage Report for backend
FileStmtsMissCoverMissing
venv/lib/python3.12/site-packages/skore
   __init__.py220100% 
   _config.py280100% 
   exceptions.py440%4–23
venv/lib/python3.12/site-packages/skore/persistence
   __init__.py00100% 
venv/lib/python3.12/site-packages/skore/persistence/item
   __init__.py55198%97
   altair_chart_item.py19191%14
   item.py22195%86
   matplotlib_figure_item.py36195%19
   media_item.py220100% 
   numpy_array_item.py27194%16
   pandas_dataframe_item.py29194%14
   pandas_series_item.py29194%14
   pickle_item.py220100% 
   pillow_image_item.py25193%15
   plotly_figure_item.py20192%14
   polars_dataframe_item.py27194%14
   polars_series_item.py22192%14
   primitive_item.py23291%13–15
   sklearn_base_estimator_item.py29194%15
venv/lib/python3.12/site-packages/skore/persistence/repository
   __init__.py20100% 
   item_repository.py59591%15–16, 202–203, 226
venv/lib/python3.12/site-packages/skore/persistence/storage
   __init__.py40100% 
   abstract_storage.py220100% 
   disk_cache_storage.py33195%44
   in_memory_storage.py200100% 
venv/lib/python3.12/site-packages/skore/project
   __init__.py20100% 
   project.py83298%280, 392
venv/lib/python3.12/site-packages/skore/sklearn
   __init__.py60100% 
   _base.py1711492%45, 58, 126, 129, 182–191, 203–>209, 224, 227–228
   find_ml_task.py61099%136–>145
   types.py130100% 
venv/lib/python3.12/site-packages/skore/sklearn/_comparison
   __init__.py50100% 
   metrics_accessor.py165297%163, 164–>166, 1278
   report.py67197%17, 249–>252
venv/lib/python3.12/site-packages/skore/sklearn/_cross_validation
   __init__.py50100% 
   metrics_accessor.py190099%153–>155, 155–>157
   report.py110198%23
venv/lib/python3.12/site-packages/skore/sklearn/_estimator
   __init__.py70100% 
   feature_importance_accessor.py133099%483–>489, 569–>578
   metrics_accessor.py3441096%174–183, 211–>220, 219, 249, 260–>262, 290, 317–321, 336, 371, 372–>374
   report.py148198%24, 253–>255
venv/lib/python3.12/site-packages/skore/sklearn/_plot
   __init__.py20100% 
   base.py60100% 
   style.py280100% 
   utils.py122595%51, 75–77, 81
venv/lib/python3.12/site-packages/skore/sklearn/_plot/metrics
   __init__.py40100% 
   precision_recall_curve.py173199%660
   prediction_error.py1640100% 
   roc_curve.py176199%649
venv/lib/python3.12/site-packages/skore/sklearn/train_test_split
   __init__.py00100% 
   train_test_split.py51196%16, 154–>158
venv/lib/python3.12/site-packages/skore/sklearn/train_test_split/warning
   __init__.py80100% 
   high_class_imbalance_too_few_examples_warning.py17190%79
   high_class_imbalance_warning.py180100% 
   random_state_unset_warning.py12188%15
   shuffle_true_warning.py10183%46
   stratify_is_set_warning.py12188%15
   time_based_column_warning.py23286%17, 73
   train_test_split_warning.py40100% 
venv/lib/python3.12/site-packages/skore/utils
   __init__.py60100% 
   _accessor.py46197%102
   _environment.py27097%30–>35
   _fixes.py80100% 
   _index.py50100% 
   _logger.py22485%15–19
   _measure_time.py100100% 
   _parallel.py38388%23–33, 124
   _patch.py13553%21–37
   _progress_bar.py360100% 
   _show_versions.py330100% 
TOTAL31858296% 

Tests Skipped Failures Errors Time
814 8 💤 0 ❌ 0 🔥 54.589s ⏱️

@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from 67f1ed4 to 77eb86a Compare April 3, 2025 15:49
@thomass-dev thomass-dev changed the title ci: Reintroduce ci.yml workflow as the "must-be-green" CI entry point ci: Introduce <backend|sphinx>-all-green jobs as required jobs Apr 3, 2025
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from d462471 to 117b615 Compare April 9, 2025 15:59
@thomass-dev thomass-dev changed the title ci: Introduce <backend|sphinx>-all-green jobs as required jobs ci: Introduce <backend|sphinx> jobs as all green required jobs Apr 9, 2025
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from 117b615 to 563c861 Compare April 9, 2025 18:12
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from b8a1a97 to e7b42f7 Compare April 24, 2025 09:37
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch 2 times, most recently from 6d7dc53 to bf52fc7 Compare April 24, 2025 12:14
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025

Documentation preview @ 35583c3

@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from 1d74549 to bf52fc7 Compare April 24, 2025 13:04
@thomass-dev thomass-dev changed the title ci: Introduce <backend|sphinx> jobs as all green required jobs ci: Introduce <backend|sphinx> jobs as "all green" required jobs Apr 24, 2025
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from bf52fc7 to 7fb05f8 Compare April 24, 2025 14:11
@thomass-dev thomass-dev marked this pull request as ready for review April 24, 2025 14:12
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from 7fb05f8 to f48e20f Compare April 24, 2025 14:16
@thomass-dev thomass-dev force-pushed the ci-required-status-checks branch from 5704864 to dcebaa5 Compare April 24, 2025 14:25
@thomass-dev thomass-dev merged commit c5c50cc into main Apr 24, 2025
28 checks passed
@thomass-dev thomass-dev deleted the ci-required-status-checks branch April 24, 2025 15:24
Copy link
Contributor

@rouk1 rouk1 left a comment

Choose a reason for hiding this comment

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

Cool !

pull-requests: read
steps:
- name: Checkout code
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why you need to pin a specific version here ?

Copy link
Collaborator Author

@thomass-dev thomass-dev Apr 24, 2025

Choose a reason for hiding this comment

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

This is only a security issue and a good practice promoted by GH after the corruption of the famous tj-actions/changed-files.

tj-actions/changed-files#2463 (comment)
https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions

You can help mitigate this risk by following these good practices:

Pin actions to a full length commit SHA

Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload. When selecting a SHA, you should verify it is from the action's repository and not a repository fork.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is also a tradeoff. If a pinned version contains a CVE then you wont get the fix for free.
To me this is too much paranoia.

Copy link
Collaborator Author

@thomass-dev thomass-dev Apr 24, 2025

Choose a reason for hiding this comment

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

I know, but there is actually no better solution to mitigate this issue, and it's promoted by GH.
By pinning an approved version, we avoid to pull future potential compromised versions.


## Context and Problem Statement

With external contributions related to ESOC, the number of pull-request is growing fast.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please disambiguate thie acronym.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

European Summer of Code, sorry too late... 😶‍🌫️ .

Copy link
Contributor

Choose a reason for hiding this comment

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

https://duckduckgo.com/?q=ESOC

leads to European Space Operations Centre 🤷🏻‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update it in another PR, thanks!

Muhammad-Rebaal pushed a commit to Muhammad-Rebaal/skore that referenced this pull request May 5, 2025
…robabl-ai#1514)

Some changes to prepare the setup of the GH merge queue, for which we
need to specify the jobs required to succeed:

- Set filenames as workflow/job names to avoid confusion,
- Add `backend` job that can be used as required 🟢 job,
- Add `sphinx` job that can be used as required 🟢 job,
- Add explicit restricted permissions to the `sphinx` workflow (no
worries, they were already restricted to read-only).

---

For a job to be required, its **workflow** has to be executed and not
skipped.
Before this PR, we skipped entire workflow via path filtering, which is
incompatible with "required jobs".


https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks


![image](https://github.com/user-attachments/assets/a48bab3f-c361-4a55-b933-9cf94057f26c)

Now we always execute workflows, but skip jobs individually based on
modified files.
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.

4 participants