feat: Allow for disabling backend when running tests via pytest #2340
Conversation
kratsg
added
feat/enhancement
kratsg
requested review from
alexander-held,
lukasheinrich and
matthewfeickert
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2340 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 69 69
Lines 4538 4538
Branches 803 803
=======================================
Hits 4460 4460
Misses 45 45
Partials 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
matthewfeickert
force-pushed
the
feat/disableBackendInTests
branch
from
6981aa6 to
1aa0e0c
Compare
There was a problem hiding this comment.
@kratsg I have a minor suggestion for the infomration that gets printed to the user in the short test summary info for the skip, but other than that LGTM!
matthewfeickert
force-pushed
the
feat/disableBackendInTests
branch
from
1aa0e0c to
e4d0290
Compare
matthewfeickert
force-pushed
the
feat/disableBackendInTests
branch
from
e4d0290 to
079d130
Compare
There was a problem hiding this comment.
@kratsg My apologies. Though I've been rebasing this PR I didn't notice until tonight that you had responded to it 😬 — my bad.
If you don't like these suggested edits then we can merge this.
| action="append", | ||
| type=str, | ||
| default=[], | ||
| choices=["tensorflow", "pytorch", "jax", "minuit"], |
There was a problem hiding this comment.
| choices=["tensorflow", "pytorch", "jax", "minuit"], | |
| choices=["numpy", "tensorflow", "pytorch", "jax", "minuit"], |
Is there a reason to not include numpy?
There was a problem hiding this comment.
Numpy is a core dependency, not an optional one.
There was a problem hiding this comment.
Yes, but I would think that if I have the ability to disable backends during testing that I should be able to disable any of them, not just optional ones.
There was a problem hiding this comment.
The point of the issue was to only disable optional ones.
That was the original case yes, but is there any technical reason to not want to also be able to skip numpy?
There was a problem hiding this comment.
I don't want to delay this anymore so I'll merge this in and open an Issue.
Co-authored-by: Matthew Feickert <[email protected]>
|
The point of the issue was to only disable optional ones.
…On Thu, Oct 5, 2023 at 07:51 Matthew Feickert ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/conftest.py
<#2340 (comment)>:
> @@ -9,6 +9,17 @@
import pyhf
+def pytest_addoption(parser):
+ parser.addoption(
+ "--disable-backend",
+ action="append",
+ type=str,
+ default=[],
+ choices=["tensorflow", "pytorch", "jax", "minuit"],
Yes, but I would think that if I have the ability to disable backends
during testing that I should be able to disable any of them, not just
optional ones.
—
Reply to this email directly, view it on GitHub
<#2340 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFZ5C7RHQQ2CFHONT7O7TTX53CODAVCNFSM6AAAAAA5AJ24JSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRQGA3TSNBQGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Pull Request Description
Resolves #1454. This adds
--disable-backendthat can be specified to skip those backends as part of the testing suite.Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: