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.

Sign up for GitHub

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

Jump to bottom

Fix notebook tests #675

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Fix notebook tests #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ jobs:
if [ "$RUNNER_OS" == "macOS" ]; then # Avoid numba/OpenMP segfault in CVMDrift (https://github.com/SeldonIO/alibi-detect/issues/648)
export NUMBA_THREADING_LAYER="workqueue"
fi
pytest alibi_detect
pytest --randomly-seed=0 alibi_detect
# Note: The pytest-randomly seed is fixed at 0 for now. Once the legacy np.random.seed(0)'s
# are removed from tests, this can be removed, allowing all tests to use random seeds.

- name: Upload coverage to Codecov
if: ${{ success() }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test_all_notebooks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,4 @@ jobs:

- name: Run notebooks
run: |
pytest --suppress-no-test-exit-code --no-cov -rA --durations=0 -vv testing/test_notebooks.py
pytest --suppress-no-test-exit-code --no-cov -rA --durations=0 -vv -p no:randomly testing/test_notebooks.py
2 changes: 1 addition & 1 deletion .github/workflows/test_changed_notebooks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,4 @@ jobs:
# adding the `or` quantifier between the names and concatenating with the test name `test_notebook_execution`.
run: |
tests="test_notebook_execution[$(echo ${FILES} | sed 's|doc/source/examples/||g' | sed 's| | or |g')]" &&
pytest --suppress-no-test-exit-code --no-cov -rA --durations=0 -vv testing/test_notebooks.py -k "$tests"
pytest --suppress-no-test-exit-code --no-cov -rA --durations=0 -vv -p no:randomly testing/test_notebooks.py -k "$tests"
5 changes: 0 additions & 5 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,8 @@ addopts =
--tb native
-W ignore
--cov=alibi_detect
--randomly-dont-reorganize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we remove this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For --randomly-seed=0, I've moved it to ci.yml because having it in the global pytest config caused an error in the notebook CI because pytest-randomly is now disabled there.

For --randomly-dont-reorganize, I decided we could remove it anyway. This controls whether or not the order of tests is randomly reorganized. From the pytest-randomly docs:

By randomly ordering the tests, the risk of surprising inter-test dependencies is reduced - a technique used in many places, for example Google’s C++ test runner googletest. Research suggests that “dependent tests do exist in practice” and a random order of test executions can effectively detect such dependencies [1].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw we can add --randomly-dont-reorganize to ci.yml if you think this change shouldn't be made without further discussion...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah, I was curious about --randomly-dont-reorganise, it seems useful no? I suppose because we don't really deal with persistent state it's not the type of thing that's going to help us much. Although perhaps it's worth keeping in for the saving tests, that's the only place where I can image one test leaking into another...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh agree that I can't think of any obvious places where tests might be leaky. I hope that the saving ones are ok since they use pytest's tmp_path. I sort of think if we keep pytest_randomly it doesn't hurt to randomise test order, but if we're removing it its not worth implementing an alternative solution to randomise tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing --randomly-dont-reorganise actually enables random order, right? In which case it seems useful to have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, random order has been enabled by removing the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jklaise just to confirm, are you saying random order seems useful to have? i.e. we stick to having the flag removed...

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks. Think this is good to merge then 👍🏻

--randomly-seed=0
#-n auto
#--forked
# randomly-dont-reorganize prevents pytest-randomly from reordering tests
# The pytest-randomly seed is fixed at 0 for now. Once the legacy np.random.seed(0)'s
# are removed from tests, this can be removed, allowing all tests to use random seeds.

[flake8]
max-line-length = 120
Expand Down