use parallel workflows #150

galleon · 2021-12-09T12:55:36Z

This PR structures the build in 3 different branches:

each branch is in charge of an operating system family
each build/test job takes 2 parameters to define a strategy :
- one for the python versions
- one for the different versions of the os we want to build or test on
  - we currently build onto the latest OS except for windows were we use windows-2016. Latest versions are currently:
  - we test on more versions. Currenty:
    - windows-2016
    - macos-10.15 and macos-11
    - unbuntu-18.04 and ubuntu-20.04
the doc is build as soon as possible i.e. as soon as the tests on linux are finished.

This structure does not increase the build time (it slightly decreases it

dbarbier · 2021-12-09T13:15:26Z

~~How are reviewers supposed to review your changes if you do not explain what these changes are supposed to do?~~ Thanks for the update

dbarbier · 2021-12-11T08:25:34Z

.github/workflows/build.yml

+          echo "::set-output name=build_macos::['macos-latest']"
+          echo "::set-output name=test_macos::['macos-10.15', 'macos-11']"
+          echo "::set-output name=build_linux::['ubuntu-latest']"
+          echo "::set-output name=test_linux::['ubuntu-18.04', 'ubuntu-20.04']"


We had previously:

Build matrix: [windows-2016, macos-10.15, ubuntu-latest]

Test matrix: [windows-latest, macos-latest, ubuntu-latest]

This PR should mimic that, and configuration changes are made in another PR.

done - back to previous non // set-up

BTW it will be simpler to write this step in Python when you will make this workflow more complex; db/shell-python does that. (And db/shell-python-extended shows how this Python code can be modified in order to implement some complex processing in future PRs, if desired)

done - taken

dbarbier · 2021-12-11T08:27:04Z

.github/workflows/build.yml

+          echo "::set-output name=build_linux::['ubuntu-latest']"
+          echo "::set-output name=test_linux::['ubuntu-18.04', 'ubuntu-20.04']"
+
+  my_echo:


Please remove this job; do not keep debugging stuff in production code, this is useless and may add more bugs

dbarbier · 2021-12-11T08:27:58Z

.github/workflows/build.yml

+        shell: bash
+    runs-on: ${{ matrix.os }}
+
+    env:


env is the same for all jobs and could be declared globally

dbarbier · 2021-12-11T08:29:47Z

.github/workflows/build.yml

+        if: steps.cache-build-dependencies.outputs.cache-hit != 'true'
+        run: echo "SKDECIDE_SKIP_DEPS=0" >> $GITHUB_ENV
+
+      - name: Build Windows wheel


Remove mentions of OS from step names (in all steps), this is no more useful when jobs are split by OS.

$ grep -i -E 'name:.*(linux|macos|windows)' .github/workflows/build.yml - name: Install omp on MacOS - name: Install and restore ccache for macOs - name: Let cmake use ccache for macOs - name: Restore docker dev image for Linux - name: Restore ccache cache for Linux

grep -i -E 'name:.*(linux|macos|windows)' .github/workflows/build.yml [🐍 scikit-decide]

dbarbier · 2021-12-11T08:34:50Z

.github/workflows/build.yml

        compiler: [gnu]
      fail-fast: true
+    defaults:
+      run:
+        shell: bash


No idea why, but test failures occur only when running in bash, remove that stance to have the same behavior as before. Remove it from all 3 test-* jobs, they were not set previously and are useless anyway on Unix.

ok back to previous state.

dbarbier · 2021-12-11T08:40:17Z

.github/workflows/build.yml

@@ -352,7 +553,7 @@ jobs:

  upload-nightly:
    if: (github.ref == 'refs/heads/master') && (github.repository == 'airbus/scikit-decide') && (github.event_name == 'schedule')
-    needs: [test-unix, test-macos, test-windows]
+    needs: [build-docs, test-macos, test-windows]


why this change?

As explained in the description of the PR. Building the doc is done as early as possible. Since it is done on ubuntu it is done after tests have been performed. Therefore build-docs depends now from test-linux and upload-nightly depends on build-docs

Previously upload-nightly and build-docs were independent, both depending on test-*. There is no reason to change that in this PR.

ok went back to same structure

dbarbier · 2021-12-11T08:51:54Z

.github/workflows/build.yml

    strategy:
+      matrix:
+        os: ${{ fromJSON(needs.init.outputs.build_windows) }}
+        python-version: ${{ fromJSON(needs.init.outputs.python_versions) }}


python-version on the left and python_versions on the right, that is not ideal

dbarbier

build.yml looks good, you can squash commits after changing the setup step as suggested.

But release.yml is wrong, you copy/pasted build.yml, you should instead have duplicated the steps in release.yml. We do not want to use ccache in release.yml, maybe there are other differences, I do not want to check, this would require too much time.

In fact release.yml could be left as is, we won't implement [ci: skip] keywords in this workflow, so it is less important. If you do want to change it anyway, please make sure that the new release.yml does the exact same thing as the old one.

dbarbier · 2021-12-12T10:38:51Z

.github/workflows/build.yml

+          python_version = ["3.7", "3.8", "3.9"]
+          build = { "macos": ["macos-10.15"], "linux": ["ubuntu-latest"], "windows": ["windows-2016"] }
+          test = { "macos": ["macos-latest"], "linux": ["ubuntu-latest"], "windows": ["windows-latest"] }
+          print("::set-output name=python_version::['3.7', '3.8', '3.9']")


You should write

print(f"::set-output name=python_version::{python_version}")

still not fixed after rebase

still not fixed

dbarbier · 2021-12-12T10:39:52Z

.github/workflows/release.yml

+          python_version = ["3.7", "3.8", "3.9"]
+          build = { "macos": ["macos-10.15"], "linux": ["ubuntu-latest"], "windows": ["windows-2016"] }
+          test = { "macos": ["macos-latest"], "linux": ["ubuntu-latest"], "windows": ["windows-latest"] }
+          print("::set-output name=python_version::['3.7', '3.8', '3.9']")


print(f"::set-output name=python_version::{python_version}")

dbarbier · 2021-12-12T10:56:26Z

[...]

In fact release.yml could be left as is, we won't implement [ci: skip] keywords in this workflow, so it is less important. If you do want to change it anyway, please make sure that the new release.yml does the exact same thing as the old one.

And if you want to modify release.yml, you should rebase on current master which also modified this file.

galleon · 2021-12-12T11:28:44Z

I have re-squashed and kept 2 commits one for each file.
Applying the same pattern on release should allow later to skip pushing release to PyPI if needed or doing only documentation publication.

dbarbier · 2021-12-12T11:43:41Z

Applying the same pattern on release should allow later to skip pushing release to PyPI if needed

I cannot imagine why that would be useful.

or doing only documentation publication.

Since release.yml is only triggered by pushing tags, this would mean that you delete and re-push a tag for a published version, this is certainly not desirable.

dbarbier · 2021-12-12T19:03:22Z

.github/workflows/build.yml

+          python_version = ["3.7", "3.8", "3.9"]
+          build = { "macos": ["macos-10.15"], "linux": ["ubuntu-latest"], "windows": ["windows-2016"] }
+          test = { "macos": ["macos-latest"], "linux": ["ubuntu-latest"], "windows": ["windows-latest"] }
+          print("::set-output name=python_version::['3.7', '3.8', '3.9']")


still not fixed

galleon · 2021-12-12T19:08:59Z

Indeed the changes for release.yml are targetting forks only (i.e. tests before tagging on main repository).

… for linux The environment is made global for the workflow

dbarbier suggested changes Dec 11, 2021

View reviewed changes

dbarbier reviewed Dec 11, 2021

View reviewed changes

dbarbier suggested changes Dec 12, 2021

View reviewed changes

dbarbier approved these changes Dec 12, 2021

View reviewed changes

dbarbier suggested changes Dec 12, 2021

View reviewed changes

galleon added 2 commits December 12, 2021 20:18

Split the build job in 3 jobs, one for windows, one for macos and one…

6f4b743

… for linux The environment is made global for the workflow

As for build.yml split the build job in three jobs - one per OS

307d54d

dbarbier approved these changes Dec 12, 2021

View reviewed changes

galleon merged commit c868ae7 into airbus:master Dec 13, 2021

galleon deleted the gal/use_parallel_workflows branch December 15, 2021 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use parallel workflows #150

use parallel workflows #150

galleon commented Dec 9, 2021 •

edited

Loading

dbarbier commented Dec 9, 2021 •

edited

Loading

dbarbier Dec 11, 2021

galleon Dec 11, 2021

dbarbier Dec 11, 2021

galleon Dec 12, 2021

dbarbier Dec 11, 2021

galleon Dec 11, 2021

dbarbier Dec 11, 2021

galleon Dec 12, 2021

dbarbier Dec 11, 2021

galleon Dec 11, 2021

dbarbier Dec 11, 2021

galleon Dec 11, 2021

galleon Dec 12, 2021

dbarbier Dec 11, 2021

galleon Dec 11, 2021

dbarbier Dec 11, 2021

galleon Dec 11, 2021

dbarbier Dec 11, 2021 •

edited

Loading

galleon Dec 12, 2021

dbarbier Dec 11, 2021

galleon Dec 11, 2021

dbarbier left a comment

dbarbier Dec 12, 2021

dbarbier Dec 12, 2021

dbarbier Dec 12, 2021

dbarbier Dec 12, 2021

galleon Dec 12, 2021

dbarbier commented Dec 12, 2021

galleon commented Dec 12, 2021

dbarbier commented Dec 12, 2021

dbarbier Dec 12, 2021

galleon commented Dec 12, 2021

use parallel workflows #150

use parallel workflows #150

Conversation

galleon commented Dec 9, 2021 • edited Loading

dbarbier commented Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier Dec 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier commented Dec 12, 2021

galleon commented Dec 12, 2021

dbarbier commented Dec 12, 2021

Choose a reason for hiding this comment

galleon commented Dec 12, 2021

galleon commented Dec 9, 2021 •

edited

Loading

dbarbier commented Dec 9, 2021 •

edited

Loading

dbarbier Dec 11, 2021 •

edited

Loading