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

Don't run tests for non-code changes #898

Merged
merged 18 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 0 additions & 73 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,76 +17,3 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-python@v3
- uses: pre-commit/action@v3.0.1
test:
runs-on: ubuntu-latest
strategy:
matrix:
config: [ {python-version: "3.10", oldest-pymc: false}, {python-version: "3.12", oldest-pymc: true}]
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: ${{ matrix.config.python-version }}
- name: Install oldest version of PyMC
if: ${{ matrix.config.oldest-pymc }}
run: pip install pymc==${{ env.OLDEST_PYMC_VERSION }}
- name: Run tests
run: |
pip install -e .[test]
pytest --cov-report=xml --no-cov-on-fail --durations=50
- name: Check oldest version of PyMC
if: ${{ matrix.config.oldest-pymc }}
run: python -c "import pymc; assert pymc.__version__ == '${{ env.OLDEST_PYMC_VERSION }}'"
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }} # use token for more robust uploads
name: ${{ matrix.config.python-version }}
fail_ci_if_error: false

test_slow:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: "3.10"
- name: Run tests
run: |
pip install -e .[test]
pytest --only-slow --cov-report=xml --no-cov-on-fail --durations=50
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }} # use token for more robust uploads
name: "test_slow"
fail_ci_if_error: false

example_notebooks:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: "3.12"
- name: Install dependencies
run: |
sudo apt-get install graphviz
pip install -e .[docs]
pip install -e .[test]
- name: Run notebooks
run: make run_notebooks


all:
if: ${{ always() }}
runs-on: ubuntu-latest
name: All checks
needs: [ test, test_slow ]
steps:
- name: Confirm checks passed
if: ${{ (needs.test.result != 'success' || needs.test_slow.result != 'success') }}
run: exit 1
71 changes: 71 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
name: Test

on:
pull_request:
branches: [main]
paths: '**.py'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we add *.ipynb to see if a change in a notebook that will break the example_notebooks job will trigger the CI and fail as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about putting that into a separate action? The tests will only need to be run with package changes. The notebooks with package or notebooks themselve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a good idea to separate them out. We would want to trigger the example_noteboooks on any change to an .ipynb or .py.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets give the separation a try

push:
branches: [main]
paths: '**.py'

env:
# The lower bound from the pyproject.toml file
OLDEST_PYMC_VERSION: "$(grep -E 'pymc *>' pyproject.toml | sed -n 's/.*>=\\([0-9]*\\.[0-9]*\\.[0-9]*\\).*/\\1/p')"

jobs:
test:
runs-on: ubuntu-latest
strategy:
matrix:
config: [ {python-version: "3.10", oldest-pymc: false}, {python-version: "3.12", oldest-pymc: true}]
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: ${{ matrix.config.python-version }}
- name: Install oldest version of PyMC
if: ${{ matrix.config.oldest-pymc }}
run: pip install pymc==${{ env.OLDEST_PYMC_VERSION }}
- name: Run tests
run: |
pip install -e .[test]
pytest --cov-report=xml --no-cov-on-fail --durations=50
- name: Check oldest version of PyMC
if: ${{ matrix.config.oldest-pymc }}
run: python -c "import pymc; assert pymc.__version__ == '${{ env.OLDEST_PYMC_VERSION }}'"
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }} # use token for more robust uploads
name: ${{ matrix.config.python-version }}
fail_ci_if_error: false

test_slow:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: "3.10"
- name: Run tests
run: |
pip install -e .[test]
pytest --only-slow --cov-report=xml --no-cov-on-fail --durations=50
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }} # use token for more robust uploads
name: "test_slow"
fail_ci_if_error: false

all:
if: ${{ always() }}
runs-on: ubuntu-latest
name: All checks
needs: [ test, test_slow ]
steps:
- name: Confirm checks passed
if: ${{ (needs.test.result != 'success' || needs.test_slow.result != 'success') }}
run: exit 1
Comment on lines +63 to +71
Copy link
Contributor

@wd60622 wd60622 Aug 1, 2024

Choose a reason for hiding this comment

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

Is this really required? @juanitorduz
Merge is already blocked if any of the tests fail

This seems to be hanging in the github ui as well

Copy link
Collaborator

@juanitorduz juanitorduz Aug 1, 2024

Choose a reason for hiding this comment

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

To be honest, I did not write this part of the CI. I agree it is not bringing much (I hope we are not missing something)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can keep and remove the if: ${{ always() }}

CC: @ricardoV94

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default behavior is exiting if either test or test_slow fail. Right now we have that functionality turned off with fail_ci_if_error: false in ci, so we have to check at the end there. I also don't see why we would want to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Lets remove that to make sure they fail the workflow

Copy link
Contributor

@ricardoV94 ricardoV94 Aug 6, 2024

Choose a reason for hiding this comment

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

The reason for delay failing is all jobs are run instead of stopping when the first one fails.

In my experience this ultimately saves on CI runs because we have a list of all failing tests that can be checked locally (since nobody runs the whole suite locally by default).

Also the first to fail can be the least informative

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Do you have a recommendation on this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Do you have a recommendation on this change?

Nope, didn't check the other changes to know what's the context. Just wanted to shine light on this bit

34 changes: 34 additions & 0 deletions .github/workflows/test_notebook.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
name: Test Notebook

on:
pull_request:
branches: [main]
paths:
- '**.py'
- '**.ipynb'
push:
branches: [main]
paths:
- '**.py'
- '**.ipynb'

env:
# The lower bound from the pyproject.toml file
OLDEST_PYMC_VERSION: "$(grep -E 'pymc *>' pyproject.toml | sed -n 's/.*>=\\([0-9]*\\.[0-9]*\\.[0-9]*\\).*/\\1/p')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont think this is needed in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct it's not.


jobs:
example_notebooks:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v3
with:
python-version: "3.12"
- name: Install dependencies
run: |
sudo apt-get install graphviz
pip install -e .[docs]
pip install -e .[test]
- name: Run notebooks
run: make run_notebooks
Loading