fix skip in test_core when no GPU available

[NimaSarajpoor]

After moving the two functions _gpu_searchsorted_left and _gpu_searchsorted_right to core.py in PR #714 , I modified test_core.py. However, my modification resulted in skipping the whole unit test test_core.py when there is no GPU.

This PR fixes this issue.

[codecov-commenter]

Codecov Report

Base: 99.58% // Head: 99.58% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (2d2f5a6) compared to base (73622f2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
- Coverage   99.58%   99.58%   -0.01%     
==========================================
  Files          81       81              
  Lines       12742    12741       -1     
==========================================
- Hits        12689    12688       -1     
  Misses         53       53              

Impacted Files	Coverage Δ
stumpy/__init__.py	100.00% <ø> (ø)
tests/test_core.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[NimaSarajpoor]

@seanlaw
I was wondering if we could generate report that shows the changes in the number of skipped tests. If we have something similar to coverage report but for skipped tests, then we could have caught this earlier.

[seanlaw]

Hmm, perhaps, I think we should move the functions into gpu_core.py. This way, indeed, the whole file can be skipped and we NEVER put any GPU stuff in core.py. Would that be a reasonable alternative? I think keeping CPU code separate from GPU code will make things much easier to maintain

I was wondering if we could generate report that shows the changes in the number of skipped tests. If we have something similar to coverage report but for skipped tests, then we could have caught this earlier.

I don't see an easy way to know what the difference in the number of skipped tests would be without re-running all of the before and after tests. Hmmm....

[NimaSarajpoor]

Hmm, perhaps, I think we should move the functions into gpu_core.py. This way, indeed, the whole file can be skipped and we NEVER put any GPU stuff in core.py. Would that be a reasonable alternative? I think keeping CPU code separate from GPU code will make things much easier to maintain

I totally agree with this! I will create gpu_core.py.

I don't see an easy way to know what the difference in the number of skipped tests would be without re-running all of the before and after tests. Hmmm....

If I find something regarding this matter, I will share it with you.

[seanlaw]

pytest.skip("Skipping Tests No GPUs Available", allow_module_level=True)

I probably should've paid closer attention to this when I was reviewing but allow_module_level=True skips the entire module (i.e., all tests in the .py file). It is okay to skip single tests within a test file by omitting allow_module_level=True:

https://github.com/TDAmeritrade/stumpy/blob/ac238b61897d53621f8e4607f88c7517f77ecf52/tests/test_non_normalized_decorator.py#L431-L435

I see that you've fixed that now in the latest commit.

However, now that we've fixed that, instead of splitting it off into core.py as I've proposed, let's keep the GPU functions where they are in core.py and simply ensure that we only deal with skipping at the local function level (i.e., remove allow_module_level=True).

=====

I understand what you are trying to do here but this section seems out of place within the test file:

if cuda.is_available():
    from stumpy.core import _gpu_searchsorted_left, _gpu_searchsorted_right

    @cuda.jit("(f8[:, :], f8[:], i8[:], i8, b1, i8[:])")
    def _gpu_searchsorted_kernel(a, v, bfs, nlevel, is_left, idx):
        # A wrapper kernel for calling device function _gpu_searchsorted_left/right.
        i = cuda.grid(1)
        if i < a.shape[0]:
            if is_left:
                idx[i] = _gpu_searchsorted_left(a[i], v[i], bfs, nlevel)
            else:
                idx[i] = _gpu_searchsorted_right(a[i], v[i], bfs, nlevel)

else:  # pragma: no cover
    from stumpy.core import (
        _gpu_searchsorted_left_driver_not_found as _gpu_searchsorted_left,
    )
    from stumpy.core import (
        _gpu_searchsorted_right_driver_not_found as _gpu_searchsorted_right,
    )

Specifically, the imports using if/else looks weird. Maybe we can simplify things by moving this logic into stumpy/__init__.py:

https://github.com/TDAmeritrade/stumpy/blob/ac238b61897d53621f8e4607f88c7517f77ecf52/stumpy/__init__.py#L32-L51

Then, you can simply do:

from stumpy import core, config, _gpu_searchsorted_left, _gpu_searchsorted_right

if cuda.is_available():
    @cuda.jit("(f8[:, :], f8[:], i8[:], i8, b1, i8[:])")
    def _gpu_searchsorted_kernel(a, v, bfs, nlevel, is_left, idx):
        # A wrapper kernel for calling device function _gpu_searchsorted_left/right.
        i = cuda.grid(1)
        if i < a.shape[0]:
            if is_left:
                idx[i] = _gpu_searchsorted_left(a[i], v[i], bfs, nlevel)
            else:
                idx[i] = _gpu_searchsorted_right(a[i], v[i], bfs, nlevel)


@pytest.mark.filterwarnings("ignore", category=NumbaPerformanceWarning)
@patch("stumpy.config.STUMPY_THREADS_PER_BLOCK", TEST_THREADS_PER_BLOCK)
def test_gpu_searchsorted():
    if not cuda.is_available():  # pragma: no cover
        pytest.skip("Skipping Tests No GPUs Available")

    .
    .
    .
    _gpu_searchsorted_kernel[blocks_per_grid, threads_per_block](
            device_A, device_V, device_bfs, nlevel, is_left, device_comp_IDX
        )

=====

Unrelated: What is concerning is that our reporting didn't explicitly tell us that the entire test_core.py was skipped:

========================================= test session starts =========================================
platform darwin -- Python 3.10.8, pytest-7.2.0, pluggy-1.0.0
rootdir: /Users/slaw/Git/stumpy.git, configfile: pytest.ini
plugins: check-links-0.7.1, cov-4.0.0, anyio-3.6.2
collected 0 items / 1 skipped

======================================= short test summary info =======================================
SKIPPED [1] tests/test_gpu_stump.py:19: Skipping Tests No GPUs Available
========================================= 1 skipped in 0.74s ==========================================
========================================= test session starts =========================================
platform darwin -- Python 3.10.8, pytest-7.2.0, pluggy-1.0.0
rootdir: /Users/slaw/Git/stumpy.git, configfile: pytest.ini
plugins: check-links-0.7.1, cov-4.0.0, anyio-3.6.2
collected 0 items / 1 skipped

So, I've changed the pytest reporting in test.sh (see commit 92e4100) and updated the flags to py.test -rsx, which will show extra summaries (-r) on skipped tests (-s) and failed tests (-x). It's still now what you were asking for but at least we would've seen that test_core.py was skipped

[NimaSarajpoor]

@seanlaw
Thank you for your thorough explanation!

However, now that we've fixed that, instead of splitting it off into core.py as I've proposed, let's keep the GPU functions where they are in core.py and simply ensure that we only deal with skipping at the local function level (i.e., remove allow_module_level=True).

Sure. I will keep it.

I think you are trying to do some sort of trade-off in your mind to see if it is worth it to put a few gpu-related code to a separate module.

Specifically, the imports using if/else looks weird. Maybe we can simplify things by moving this logic into stumpy/__init__.py:

Thank you for the suggestion! I've moved the if-else logic to the __init__.py.

So, I've changed the pytest reporting in test.sh (see commit 92e4100) and updated the flags to py.test -rsx, which will show extra summaries (-r) on skipped tests (-s) and failed tests (-x). It's still now what you were asking for but at least we would've seen that test_core.py was skipped

I merged the changes and ran tests locally. The pytest result's report is good 👍

---

A couple of notes:

1. Please wait till I test modules in my other PC that has GPU so we can make sure everything is fine.

2. Could you please checkout the changed files to see if they make sense or not?

[seanlaw]

Please wait till I test modules in my other PC that has GPU so we can make sure everything is fine.

Sounds good

I think you are trying to do some sort of trade-off in your mind to see if it is worth it to put a few gpu-related code to a separate module.

Yeah, I forgot that we had a mixture in test_non_normalized and that it was possible. Adding a gpu_core.py seems like a bit much at this stage with only two functions. Keeping it in core.py for now seems "better" until we amass more core-like GPU functions (i.e., 5-10+ core GPU functions).

Could you please checkout the changed files to see if they make sense or not?

I will find some time to do that

[seanlaw]

It looks cleaner already!

[NimaSarajpoor]

@seanlaw
I tested gpu_stump and gpu_aamp, and their tests functions are all passing. I also removed an unnecessary import of functions (see last commit).

So, please let me know if there is anything left to do. If we are good here, feel free to merge this after the test suite finishes.

[seanlaw]

Thanks @NimaSarajpoor!