Skip to content

Fix CUDA output tensor for metric learning model - #94

Open
Baucki1 wants to merge 6 commits into
key4hep:mainfrom
Baucki1:main
Open

Fix CUDA output tensor for metric learning model#94
Baucki1 wants to merge 6 commits into
key4hep:mainfrom
Baucki1:main

Conversation

@Baucki1

@Baucki1 Baucki1 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

BEGINRELEASENOTES

  • Small change to ONNXInferenceModel adding a dedicated CUDA path, binding ONNX outputs directly to CUDA device
  • Small fix to OnnxMetricLearning to ensure output tensor is on the correct device for buildEdgesFRNN (ACTS) when running on CUDA
  • Copy node feature input tensor for downstream pipeline (edge classifiers) directly into ACTS tensor on CUDA device memory to force ACTS' OnnxEdgeClassifier to run on CUDA (device is picked up from input tensor); skips intermediate conversion to torch tensor to move to CUDA device (std::vector -> torchTensor -> ActsTensor)

ENDRELEASENOTES

@Baucki1
Baucki1 marked this pull request as ready for review September 8, 2026 10:55
Comment on lines +213 to +214
[[nodiscard]] std::vector<Ort::Value> runInference(const T& inputData, bool runOnCuda = false,
std::size_t cudaDeviceIndex = 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, this is rather late in the whole process. Could we tie runOnCuda to the cudaDeviceIndex, e.g. by using a negative index as a false-y value for that? Or do other inference wrappers generally also do a flag and an index?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, we can do that! I checked again how this is done in ACTS.
In ACTS the device is just picked up from e.g. the tensor argument tensor.device (device.type, device.index, etc.)
Maybe something similar would work here, for example passing a device as single argument to runInference: this would require includes from ACTS in ONNXInferenceModel or overloading the method, e.g. like

std::vector<Ort::Value> ONNXInferenceModel::runInference(const T& inputData, const DeviceT& device) {
  return runInference(inputData, device.isCuda(), static_cast<std::size_t>(device.index));
}

Or instead a combination (cudaDeviceIndex as int, where <0 means CPU):

std::vector<Ort::Value> ONNXInferenceModel::runInference(const T& inputData, const DeviceT& device) {
  const int cudaDeviceIndex = device.isCuda() ? static_cast<int>(device.index) : -1;
  return runInference(inputData, cudaDeviceIndex);
}

Let me know what you think.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think at least for now, we could just include the ACTS header in the ONNXInferenceModel. That would make the whole inference a bit harder to lift out of here, but in the end, that should be a fairly small dependency to remove if we ever get to a more generic solution.

@tmadlener tmadlener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The formatting that pre-commit complains about is pre-existing (due to the switch to LLVM 22 for the nightlies stack). (see e.g. #96)

@jmcarcell is there anything we can do to the .clang-format to retain the LLVM 20 behavior? Otherwise we will have quite some formatting in the next few days / weeks.

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