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

Tests: Correctly handle GPU-resident Tensorflow tensors #653

Merged
merged 2 commits into from
May 6, 2022

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented May 5, 2022

Tensorflow automatically allocates on the GPU if possible, which is something the tests below didn't correctly account for. Since the CI host machine's CUDA libraries were not correctly configured, TF silently always used the CPU thereby hiding this bug.

@shadeMe shadeMe requested review from danieldk and svlandeg May 5, 2022 17:14
@shadeMe shadeMe added bug Bugs and behaviour differing from documentation tests Tests and test suite interop / tensorflow TensorFlow interoperability labels May 5, 2022
@shadeMe
Copy link
Collaborator Author

shadeMe commented May 5, 2022

@explosion-bot please test_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented May 5, 2022

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/thinc-gpu-test-suite/builds/61

Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Missed a possible simplification in my first review.

thinc/tests/util.py Outdated Show resolved Hide resolved
@danieldk danieldk self-requested a review May 6, 2022 08:28
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

🎉

@danieldk danieldk merged commit a7916c8 into explosion:develop May 6, 2022
@shadeMe shadeMe deleted the fix/tensorflow-gpu-tests branch May 6, 2022 13:45
danieldk pushed a commit to danieldk/thinc that referenced this pull request May 11, 2022
* Tests: Correctly handle GPU-resident Tensorflow tensors

* Simplify `is_supported_backend_array`
danieldk pushed a commit to danieldk/thinc that referenced this pull request May 11, 2022
* Tests: Correctly handle GPU-resident Tensorflow tensors

* Simplify `is_supported_backend_array`
shadeMe added a commit to shadeMe/thinc that referenced this pull request May 11, 2022
* Tests: Correctly handle GPU-resident Tensorflow tensors

* Simplify `is_supported_backend_array`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and behaviour differing from documentation interop / tensorflow TensorFlow interoperability tests Tests and test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants