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

Use only CUDA devices supported by the SCRAM toolfile #286

Conversation

fwyzard
Copy link

@fwyzard fwyzard commented Mar 14, 2019

Enable CUDA support only if there is at least once CUDA device compatible with the CUDA architectures enabled in SCRAM's cuda.xml toolfile, and restrict CUDAService and its clients to the same list.

@fwyzard
Copy link
Author

fwyzard commented Mar 14, 2019

Fixes #280 .

Copy link

@makortel makortel left a comment

Choose a reason for hiding this comment

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

I think the approach itself is ok, thanks.

HeterogeneousCore/CUDAServices/src/supportedCudaDevices.cu Outdated Show resolved Hide resolved
<< ret << ") " << cudaGetErrorString( ret )
<< ". Running only tests not requiring devices.");
if (deviceCount == 0) {
WARN("No supported CUDA devices available. Running only tests not requiring devices.");
Copy link

@makortel makortel Mar 14, 2019

Choose a reason for hiding this comment

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

Could this be changed to use exitSansCUDADevices()? Or, should that be changed to use supportedCudaDevices() as well?

Copy link
Author

Choose a reason for hiding this comment

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

Good point.

Yes, if we do not build for any of the available devices, we should skip those tests as well.

Choose a reason for hiding this comment

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

What about using the exitSansCUDADevices() here? Or do you prefer to return to that later?

Copy link
Author

Choose a reason for hiding this comment

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

I am not very familiar with how the test harness works, but if I understand the intent, you wanted to run part of the tests even if there are no available devices:

    SECTION("Enabled only if there are CUDA capable GPUs") {
      auto cs = makeCUDAService(ps, ar);
      if(deviceCount <= 0) {
        REQUIRE(cs.enabled() == false);
        WARN("CUDAService is disabled as there are no CUDA GPU devices");
      }
      else {
        REQUIRE(cs.enabled() == true);
        INFO("CUDAService is enabled");
      }
    }

If we call exitSansCUDADevices() the test would never reach that part.

Choose a reason for hiding this comment

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

Right, so I was not able read my own code.

Enable CUDA support only if there is at least once CUDA device
compatible with the CUDA architectures enabled in SCRAM's cuda.xml
toolfile.
Restrict the CUDAService and its clients to use only the devices
supported by the CUDA architectures enabled in SCRAM's cuda.xml
toolfile.
@fwyzard fwyzard force-pushed the Patatrack_use_only_supported_devices branch from f42f3f3 to 68089fb Compare March 14, 2019 20:47
…ADevices

Check for available supported devices also in exitSansCUDADevices.
@fwyzard fwyzard force-pushed the Patatrack_use_only_supported_devices branch from e9e6c7c to fff1f68 Compare March 14, 2019 21:04
@fwyzard
Copy link
Author

fwyzard commented Mar 14, 2019

OK, I've cleaned up a bit the interface.

@makortel does this address all your comments ?

@fwyzard fwyzard closed this Mar 14, 2019
@fwyzard fwyzard reopened this Mar 14, 2019
@makortel
Copy link

+1

@fwyzard fwyzard merged commit 1b21d1c into cms-patatrack:CMSSW_10_5_X_Patatrack Mar 14, 2019
@fwyzard fwyzard deleted the Patatrack_use_only_supported_devices branch March 14, 2019 22:00
felicepantaleo added a commit that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants