From 1c7604c78041cd9b07a91e7db495f994bb113001 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 21 Sep 2022 12:11:05 +0200 Subject: [PATCH 01/14] CI Run inference tests only conditionally As discussed, only run the slow inference API tests on main or when the PR title contains the word "inference". As always, these things are hard to test. --- .github/workflows/build-test.yml | 3 +- .github/workflows/inference-test.yml | 58 ++++++++++++++++++++++++++++ CONTRIBUTING.rst | 7 +++- pyproject.toml | 1 + skops/hub_utils/tests/test_hf_hub.py | 4 +- 5 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/inference-test.yml diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 61522bf5..8a000c0a 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -47,8 +47,9 @@ jobs: - name: Tests env: SUPER_SECRET: ${{ secrets.HF_HUB_TOKEN }} + # skip slow inference tests here, see inference-test.yml run: | - python -m pytest -s -v --cov-report=xml skops/ + python -m pytest -s -v --cov-report=xml -m "not inference" skops/ - name: Mypy run: mypy --config-file pyproject.toml skops diff --git a/.github/workflows/inference-test.yml b/.github/workflows/inference-test.yml new file mode 100644 index 00000000..a3b55ebf --- /dev/null +++ b/.github/workflows/inference-test.yml @@ -0,0 +1,58 @@ +# Tests hitting the inference API are slow and prone to hanging, resulting in +# timeouts. Therefore, we only run them on the main branch or if the PR title +# contains the word "inference". See #118 + +name: pytest + +on: + - push + - pull_request + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + pytest: + + runs-on: ${{ matrix.os }} + if: "(github.repository == 'skops-dev/skops') && ((github.ref == 'refs/heads/main') || contains(github.event.pull_request.title, 'inference'))" + strategy: + fail-fast: false # need to see which ones fail + matrix: + os: [ubuntu-latest, windows-latest, macos-latest] + python: ["3.7", "3.8", "3.9", "3.10.6"] + + # Timeout: https://stackoverflow.com/a/59076067/4521646 + timeout-minutes: 15 + + steps: + - uses: actions/checkout@v3 + - name: Set up Python ${{ matrix.python }} + uses: actions/setup-python@v3 + with: + python-version: ${{ matrix.python }} + + - name: Install dependencies + run: | + pip install .[docs] + python --version + pip --version + pip list + shell: bash + + - name: Inference tests + env: + SUPER_SECRET: ${{ secrets.HF_HUB_TOKEN }} + run: | + python -m pytest -s -v --cov-report=xml -m "inference" skops/ + + - name: Upload coverage to Codecov + uses: codecov/codecov-action@v2 + with: + env_vars: OS,PYTHON + fail_ci_if_error: true + files: ./coverage.xml + flags: unittests + name: codecov-umbrella + verbose: true diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 03bc4a26..306accc8 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -73,7 +73,12 @@ internet with: .. code:: bash - pytest -m "not network" + pytest -m "not network" skops + +Similarly, there is a flag, ``-m inference`` for tests that hit the Hugging Face +Inference API, which can be quite slow or even hang. Skip these tests as long as +you don't make any changes to this functionality. If you already skip network +tests, the inference tests will also be skipped. Releases diff --git a/pyproject.toml b/pyproject.toml index 64a3866b..4a880028 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -13,6 +13,7 @@ filterwarnings = [ ] markers = [ "network: marks tests as requiring internet (deselect with '-m \"not network\"')", + "inference: marks tests that call inference API (deselect with '-m \"not inference\"')", ] addopts = "--cov=skops --cov-report=term-missing --doctest-modules" diff --git a/skops/hub_utils/tests/test_hf_hub.py b/skops/hub_utils/tests/test_hf_hub.py index 362c2664..82df2268 100644 --- a/skops/hub_utils/tests/test_hf_hub.py +++ b/skops/hub_utils/tests/test_hf_hub.py @@ -391,6 +391,7 @@ def repo_path_for_inference(): yield Path(repo_path) +@pytest.mark.inference @pytest.mark.network @flaky(max_runs=3) @pytest.mark.parametrize( @@ -408,7 +409,8 @@ def test_inference( repo_path_for_inference, destination_path, ): - # test inference backend for classifier and regressor models. + # Test inference backend for classifier and regressor models. This test can + # take a lot of time. client = HfApi() repo_path = repo_path_for_inference From 5ab407fa7d53980d33c366808216cb2745ec8336 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 21 Sep 2022 12:15:27 +0200 Subject: [PATCH 02/14] Change name of inference workflow For better differentiation from normal pytest. --- .github/workflows/inference-test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/inference-test.yml b/.github/workflows/inference-test.yml index a3b55ebf..c69ac5e2 100644 --- a/.github/workflows/inference-test.yml +++ b/.github/workflows/inference-test.yml @@ -2,7 +2,7 @@ # timeouts. Therefore, we only run them on the main branch or if the PR title # contains the word "inference". See #118 -name: pytest +name: inference-tests on: - push From e216ba184ef611094227622d8bf0998a53d78d89 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 21 Sep 2022 12:18:00 +0200 Subject: [PATCH 03/14] Fix install for inference tests --- .github/workflows/inference-test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/inference-test.yml b/.github/workflows/inference-test.yml index c69ac5e2..5f7bcb09 100644 --- a/.github/workflows/inference-test.yml +++ b/.github/workflows/inference-test.yml @@ -13,7 +13,7 @@ concurrency: cancel-in-progress: true jobs: - pytest: + inference-pytest: runs-on: ${{ matrix.os }} if: "(github.repository == 'skops-dev/skops') && ((github.ref == 'refs/heads/main') || contains(github.event.pull_request.title, 'inference'))" @@ -35,7 +35,7 @@ jobs: - name: Install dependencies run: | - pip install .[docs] + pip install .[tests] python --version pip --version pip list From 7fa4b76111a652313c46da1ceaad12b3f2c38ab0 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 21 Sep 2022 12:21:18 +0200 Subject: [PATCH 04/14] Docs dependencies are needed to run tests Maybe we should change that... --- .github/workflows/inference-test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/inference-test.yml b/.github/workflows/inference-test.yml index 5f7bcb09..07874791 100644 --- a/.github/workflows/inference-test.yml +++ b/.github/workflows/inference-test.yml @@ -35,7 +35,7 @@ jobs: - name: Install dependencies run: | - pip install .[tests] + pip install .[docs,tests] python --version pip --version pip list From 1d730cc78c843ab10e61d8f3622114cc5aa5e3ef Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Wed, 21 Sep 2022 12:49:06 +0200 Subject: [PATCH 05/14] Empty commit to trigger CI From ab3ccfeb33fbbbba4813e4ba333270f4e51e8037 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Thu, 29 Sep 2022 14:43:08 +0200 Subject: [PATCH 06/14] Reviewer comments - Check commit message instead of PR title - Have one job to run inference tests only for one matrix set - Have one job to run inference tests for full matrix if commit message contains "inference" --- ...est.yml => inference-test-conditional.yml} | 24 +++------- .../inference-test-unconditional.yml | 48 +++++++++++++++++++ 2 files changed, 55 insertions(+), 17 deletions(-) rename .github/workflows/{inference-test.yml => inference-test-conditional.yml} (57%) create mode 100644 .github/workflows/inference-test-unconditional.yml diff --git a/.github/workflows/inference-test.yml b/.github/workflows/inference-test-conditional.yml similarity index 57% rename from .github/workflows/inference-test.yml rename to .github/workflows/inference-test-conditional.yml index 07874791..72db99fe 100644 --- a/.github/workflows/inference-test.yml +++ b/.github/workflows/inference-test-conditional.yml @@ -1,6 +1,6 @@ # Tests hitting the inference API are slow and prone to hanging, resulting in -# timeouts. Therefore, we only run them on the main branch or if the PR title -# contains the word "inference". See #118 +# timeouts. Therefore, we only run inference tests on the full test matrix if +# the head commit message contains the word "inference". See #118 name: inference-tests @@ -13,15 +13,15 @@ concurrency: cancel-in-progress: true jobs: - inference-pytest: + conditional-inference-pytest: runs-on: ${{ matrix.os }} - if: "(github.repository == 'skops-dev/skops') && ((github.ref == 'refs/heads/main') || contains(github.event.pull_request.title, 'inference'))" + if: "(github.repository == 'skops-dev/skops') && contains(github.event.head_commit.message, 'inference')" strategy: fail-fast: false # need to see which ones fail matrix: os: [ubuntu-latest, windows-latest, macos-latest] - python: ["3.7", "3.8", "3.9", "3.10.6"] + python: ["3.7", "3.8", "3.9", "3.10"] # Timeout: https://stackoverflow.com/a/59076067/4521646 timeout-minutes: 15 @@ -41,18 +41,8 @@ jobs: pip list shell: bash - - name: Inference tests + - name: Conditional inference tests env: SUPER_SECRET: ${{ secrets.HF_HUB_TOKEN }} run: | - python -m pytest -s -v --cov-report=xml -m "inference" skops/ - - - name: Upload coverage to Codecov - uses: codecov/codecov-action@v2 - with: - env_vars: OS,PYTHON - fail_ci_if_error: true - files: ./coverage.xml - flags: unittests - name: codecov-umbrella - verbose: true + python -m pytest -s -v -m "inference" skops/ diff --git a/.github/workflows/inference-test-unconditional.yml b/.github/workflows/inference-test-unconditional.yml new file mode 100644 index 00000000..e21a8619 --- /dev/null +++ b/.github/workflows/inference-test-unconditional.yml @@ -0,0 +1,48 @@ +# Tests hitting the inference API are slow and prone to hanging, resulting in +# timeouts. Therefore, we only run a subset of settings with inference tests by +# default. See #118 + +name: inference-tests + +on: + - push + - pull_request + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + inference-pytest: + + runs-on: ${{ matrix.os }} + if: "(github.repository == 'skops-dev/skops')" + strategy: + fail-fast: false # need to see which ones fail + matrix: + os: [ubuntu-latest] + python: ["3.10"] + + # Timeout: https://stackoverflow.com/a/59076067/4521646 + timeout-minutes: 15 + + steps: + - uses: actions/checkout@v3 + - name: Set up Python ${{ matrix.python }} + uses: actions/setup-python@v3 + with: + python-version: ${{ matrix.python }} + + - name: Install dependencies + run: | + pip install .[docs,tests] + python --version + pip --version + pip list + shell: bash + + - name: Unconditional inference tests + env: + SUPER_SECRET: ${{ secrets.HF_HUB_TOKEN }} + run: | + python -m pytest -s -v -m "inference" skops/ From 62a6f757635d270dc445ea21546423f603ce66bb Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Thu, 29 Sep 2022 14:45:30 +0200 Subject: [PATCH 07/14] Rename job for better distinction --- .github/workflows/inference-test-unconditional.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/inference-test-unconditional.yml b/.github/workflows/inference-test-unconditional.yml index e21a8619..dde2742b 100644 --- a/.github/workflows/inference-test-unconditional.yml +++ b/.github/workflows/inference-test-unconditional.yml @@ -13,7 +13,7 @@ concurrency: cancel-in-progress: true jobs: - inference-pytest: + unconditional-inference-pytest: runs-on: ${{ matrix.os }} if: "(github.repository == 'skops-dev/skops')" From e58dbce1e67cc05cc7632824dd20b2a4e0eed99e Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Thu, 29 Sep 2022 15:02:37 +0200 Subject: [PATCH 08/14] Reviewer comments - skip partial test if full test runs - wording: use "full" and "partial" instead of "conditional" and "unconditional" --- ...rence-test-conditional.yml => inference-test-full.yml} | 8 ++++---- ...-test-unconditional.yml => inference-test-partial.yml} | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) rename .github/workflows/{inference-test-conditional.yml => inference-test-full.yml} (86%) rename .github/workflows/{inference-test-unconditional.yml => inference-test-partial.yml} (85%) diff --git a/.github/workflows/inference-test-conditional.yml b/.github/workflows/inference-test-full.yml similarity index 86% rename from .github/workflows/inference-test-conditional.yml rename to .github/workflows/inference-test-full.yml index 72db99fe..a4f9194b 100644 --- a/.github/workflows/inference-test-conditional.yml +++ b/.github/workflows/inference-test-full.yml @@ -1,6 +1,6 @@ # Tests hitting the inference API are slow and prone to hanging, resulting in # timeouts. Therefore, we only run inference tests on the full test matrix if -# the head commit message contains the word "inference". See #118 +# the head commit message contains the text "[CI inference]". See #118 name: inference-tests @@ -13,10 +13,10 @@ concurrency: cancel-in-progress: true jobs: - conditional-inference-pytest: + inference-pytest-full: runs-on: ${{ matrix.os }} - if: "(github.repository == 'skops-dev/skops') && contains(github.event.head_commit.message, 'inference')" + if: "(github.repository == 'skops-dev/skops') && contains(github.event.head_commit.message, '[CI inference]')" strategy: fail-fast: false # need to see which ones fail matrix: @@ -41,7 +41,7 @@ jobs: pip list shell: bash - - name: Conditional inference tests + - name: Full inference tests env: SUPER_SECRET: ${{ secrets.HF_HUB_TOKEN }} run: | diff --git a/.github/workflows/inference-test-unconditional.yml b/.github/workflows/inference-test-partial.yml similarity index 85% rename from .github/workflows/inference-test-unconditional.yml rename to .github/workflows/inference-test-partial.yml index dde2742b..10e6e970 100644 --- a/.github/workflows/inference-test-unconditional.yml +++ b/.github/workflows/inference-test-partial.yml @@ -13,10 +13,10 @@ concurrency: cancel-in-progress: true jobs: - unconditional-inference-pytest: + inference-pytest-partial: runs-on: ${{ matrix.os }} - if: "(github.repository == 'skops-dev/skops')" + if: "(github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]'))" strategy: fail-fast: false # need to see which ones fail matrix: @@ -41,7 +41,7 @@ jobs: pip list shell: bash - - name: Unconditional inference tests + - name: Partial inference tests env: SUPER_SECRET: ${{ secrets.HF_HUB_TOKEN }} run: | From c584da9361f8d5bc0fdb19101e5a2ae044d023a5 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Thu, 29 Sep 2022 15:14:37 +0200 Subject: [PATCH 09/14] Empty commit to trigger [CI inference] From b7018a898ca11f55e3f8beafe2ec05d06e7aa2ec Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Thu, 29 Sep 2022 16:25:07 +0200 Subject: [PATCH 10/14] Use different names for workflows --- .github/workflows/inference-test-full.yml | 2 +- .github/workflows/inference-test-partial.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/inference-test-full.yml b/.github/workflows/inference-test-full.yml index a4f9194b..723ab5e0 100644 --- a/.github/workflows/inference-test-full.yml +++ b/.github/workflows/inference-test-full.yml @@ -2,7 +2,7 @@ # timeouts. Therefore, we only run inference tests on the full test matrix if # the head commit message contains the text "[CI inference]". See #118 -name: inference-tests +name: inference-tests-full on: - push diff --git a/.github/workflows/inference-test-partial.yml b/.github/workflows/inference-test-partial.yml index 10e6e970..e9f23ad2 100644 --- a/.github/workflows/inference-test-partial.yml +++ b/.github/workflows/inference-test-partial.yml @@ -2,7 +2,7 @@ # timeouts. Therefore, we only run a subset of settings with inference tests by # default. See #118 -name: inference-tests +name: inference-tests-partial on: - push From 269053e46bbcdb91525835ebf27ea9b6c5ddf774 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Thu, 29 Sep 2022 17:25:44 +0200 Subject: [PATCH 11/14] Empty commit to trigger [CI inference] From adc42c1499319554742c04cde9cf1fd470bb5a7d Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Fri, 30 Sep 2022 11:42:38 +0200 Subject: [PATCH 12/14] Change expression syntax --- .github/workflows/inference-test-full.yml | 2 +- .github/workflows/inference-test-partial.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/inference-test-full.yml b/.github/workflows/inference-test-full.yml index 723ab5e0..7d48b56b 100644 --- a/.github/workflows/inference-test-full.yml +++ b/.github/workflows/inference-test-full.yml @@ -16,7 +16,7 @@ jobs: inference-pytest-full: runs-on: ${{ matrix.os }} - if: "(github.repository == 'skops-dev/skops') && contains(github.event.head_commit.message, '[CI inference]')" + if: ${{ (github.repository == 'skops-dev/skops') && contains(github.event.head_commit.message, '[CI inference]') }} strategy: fail-fast: false # need to see which ones fail matrix: diff --git a/.github/workflows/inference-test-partial.yml b/.github/workflows/inference-test-partial.yml index e9f23ad2..daadcbfd 100644 --- a/.github/workflows/inference-test-partial.yml +++ b/.github/workflows/inference-test-partial.yml @@ -16,7 +16,7 @@ jobs: inference-pytest-partial: runs-on: ${{ matrix.os }} - if: "(github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]'))" + if: ${{ (github.repository == 'skops-dev/skops') && (!contains(github.event.head_commit.message, '[CI inference]')) }} strategy: fail-fast: false # need to see which ones fail matrix: From 3b58d691eb172e7e59be6158a503ddc903b9b4fb Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Fri, 30 Sep 2022 12:03:31 +0200 Subject: [PATCH 13/14] Empty commit to trigger [CI inference] From 86b0c4d8ee88e2a59582ddd205fb67f83b3e4b97 Mon Sep 17 00:00:00 2001 From: Benjamin Bossan Date: Fri, 30 Sep 2022 12:16:44 +0200 Subject: [PATCH 14/14] Empty commit that should not trigger CI [skip ci]