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

Sanity checks for GPU tests, 3rd party framework tensor conversion changes #646

Merged
merged 10 commits into from
May 2, 2022

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented Apr 25, 2022

  • Add sanity checks to handle cases of hidden GPU devices
  • Allow ...2xp utility methods to accept a target Ops object for conversions

@svlandeg svlandeg added enhancement Feature requests and improvements feat / ops Backends and maths labels Apr 26, 2022
@svlandeg
Copy link
Member

First of two stacked PRs (to be merged with a merge commit; squashing/rebasing will break the 2nd, in-progress PR).

I'm not sure I understand this? If we squash this one, can't you rebase the next PR or merge in develop? (not sure what you mean by "will break")

@shadeMe
Copy link
Collaborator Author

shadeMe commented Apr 26, 2022

Actually, please ignore that last bit - Looks like I misunderstood how stacked PRs work. The original intention was to work on a PR that was dependent on these changes whilst waiting for it to be merged. I was hoping to stack the 2nd PR on this in such a way that I wouldn't have to do any additional merges (i.e., from explosion:develop after this PR is merged), but it seems like it doesn't work when attempting to merge branches between forks.

So, this PR can be squashed/rebased into explosion:develop. I'll just merge that into my local repo and rebase by PR as you suggested.

thinc/util.py Outdated Show resolved Hide resolved
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.

All of these changes feel pretty reasonable to me!

Maybe the only thing I was wondering about is whether the ops argument should be keyword-only. Then again I don't think we'll likely be expanding these methods with many more arguments in the future?

From a practical point of view, I'd love it if Daniël could give this a quick review as well. Considering he's out at the moment, we can either just merge this already (it's to develop anyway) or wait for him to get back - do you have a preference, especially considering the follow-up PR?

@shadeMe
Copy link
Collaborator Author

shadeMe commented Apr 26, 2022

Re. ops - Yeah, it can be keyword-only, and I too doubt that we'll be adding many more arguments - if at all any - in the future. Will change that.

Re. merging - I'm fine with leaving this open until Daniël gets a chance to take a look at it. The other PR doesn't include any urgent fixes, and we still need to fix the Buildkite GPU build environment to actually test the changes in CI. So, it can wait.

thinc/util.py Outdated Show resolved Hide resolved
thinc/util.py Outdated Show resolved Hide resolved
thinc/util.py Outdated Show resolved Hide resolved
thinc/util.py Outdated Show resolved Hide resolved
thinc/util.py Outdated Show resolved Hide resolved
thinc/backends/_custom_kernels.py Outdated Show resolved Hide resolved
Add `Ops` type to `..2xp` functions
Defer `cupy` deprecation-related changes to a different PR
@shadeMe
Copy link
Collaborator Author

shadeMe commented Apr 27, 2022

Hmm, looks like the mypy checks fail when ops: "Ops" is used as the type annotation. Using a fully/partially qualified identifier (thinc.api.Ops or .api.Ops) doesn't fix it either. Any suggestions?

Nevermind.

@shadeMe shadeMe changed the title Sanity checks for GPU tests, cupy deprecation changes, 3rd party framework tensor conversion changes Sanity checks for GPU tests, 3rd party framework tensor conversion changes Apr 27, 2022
@shadeMe shadeMe requested review from svlandeg and danieldk and removed request for danieldk May 2, 2022 12:09
@danieldk
Copy link
Contributor

danieldk commented May 2, 2022

Merging, since @svlandeg also approved, and this is to develop, so we can still fix any issues that might pop up.

@danieldk danieldk merged commit 7a76ca0 into explosion:develop May 2, 2022
@shadeMe shadeMe deleted the refactor/target-ops-2xp-gpu branch May 2, 2022 13:09
shadeMe added a commit to shadeMe/thinc that referenced this pull request May 3, 2022
…anges (explosion#646)

* Add sanity checks to handle cases of hidden GPU devices (`CUDA_VISIBLE_DEVICES=-1`)
Add `has_tensorflow_gpu`

* Allow `...2xp` utility methods to accept a target `Ops` object for conversions
Refactor 3rd party framework GPU tensor detection

* Handle `cupy.fromDlpack` deprecation for `cupy >= 10.0.0`

* Make `ops` arg in `...2xp` functions keyword-only

* Short-circuit `..._gpu_array` functions
Add `Ops` type to `..2xp` functions
Defer `cupy` deprecation-related changes to a different PR

* Fix type annotation

* Defer sanity check in `_custom_kernels.py` to another PR
danieldk pushed a commit to danieldk/thinc that referenced this pull request May 11, 2022
…anges (explosion#646)

* Add sanity checks to handle cases of hidden GPU devices (`CUDA_VISIBLE_DEVICES=-1`)
Add `has_tensorflow_gpu`

* Allow `...2xp` utility methods to accept a target `Ops` object for conversions
Refactor 3rd party framework GPU tensor detection

* Handle `cupy.fromDlpack` deprecation for `cupy >= 10.0.0`

* Make `ops` arg in `...2xp` functions keyword-only

* Short-circuit `..._gpu_array` functions
Add `Ops` type to `..2xp` functions
Defer `cupy` deprecation-related changes to a different PR

* Fix type annotation

* Defer sanity check in `_custom_kernels.py` to another PR
shadeMe added a commit to shadeMe/thinc that referenced this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements feat / ops Backends and maths
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants