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 ability to link to additional libraries for from_file processors #1366

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

tomeichlersmith
Copy link
Member

@tomeichlersmith tomeichlersmith commented Jun 12, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

I very quickly noticed that this stand-alone processor function can be updated to link to additional pre-compiled ldmx-sw libraries which is helpful in many circumstances. Specifically, I am working with a new graduate student who wished to have her analyzer user the detector ID classes in the DetDescr library and updating the compilation command to include -lDetDescr was all that was necessary.

The developments here are focused on allowing for the user to add other libraries to link. Pretty much any library that is system-findable (or installed with ldmx-sw) should be linkable in this way, although I haven't tested this thoroughly.

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.

@tomeichlersmith tomeichlersmith changed the title add ability to link to additional libraries add ability to link to additional libraries for from_file processors Jun 12, 2024
Copy link
Member Author

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

The complexity here is that some things work well without the additional linking because the libraries are loaded via other links. The example I've crafted below is the one I've first encountered, but may not be the simplest/minimal example.

cfg.py

We're just testing compilation and loading so I don't run over any events.

from LDMX.Framework import ldmxcfg
p = ldmxcfg.Process('ana')
p.sequence = [ ldmxcfg.Analyzer.from_file('MyAnalyzer.cxx') ]
p.inputFiles = [ ]
p.histogramFile = 'hist.root'

MyAnalyzer.cxx

#include "Framework/EventProcessor.h"

#include "DetDescr/EcalID.h"
#include "Ecal/Event/EcalHit.h"

class MyAnalyzer : public framework::Analyzer {
 public:
  MyAnalyzer(const std::string& name, framework::Process& p)
    : framework::Analyzer(name, p) {}
  ~MyAnalyzer() = default;
  void onProcessStart() final;
  void analyze(const framework::Event& event) final;
};

void MyAnalyzer::onProcessStart() {
  // this is where we will define the histograms we want to fill
  ldmx::EcalID id{0,0,0};
  auto [u, v] = id.getCellUV();
  std::cout << u << ", " << v << std::endl;
}

void MyAnalyzer::analyze(const framework::Event& event) {
  // this is where we will fill the histograms
}

DECLARE_ANALYZER(MyAnalyzer);

Note: In between runs, make sure to rm libMyAnalyzer.so to make sure that the re-compilation occurs.

on trunk

We get an undefined symbol error and library-load time.

tom@appa:~/code/ldmx/1366-from_file-needs$ denv fire cfg.py 
---- LDMXSW: Loading configuration --------
Processor source file /home/tom/code/ldmx/1366-from_file-needs/MyAnalyzer.cxx is newer than its compiled library /home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so (or library does not exist), recompiling...
done compiling /home/tom/code/ldmx/1366-from_file-needs/MyAnalyzer.cxx
Configuration Error [LibraryLoadFailure] : Error loading library '/home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so':/home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so: undefined symbol: _ZNK4ldmx6EcalID9getCellUVEv
  at /home/tom/code/ldmx/ldmx-sw/Framework/src/Framework/PluginFactory.cxx:94 in loadLibrary

on this branch without needs

i.e. the config is as shown above.
We get the same error.

tom@appa:~/code/ldmx/1366-from_file-needs$ denv fire cfg.py 
---- LDMXSW: Loading configuration --------
Processor source file /home/tom/code/ldmx/1366-from_file-needs/MyAnalyzer.cxx is newer than its compiled library /home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so (or library does not exist), recompiling...
done compiling /home/tom/code/ldmx/1366-from_file-needs/MyAnalyzer.cxx
Configuration Error [LibraryLoadFailure] : Error loading library '/home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so':/home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so: undefined symbol: _ZNK4ldmx6EcalID9getCellUVEv
  at /home/tom/code/ldmx/ldmx-sw/Framework/src/Framework/PluginFactory.cxx:94 in loadLibrary

on this branch with needs=['DetDescr']

i.e. the config above was changed with

3c3
< p.sequence = [ ldmxcfg.Analyzer.from_file('MyAnalyzer.cxx') ]
---
> p.sequence = [ ldmxcfg.Analyzer.from_file('MyAnalyzer.cxx', needs=['DetDescr']) ]

No error! 🎉

tom@appa:~/code/ldmx/1366-from_file-needs$ denv fire cfg.py 
---- LDMXSW: Loading configuration --------
Processor source file /home/tom/code/ldmx/1366-from_file-needs/MyAnalyzer.cxx is newer than its compiled library /home/tom/code/ldmx/1366-from_file-needs/libMyAnalyzer.so (or library does not exist), recompiling...
done compiling /home/tom/code/ldmx/1366-from_file-needs/MyAnalyzer.cxx
---- LDMXSW: Configuration load complete  --------
---- LDMXSW: Starting event processing --------
0, 0
---- LDMXSW: Event processing complete  --------

@tomeichlersmith tomeichlersmith marked this pull request as ready for review June 14, 2024 15:29
Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

I ran the code on an existing analyzer I had which didnt have any needs, all looks good.

@tomeichlersmith tomeichlersmith merged commit 5cf7248 into trunk Jun 17, 2024
8 checks passed
@tomeichlersmith tomeichlersmith deleted the from_file-needs branch June 17, 2024 14:52
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