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

Improve ROOT compatibility and ability to build without TPLs #104

Merged
merged 8 commits into from
Jan 19, 2021

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Jan 16, 2021

I'm adding SWIG wrappers to celeritas (primarily for my own personal debugging/data exploration, could be useful elsewhere though), and it's better to not have to conditionally wrap files and objects based on what's configured. With this change, all the classes are defined and their headers can be included (aside from vecgeom, which we can't forward declare due to inline namespaces and macros). Tests that rely on disabled configuration issues now build but are "disabled" when running through cmake, and invoking them manually explains why they're disabled:

../src/io/RootImporter.noroot.cc:21:
celeritas: required dependency is disabled in this build: ROOT

@sethrj sethrj added the core Software engineering infrastructure label Jan 16, 2021
@pcanal
Copy link
Contributor

pcanal commented Jan 17, 2021

Alternatively (especially since we already build against ROOT) we could use PyROOT (or the root prompt if you prefer a C++ prompt :)) and yes, there is also Jupyter support.

@sethrj
Copy link
Member Author

sethrj commented Jan 18, 2021

Alternatively (especially since we already build against ROOT) we could use PyROOT (or the root prompt if you prefer a C++ prompt :)) and yes, there is also Jupyter support.

Is this with regard to the SWIG interfaces (#105) or the new macro?

I went with SWIG because I'm familiar with all its foibles and can get an interface up and running quite quickly. I have little experience with ROOT; and when @stognini last tried to get PyROOT up and running he couldn't. So if you're up for taking some time to show us how to get started with pyroot/jupyter for interfacing to celeritas classes and data, I'd be delighted to learn :)

@sethrj
Copy link
Member Author

sethrj commented Jan 18, 2021

@pcanal Speaking of ROOT, though: on this branch ROOT fails on the docker image but not on my mac. (I haven't tested on any other systems, but it fails on a pretty minimal build on the ubuntu image.) Digging into this now, the failure is in the geant-exporter-cat:

1/2 Test #41: app/geant-exporter ...............   Passed    0.75 sec
    Start 42: app/geant-exporter-cat
2/2 Test #42: app/geant-exporter-cat ...........***Failed    0.21 sec
status: Opening ROOT file
status: Loading particle data
Error in <TTree::SetBranchAddress>: unknown branch -> ImportParticle
../app/geant-exporter/geant-exporter-cat.cc:272: critical: Exception while reading ROOT file 'test-data.root': ../src/io/RootImporter.cc:126:
celeritas: internal assertion failed: !particle.name.empty()

but looking further the first issue is in the geant-exporter itself:

41: info: Loading geometry from /home/celeritas/celeritas/app/geant-exporter/data/four-steel-slabs.gdml
41: info: Created ROOT output file 'test-data.root'
41: status: Exporting particles
41: Error in <TTree::Branch>: The pointer specified for ImportParticle is not of a class or type known to ROOT
41: info: Added 19 particles
41: status: Exporting physics tables
41: Error in <TTree::Branch>: The pointer specified for ImportProcess is not of a class or type known to ROOT
41: info: Added 6 processes for e+
41: info: Added 5 processes for e-
41: info: Added 5 processes for gamma
41: status: Exporting material and volume information
41: Error in <TTree::Branch>: The pointer specified for GdmlGeometryMap is not of a class or type known to ROOT
41: info: Added 2 materials
41: status: Closing output file
1/1 Test #41: app/geant-exporter ...............   Passed    0.76 sec

So first of all, the "Error"s being printed to cout are truly errors but they don't cause the test to fail. Is there a way to get errors to raise an exception, or a flag we can check somewhere in the code to make sure ROOT worked?

Second, I figured out by process of elimination that linking the Import*.cc files directly into the CeleritasIO library, rather than into the upstream Celeritas library, fixes the issue so that ROOT can "see" the classes. Any idea what the hell is up with that?

@sethrj
Copy link
Member Author

sethrj commented Jan 18, 2021

@pcanal Follow-up: I tested on RHEL8 and ROOT works both ways there. I even added the CMAKE_SHARED_LINKER_FLAGS=-Wl,--no-as-needed flag which was necessary to get VecGeom+CUDA working on Ubuntu. Could be a bug in ROOT that shows up due to some of the other nonstandard ubuntu toolchain compiler flags? Is ROOT regularly tested on Ubuntu?

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

Error in TTree::Branch: The pointer specified for ImportParticle is not of a class or type known to ROOT

This indicates that the dictionary for ImportParticle was not loaded. Usually it loaded as part of shared library. (The as-needed flag can be a problem there as nothing used the symbol, they just get register via a global static initialization).

The error should be detectable at run-time by checking the return value of the call to TTree::Branch.

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

Is ROOT regularly tested on Ubuntu?

Yes :)

some of the other nonstandard ubuntu toolchain compiler flags?

Yes, it is possible that some unusual flag as negative effects ... Which one are you thinking about.

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

Error in TTree::SetBranchAddress: unknown branch -> ImportParticle

SetBranchAddress should also have returned an error code.

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

and when @stognini last tried to get PyROOT up and running he couldn't.

What was the problem?

@sethrj
Copy link
Member Author

sethrj commented Jan 19, 2021

This indicates that the dictionary for ImportParticle was not loaded. Usually it loaded as part of shared library. (The as-needed flag can be a problem there as nothing used the symbol, they just get register via a global static initialization).

This is a good point -- it might be that when the CeleritasIO library has only the ROOT definitions, the linker removes it from the geant-exporter. This definitely sounds like a case where we shouldn't really be linking that library as SHARED but should be creating a MODULE and the loading with dlopen or some suitable ROOT utility. Have you got any suggestions?

EDIT: I checked the failing case and the geant-exporter did not keep link libCeleritasRoot linked into the exe. I only had the cmake configuration for the shared linker flags set to not do "as-needed", because that was necessary for VecGeom. Adding exe linker flags now keeps the library linked as expected. I still regard this as something of a hack -- the MODULE/dlopen method is going to be more robust.

The error should be detectable at run-time by checking the return value of the call to TTree::Branch.

Thanks! I added some checks and it appears to catch the error. Could you please take a look at my changes to make sure I'm doing it correctly or if I missed any spots?

This makes it clearer when running tests what functionality is disabled:
it shows "not run" rather than fewer tests. It also allows inclusion of
the header files even when the features are disabled.

The non-ROOT-specific IO routines have been moved into the main
celeritas library, and the library name changed to match.
@stognini
Copy link
Member

and when @stognini last tried to get PyROOT up and running he couldn't.

What was the problem?

Root does not work well with anaconda. Simply installing python + root does work.

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

Root does not work well with anaconda. Simply installing python + root does work.

There is a build of ROOT in anaconda though ... isn't that one working?

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

I added some checks and it appears to catch the error. Could you please take a look at my changes to make sure I'm doing it correctly or if I missed any spots

Looks like you update all that kind of error. In addition, testing the result of TFile creation (TFile::Open) should be a kin to:

if (file == nullptr || file->IsZombie()) 
   ErrorOut(...);

Another to check the return value of is:

root_file_->Write();

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

Is there a way to get errors to raise an exception, or a flag we can check somewhere in the code to make sure ROOT worked?

In addition to check the return value, we can turn Error message into exception by inserting our own message handler but this has the downside that (when Celeritas incorporated/used-from Experiment framework, this would compete with that framework's error message handler.

@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

or some suitable ROOT utility. Have you got any suggestions?

ROOT is capable of autoloading the library that contains the dictionary.

In the current version this is based on the content of files with prefix .rootmap that are generated during dictionary generation and are taken in account if they are found on the LD_LIBRARY_PATH

The one produced by celeritas needed some improvement to work properly. Namely I see:

cat ./src/libCeleritasRootGeantParticle.rootmap
{ decls }
namespace celeritas {  }

[ libCeleritasRootGeantParticle.so ]
# List of selected classes
class celeritas::GeantParticle
header io/GeantParticle.hh

But I can not find any library named libCeleritasRootGeantParticle.so, if the dictionary generation step is given the name of the actual shared library containing the compiled dictionary, then ROOT will be able to autoload that library.

@stognini
Copy link
Member

Root does not work well with anaconda. Simply installing python + root does work.

There is a build of ROOT in anaconda though ... isn't that one working?

I just did not want to depend on (another) root managed by conda, and tried to make it work with an external root (either standalone build or from another package manager, such as spack). The main issue is most likely due to me trying to build root in the anaconda run-time environment, which ended up lacking information to be linked correctly.

@sethrj
Copy link
Member Author

sethrj commented Jan 19, 2021

But I can not find any library named libCeleritasRootGeantParticle.so, if the dictionary generation step is given the name of the actual shared library containing the compiled dictionary, then ROOT will be able to autoload that library.

Changing the library name from celeritas_root to libCeleritasRootGeantParticle wouldn't be a big deal. Does it use .dylib on the mac like other libraries or require that specific suffix? And does it have to be in the LD_LIBRARY_PATH (or DYLD_LIBRARY_PATH on mac) or can it be set at configure/install time?

@sethrj sethrj requested a review from pcanal January 19, 2021 17:19
@sethrj sethrj changed the title Add "not configured" macro Improve ROOT compatibility and ability to build without TPLs Jan 19, 2021
@pcanal
Copy link
Contributor

pcanal commented Jan 19, 2021

Does it use .dylib on the mac like other libraries or require that specific suffix?

It should work with dylib also.

And does it have to be in the LD_LIBRARY_PATH

It has to be on "a" LIBRARY_PATH since the purpose is to "discover" libraries that have not been linked to the executable.

In practice LD_LIBRARY_PATH is the usually the most convenient but in fact it "just" need to be on one of the directories listed by gSystem->GetDynamicPath() and there is several way to add path there (.rootrc configuration file, LD_LIBRARY_PATH or explicit C++ calls)

@sethrj sethrj merged commit 05624cf into celeritas-project:master Jan 19, 2021
@sethrj
Copy link
Member Author

sethrj commented Jan 19, 2021

Thanks for the review @pcanal !

@sethrj sethrj deleted the not-configured-macro branch January 25, 2021 14:56
stognini pushed a commit to stognini/celeritas that referenced this pull request Feb 1, 2021
…as-project#104)

* Add `CELER_NOT_CONFIGURED` macro
* Remove extra link requirement for event reader test
* Disable but still compile HepMC3 and ROOT tests when unconfigured
* Fix public/private and implementation include
* Add dockerfile script to launch local testing for an MR
* Add error checking code to ROOT calls
* Add necessary linker flags to force shared library link in Ubuntu

This makes it clearer when running tests what functionality is disabled:
it shows "not run" rather than fewer tests. It also allows inclusion of
the header files even when the features are disabled.

The non-ROOT-specific IO routines have been moved into the main
celeritas library, and the library name changed to match.
@sethrj sethrj added external Dependencies and framework-oriented features and removed core Software engineering infrastructure labels Feb 18, 2023
@sethrj sethrj added the enhancement New feature or request label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external Dependencies and framework-oriented features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants