Add Test strategy #688
Add Test strategy #688
Conversation
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
=======================================
- Coverage 95% 73% -22%
=======================================
Files 166 166
Lines 6413 6413
=======================================
- Hits 6105 4684 -1421
- Misses 308 1729 +1421 |
|
well seems that some tests at the beginning is hanging, have you tried to run it locally? |
… into feature/test-strategy
| @@ -55,22 +55,29 @@ def sk_auc(x, y, reorder=False): | |||
|
|
|||
| @pytest.mark.parametrize("x, y", _examples) | |||
| class TestAUC(MetricTester): | |||
| @pytest.mark.parametrize("ddp", [False]) | |||
| @pytest.mark.parametrize(MetricTesterDDPCases.name_strategy(), MetricTesterDDPCases.cases_strategy()) | |||
There was a problem hiding this comment.
DDP apparently fails and therefore was probably excluded in the tests, yet there is no mention in the docs, that DDP is not supported.
There was a problem hiding this comment.
@Borda @SkafteNicki any idea how to handle this? removing the full ddp/device strategy and opening an issue to fix auc would be my recommendation
There was a problem hiding this comment.
Its hard for me to remember, but basically AUC needs ordered input to be working and while we are testing on ordered input when we move to ddp it somehow gets unordered. I wonder if we should just set the reorder flag to True whenever ddp=True.
twsl
requested review from
ananyahjha93,
Borda,
ethanwharris,
justusschock,
SeanNaren and
tchaton
as code owners
There was a problem hiding this comment.
hi, thx for this PR, just very sure if it is really needed... what you try to set here is running CPU and GPU in a single test call, but even now we are trying to optimize the test to run as efficiently as possible, so on CPU machine run just CPU version and on GPU run CUDA version (and skip CPU version as it was run already elsewhere)
Said so I think that is also by user expectation, test on the best available resources, so on GPU machine run CUDA tests and on CPU imagine the rest... so if you have a case that you have GPU and you want to run CPU test you can simply disable GPU visibility for the particular run, for example: CUDA_VISIBLE_DEVICES=-1 pytest ...
| @@ -75,25 +75,28 @@ def average_metric(preds, target, metric_func): | |||
| class TestPESQ(MetricTester): | |||
| atol = 1e-2 | |||
|
|
|||
| @pytest.mark.parametrize("ddp", [True, False]) | |||
| @pytest.mark.parametrize(MetricTesterDDPCases.name_strategy(), MetricTesterDDPCases.cases_strategy()) | |||
There was a problem hiding this comment.
I would say it is cleaner if it will be a constant or function returning tuple:
def ddp_name_options():
return "ddp", (True, False)and later:
@pytest.mark.parametrize(*ddp_name_options())There was a problem hiding this comment.
The reason I added this stategy was that now all available test cases are generated and you can explicitly test them locally. This way you can guarantee full coverage, don't depend on the ci machines or some launch flags and each test states on which device it runs. This way you dont have to check the machine stats to e.g. find out, if a test failed on single or multi gpu.
In order to optimize the tests, having an additional skip condition with an env var could achieve the same. Especially for test optimization you could then run ddp=false on a machine with a single gpu and ddp=True on one with multiple.
I'm fine putting it into a single function, my idea was to stick with two parameter style of parametrize
There was a problem hiding this comment.
I agree with @Borda that it would be better to return a tuple there.
For the general purpose: I think we should split this between arguments for every test (like devices and maybe also ddp strategies) for which we should use pytest fixtures (that's what they are there for) and then you could call pytest --devices=XX tests. For test-specific arguments this approach is fine IMO
There was a problem hiding this comment.
I'm unsure how to continue with this. Any suggestions?
Borda
force-pushed
the
master
branch
3 times, most recently
from
3a0f7dc
to
317182c
Compare
|
@SkafteNicki , I think it makes sense to merge this first before merging #867 to make sure things continue working correctly on GPUs. |
|
Thank you for your suggestion, but atm we are in the process of additional test optimization 😇 |
What does this PR do?
Fixes #675
This is just a proposal as I don't have multiple gpus for testing.
This increases the number of tests a lot!
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃