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

install imagecodecs and openslide-python dependencies so additional tests will run #634

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Nov 17, 2023

This MR may initially fail during wheel testing due to #626, as a few test cases relying on these packages were not being run at the time #619 was merged.

@grlee77 grlee77 added improvement Improves an existing functionality non-breaking Introduces a non-breaking change maintenance labels Nov 17, 2023
@grlee77 grlee77 added this to the v23.12.00 milestone Nov 17, 2023
@grlee77 grlee77 self-assigned this Nov 17, 2023
@grlee77 grlee77 requested review from a team as code owners November 17, 2023 01:50
@grlee77 grlee77 force-pushed the grelee/install-additional-test-case-dependencies branch from 1a9988f to ccaa73c Compare November 17, 2023 13:37
ci/test_wheel.sh Outdated Show resolved Hide resolved
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 18, 2023

Okay, libopenslide is now being found and the test_converter case passes. There is a single test failure corresponding to #626 and for some reason also a segfault upon pytest exit. This is the end of the log

FAILED python/cucim/tests/unit/clara/test_image_cache.py::test_cache_hit_miss - assert (0, 0) == (0, 1)
  At index 1 diff: 0 != 1
  Full diff:
  - (0, 1)
  ?     ^
  + (0, 0)
  ?     ^
==== 1 failed, 9665 passed, 42 skipped, 127 warnings in 2718.67s (0:45:18) =====
pure virtual method called
terminate called without an active exception
ci/test_wheel.sh: line 34:  1078 Aborted                 (core dumped) python -m pytest ./python/cucim
Error: Process completed with exit code 134.

@jakirkham
Copy link
Member

^ @gigony

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Greg! 🙏

Had a follow up question below (assuming this change works)

ci/test_wheel.sh Outdated
Comment on lines 20 to 23
# TODO: fix and remove two test cases skipped here
# test_cache_hit_miss currently fails
# test_converter passes, but a segfault occurs on pytest exit when it is enabled
python -m pytest ./python/cucim -k "not test_converter and not test_cache_hit_miss"
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to move these to PyTest decorators?

We could also use skipif to more carefully restrict when they are skipped

@pytest.mark.skip(reason="currently fails")
def test_cache_hit_miss():
    ...
 
@pytest.mark.skip(reason="passes, but a segfault occurs on pytest exit when it is enabled")
def test_converter():
    ...

@jakirkham
Copy link
Member

After a bit of effort Gigon was able to reproduce this when we met yesterday using a Ubuntu 18 Docker container and installing the wheel. With a bit of debugging it appears this goes back to an old cuFile issue that we thought had been fixed with PR ( #158 ). However it appears this issue is back

@jakirkham
Copy link
Member

After further discussion offline, it sounds like the cuFile bug persists in CUDA 11. So we are opting to skip related tests on CUDA 11

This problem does not occur in CUDA 12. So we run all test there

@jakirkham jakirkham changed the base branch from branch-23.12 to branch-24.02 December 1, 2023 23:41
@jakirkham jakirkham modified the milestones: v23.12.00, v24.02.00 Dec 1, 2023
@jakirkham
Copy link
Member

After discussion with Greg offline, we decided to move this to 24.02 as we are already in code freeze for 23.12

@jakirkham
Copy link
Member

Style check failure is unrelated. It is coming from the version update script. Attempting to address in PR ( #648 )

@jakirkham jakirkham requested a review from gigony December 4, 2023 19:51
Copy link
Contributor

@gigony gigony left a comment

Choose a reason for hiding this comment

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

Thanks @grlee77 ! Looks good to me!

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 69c0099 into rapidsai:branch-24.02 Dec 4, 2023
39 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality maintenance non-breaking Introduces a non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants