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

Add StreamID parameter to Transformer callback functions #47036

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Jan 2, 2025

PR description:

This PR adds edm::StreamID parameter to the callback functions for the Transformer EDModule ability. Knowing the StreamID would be useful in implementing implicit host-to-device data product copies for EDProducers (in addition to their implicit device-to-host copies).

Resolves cms-sw/framework-team#1130

PR validation:

Framework unit tests run.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 2, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 2, 2025

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47036/43159

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 2, 2025

A new Pull Request was created by @makortel for master.

It involves the following packages:

  • FWCore/Framework (core)
  • FWCore/Integration (core)
  • HeterogeneousCore/AlpakaCore (heterogeneous)

@Dr15Jones, @cmsbuild, @fwyzard, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@missirol, @rovere, @wddgit this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

makortel commented Jan 2, 2025

enable gpu

@makortel
Copy link
Contributor Author

makortel commented Jan 2, 2025

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 3, 2025

-1

Failed Tests: ClangBuild
Size: This PR adds an extra 76KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d134de/43617/summary.html
COMMIT: 935bb55
CMSSW: CMSSW_15_0_X_2025-01-02-2300/el8_amd64_gcc12
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47036/43617/install.sh to create a dev area with all the needed externals and cmssw changes.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' /usr/bin/time -v scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

The StreamID will be needed to implement implicit host-to-device
copies for Alpaka EDProducers
@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 3, 2025

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47036/43161

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 3, 2025

Pull request #47036 was updated. @Dr15Jones, @cmsbuild, @fwyzard, @makortel, @smuzaffar can you please check and sign again.

@makortel
Copy link
Contributor Author

makortel commented Jan 3, 2025

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 3, 2025

-1

Failed Tests: GpuUnitTests
Size: This PR adds an extra 32KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d134de/43619/summary.html
COMMIT: 0e28aeb
CMSSW: CMSSW_15_0_X_2025-01-03-1100/el8_amd64_gcc12
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47036/43619/install.sh to create a dev area with all the needed externals and cmssw changes.

GPU Unit Tests

I found 2 errors in the following unit tests:

---> test testCudaDeviceAdditionWrapper had ERRORS
---> test testCudaDeviceAdditionKernel had ERRORS

Comparison Summary

Summary:

GPU Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 7
  • DQMHistoTests: Total histograms compared: 53071
  • DQMHistoTests: Total failures: 381
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 52690
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 6 files compared)
  • Checked 24 log files, 30 edm output root files, 7 DQM output files
  • TriggerResults: no differences found

@fwyzard
Copy link
Contributor

fwyzard commented Jan 4, 2025

ignore tests-rejected with ib-failure

@fwyzard
Copy link
Contributor

fwyzard commented Jan 4, 2025

+heterogeneous

@Dr15Jones
Copy link
Contributor

The code seems fine. The only downside I see is that this interface would not work if we were to extend this system to also work with Runs and LuminosityBlocks. I think it is fine to not consider that until such a change is needed.

@makortel
Copy link
Contributor Author

makortel commented Jan 7, 2025

+core

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2025

This pull request is fully signed and it will be integrated in one of the next master IBs (test failures were overridden). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @rappoccio, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b5cf409 into cms-sw:master Jan 7, 2025
13 of 14 checks passed
@makortel makortel deleted the transformerStream branch January 7, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add StreamID parameter to Transfomer callback functions
5 participants