-
Notifications
You must be signed in to change notification settings - Fork 35
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 example offloading template #1573
base: develop
Are you sure you want to change the base?
Conversation
Test summary0 tests 0 ✅ 0s ⏱️ Results for commit 8895050. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the example up a directory, make sure it shows up in the manual, and add it to scripts/ci/test-examples.sh
…gManager instead of TrackingAction
A couple of general comments:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good start, but may need some refinement to make the example more general.
example/offload-template/main.cc
Outdated
} | ||
|
||
// Construct run manager | ||
G4RunManager run_manager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
G4RunManager run_manager; | |
auto runManager = G4RunManagerFactory::CreateRunManager(G4RunManagerType::Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make it MT if Geant4 is built with threading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually made this a G4RunManagerType::Serial
to run a single-thread execution, and just added a comment on the MT case.
Edit: My reasoning to keep it single-threaded is so that we don't have to also require an install with GEANT4_BUILD_MULTITHREADED=ON
as a condition just to test a tiny offload example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GEANT4_BUILD_MULTITHREADED=ON is now the default (so, you do not need explicitly to set it), which then is able to run both sequential and tasking (default tasking option is the traditional old MT). So it will support all cases.
// Construct Celeritas offloading interface | ||
CelerSimpleOffload().Build( | ||
&CelerSetupOptions(), &CelerSharedParams(), &CelerLocalTransporter()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in BuildForMaster (assuming that the default run manager (tasking run manager) is used)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, because it is not an MT application. But good point, I may have to either make it an MT example, or add a few notes throughout it about the MT case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should support MT? Most new Geant4 apps should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought of that. But then we have to require a Geant4 install with GEANT4_BUILD_MULTITHREADED=ON
for a tiny example. Fairly, that would be standard for most cases, but seemed simpler to run a single-threaded one, so any compilation works, and explain how MT should be handled in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BuildForMaster
member function can be supplied always. If Geant4 is not MT enabled, it'll never be called, but by being there both cases can be supported.
Sorry that I did not know that TrackingManagerOffload already implemented and you already used the interface. Your approach is good. |
// Construct Celeritas offloading interface | ||
CelerSimpleOffload().Build( | ||
&CelerSetupOptions(), &CelerSharedParams(), &CelerLocalTransporter()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BuildForMaster
member function can be supplied always. If Geant4 is not MT enabled, it'll never be called, but by being there both cases can be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please take a look at the couple remaining suggestions.
f7e6e40
to
4ce08ad
Compare
Hey @drbenmorgan , I've been trying to understand some weird behavior in the multiple cmake package scripts we use and thought you might know... https://github.com/celeritas-project/celeritas/actions/runs/12812027794 shows @stognini 's example failing for Geant4 11.1 and 11.2 . I finally realized we'd already "solved" this issue before: https://github.com/celeritas-project/celeritas/blob/eb9b0d721354503f12d3fbf18be0171225dcfdb8/cmake/CeleritasConfig.cmake.in#L70--L72 is due to #1152 , which notes "issues with the new geant4 cmake variable cache break[ing] xercesc importing for 11.1–11.2". For some reason though our previous fix fails when Celeritas isn't marked Do you have any ideas how to better fix this? Thanks! |
Argh, this looks like the same issue as in apt-sim/AdePT#267, the |
Yeah, we added that and it worked as long as celeritas and Geant were both required 😅 |
O.k., no better ideas from my side at the moment - will try and take a look on the VecGeom etc side next week... |
Aha, I see in the vecgeom config file:
... which I introduced in VecGeom 52f767d29f, which maybe is conflicting with something in Geant4 and/or XercesC... |
This reverts commit e4eed68.
This PR adds a new example that acts more as a template structure for users to learn how to either add Celeritas capabilities to their existing framework or build a new Geant4 application (with offloading capabilities) from scratch.
It is technically very similar to the existing
simple-offload.cc
, but it provides more directions to the user, as it is much more similar to the well known examples found in Geant4. The main reasons to add it, in my opinion, is that it provides:CMakeLists.txt
that teaches the user how to link a Celeritas' install against their project; and.hh/.cc
files, as any Geant4 example, which should make direct comparisons to existing Geant4 codes more intuitive.