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

Force cuda since torch ask for a device, not if cuda is in fact avail… #15

Merged
merged 3 commits into from Dec 17, 2021

Conversation

ccoulombe
Copy link
Contributor

Force cuda since torch ask for a device, not if cuda is in fact available.

torch.cuda.is_available() is misleading since it actually checks for an available device, not if cuda is available.
Building on build-nodes where a gpu device is not necessarily available then fails. Force cuda through FORCE_CUDA=1 to allow building.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2021
@bowenc0221
Copy link
Contributor

Thanks for the PR, two questions:

  1. FORCE_CUDA is not always set in environment variable, need to provide it with a default value
  2. Do we still need CUDA_HOME when using FORCE_CUDA=1?

Maybe let's follow the same logic as detectron2 (and remove is_rocm_pytorch):
https://github.com/facebookresearch/detectron2/blob/9176db667837c57e7118d92a90f887565bd6d4c1/setup.py#L66-L68

@ccoulombe
Copy link
Contributor Author

ccoulombe commented Dec 16, 2021

  1. os.environ.get('FORCE_CUDA') returns None which evaluate to False when the env. variable is not set. So it not necessary to provide a different default value.
>>> import os
>>> os.environ['FORCE_CUDA'] = '1'
>>> bool(os.environ.get('FORCE_CUDA'))
True
>>> del os.environ['FORCE_CUDA']
>>> bool(os.environ.get('FORCE_CUDA'))
False
  1. And yes, CUDA_HOME needs to be set and exists in order to build the code with GPU support.

The current code is equivalent to what detectron2 does

@bowenc0221
Copy link
Contributor

Hi @ccoulombe, I've tested the PR and it works well. Please modify the error message to reflect your change as well (pls see my comment) and I will merge this PR. Thanks!

@ccoulombe
Copy link
Contributor Author

ccoulombe commented Dec 16, 2021 via email

@bowenc0221 bowenc0221 merged commit 31aba60 into facebookresearch:main Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants