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

test: Make fail_backend markers add pytest.mark.xfail and remove fail_jax marker on percentile tests #1730

Merged
merged 6 commits into from
Dec 9, 2021

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Dec 9, 2021

Description

Resolves #1729

To get xfail_strict = true to work properly in pyproject.toml it needs to be located under [tool.pytest.ini_options] as is noted in the pytest docs:

One might wonder why [tool.pytest.ini_options] instead of [tool.pytest] as is the case with other tools.

The reason is that the pytest team intends to fully utilize the rich TOML data format for configuration in the future, reserving the [tool.pytest] table for that. The ini_options table is being used, for now, as a bridge between the existing .ini configuration system and the future configuration format.

Once that has been moved then to deal with the issue of pytest.xfail skipping tests instead of running them and checking results like pytest.mark.xfail (c.f. Issue #1729 for discussion on this), add pytest.mark.xfail to the request node for the backend extra that has the fail_{backend} marker already placed on it. This marker placing ensures that placing a fail_backend marker on a test will fail the test if that backend does NOT fail — which was the point of PR #1702.

With that working, the fail_jax markers can be removed from the percentile tests in test_tensor.py and remove the only_jax tests added for them now that jax v0.2.26 is out and brings with it a fix for jax-ml/jax#8513. Also, remove the fail_pytorch marker for test_ND_gather (thanks to it being found by the now working xfail strict).

An update to the lower bounds for the jax extra in setup.py

pyhf/setup.py

Line 10 in 43c1567

'jax': ['jax>=0.2.10', 'jaxlib>=0.1.60,!=0.1.68'], # c.f. Issue 1501

and the lower bound constraints in

# jax
# Use Google Cloud Storage buckets for long term wheel support
# c.f. https://github.com/google/jax/discussions/7608#discussioncomment-1269342
--find-links https://storage.googleapis.com/jax-releases/jax_releases.html
jax==0.2.10
jaxlib==0.1.60 # jax v0.2.10 requires jaxlib>=0.1.60

is not needed as (interestingly) these lower bounds are old enough to have the desired behavior that the lastest versions have (again), and so can be left unchanged. So for the time being the scipy lower bound increase that is currently in jax won't pose an issue for the constraints.txt file installs for a while (c.f. jax-ml/jax#8871).

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Move `xfail_strict = true` into [tool.pytest.ini_options] to have it
be picked up by pytest properly.
   - c.f. https://docs.pytest.org/en/6.2.x/customize.html#pyproject-toml
   - Amends PR #1702
* Edit the conftest.py `backend` fixture to mark test nodes that have
`@pytest.mark.fail_{backend}` markers (e.g. fail_jax) with
pytest.mark.xfail to ensure that the test are actually run and checked
to make sure that they don't pass when when they are expected to fail.
This is necessary as using pytest.xfail actually skips the test as it
is assumed that the test is known to fail for reasonable reasons, and
pytest.mark.xfail is intended to be the way to check fail status.
c.f.:
   - https://docs.pytest.org/en/6.2.x/skipping.html#xfail
   - https://docs.pytest.org/en/6.2.x/reference.html#pytest-xfail
   - https://docs.pytest.org/en/6.2.x/reference.html#pytest-mark-xfail-ref
* Remove fail_jax marker from percentile tests and remove only_jax
tests as no longer needed given jax v0.2.26 addresses the problems
they were added to work around.
* Remove fail_pytorch marker from test_ND_gather as the test is now
passing, causing an error given xfail_strict.

@matthewfeickert matthewfeickert added tests pytest fix A bug fix labels Dec 9, 2021
@matthewfeickert matthewfeickert self-assigned this Dec 9, 2021
@matthewfeickert matthewfeickert added the docs Documentation related label Dec 9, 2021
@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #1730 (a05f91d) into master (eb45d60) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1730   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files          64       64           
  Lines        4270     4270           
  Branches      683      683           
=======================================
  Hits         4190     4190           
  Misses         46       46           
  Partials       34       34           
Flag Coverage Δ
contrib 26.25% <ø> (ø)
doctest 60.58% <ø> (ø)
unittests 96.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/tensor/jax_backend.py 98.58% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb45d60...a05f91d. Read the comment docs.

@matthewfeickert matthewfeickert changed the title tests: Use pytest.mark.xfail over pytest.xfail and remove fail_jax marker on percentile tests test: Use pytest.mark.xfail over pytest.xfail and remove fail_jax marker on percentile tests Dec 9, 2021
@matthewfeickert matthewfeickert changed the title test: Use pytest.mark.xfail over pytest.xfail and remove fail_jax marker on percentile tests test: Make fail_backend markers equivalent to pytest.mark.xfail and remove fail_jax marker on percentile tests Dec 9, 2021
@matthewfeickert matthewfeickert changed the title test: Make fail_backend markers equivalent to pytest.mark.xfail and remove fail_jax marker on percentile tests test: Make fail_backend markers add pytest.mark.xfail and remove fail_jax marker on percentile tests Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related fix A bug fix tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of xfail in conftest.py hiding passing conditions
2 participants