Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49632/47174 Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
Thanks for this! Could you run code-format and update the PR? |
|
Milestone for this pull request has been moved to CMSSW_16_1_X. Please open a backport if it should also go in to CMSSW_16_0_X. |
|
just following up on this: can you run code-format and update the PR when you get the chance? thanks |
|
Milestone for this pull request has been moved to CMSSW_17_0_X. Please open a backport if it should also go in to CMSSW_16_1_X. |
|
Pull request #49632 was updated. @BenjaminRS, @cmsbuild, @quinnanm can you please check and sign again. |
|
test parameters:
|
|
please test |
|
-1 Failed Tests: RelVals Failed RelVals |
| void* handle = dlopen("libclobber.so", RTLD_NOW | RTLD_GLOBAL); | ||
| if (!handle) { | ||
| std::cerr << "dlopen failed: " << dlerror() << "\n"; | ||
| return; | ||
| } | ||
|
|
||
| using f_t = void (*)(); | ||
|
|
||
| dlerror(); // clear any old error | ||
| f_t f = reinterpret_cast<f_t>(dlsym(handle, "f")); | ||
|
|
||
| const char* err = dlerror(); | ||
| if (err) { | ||
| std::cerr << "dlsym failed: " << err << "\n"; | ||
| return; | ||
| } | ||
|
|
||
| f(); // call into proxy |
There was a problem hiding this comment.
This seems to be causing the relval crashes- is this supposed to be here? maybe it's an unintended inclusion in the code-format commit?
There was a problem hiding this comment.
this #include <dlfcn.h> is probably not intended as well? (link)
|
would it be possible to move the NNTau code to using triggerflow @m-glowacki ? |
Wires the new cms-hls4ml/NNPuppiTauModel package into the SCRAM build following the L1TSC4NGJetModel/AXOL1TL precedent, so TauNNIdHW can load its weights via hls4mlEmulator::ModelLoader instead of in-tree global arrays (avoids the symbol-clobbering bug class in cms-sw/cmssw#49632).
Replaces the in-tree hls4ml weight arrays (w2/b2/.../w20/b20, global linkage, no namespace) with a call through hls4mlEmulator::Model, loaded at runtime via hls4mlEmulator::ModelLoader from the new cms-hls4ml/ NNPuppiTauModel external. This follows the same pattern already used for JetId/L1TSC4NGJetID, and avoids ELF symbol clashes when multiple hls4ml models are loaded into the same process (cms-sw#49632). - TauNNIdHW takes a std::shared_ptr<hls4mlEmulator::Model> in its constructor; EvaluateNN() becomes a thin prepare_input/predict/ read_result wrapper instead of inlining the dense/relu/sigmoid layer sequence. - L1NNTauProducer gains a ModelLoader member and a configurable tauModelPath parameter (default "NNPuppiTauModel_v1"), mirroring L1TSC4NGJetProducer. - Deletes the now-dead in-tree taus/{defines.h,tau_parameters.h,weights/, nnet_utils/} -- fully superseded by the external package. - Adds a CppUnit test (testNNPuppiTauModel.cppunit.cc) that loads the external model (including two instances at once, the scenario that used to clash on un-namespaced symbols) and sanity-checks predict() output.
Wires the new cms-hls4ml/NNPuppiTauModel package into the SCRAM build following the L1TSC4NGJetModel/AXOL1TL precedent, so TauNNIdHW can load its weights via hls4mlEmulator::ModelLoader instead of in-tree global arrays (avoids the symbol-clobbering bug class in cms-sw/cmssw#49632).
Mirrors the cms-hls4ml externals pattern used by L1TSC4NGJetModel/AXOL1TL: namespaces all hls4ml-generated weights, configs, and the dense/activation call sequence under hls4ml_nnpuppitaumodel_v1, and exposes only the extern "C" create_model()/destroy_model() factory pair across the .so boundary. This avoids ELF symbol clashes (w2/b2/.../w20/b20) when multiple hls4ml models are loaded into the same CMSSW process (cms-sw/cmssw#49632). NNPuppiTauModel_v1.{h,cpp} are a manual, logic-preserving extraction of TauNNIdHW::EvaluateNN()'s layer call sequence into a standalone namespaced free function, since this model's network body was never kept as a free function in CMSSW (unlike other hls4ml exports). Co-authored-by: artlbv <arturlbv@gmail.com>
Wires the new cms-hls4ml/NNPuppiTauModel package into the SCRAM build following the L1TSC4NGJetModel/AXOL1TL precedent, so TauNNIdHW can load its weights via hls4mlEmulator::ModelLoader instead of in-tree global arrays (avoids the symbol-clobbering bug class in cms-sw/cmssw#49632). Co-authored-by: artlbv <arturlbv@gmail.com>
Replaces the in-tree hls4ml weight arrays (w2/b2/.../w20/b20, global linkage, no namespace) with a call through hls4mlEmulator::Model, loaded at runtime via hls4mlEmulator::ModelLoader from the new cms-hls4ml/ NNPuppiTauModel external. This follows the same pattern already used for JetId/L1TSC4NGJetID, and avoids ELF symbol clashes when multiple hls4ml models are loaded into the same process (cms-sw#49632). - TauNNIdHW takes a std::shared_ptr<hls4mlEmulator::Model> in its constructor; EvaluateNN() becomes a thin prepare_input/predict/ read_result wrapper instead of inlining the dense/relu/sigmoid layer sequence. - L1NNTauProducer gains a ModelLoader member and a configurable tauModelPath parameter (default "NNPuppiTauModel_v1"), mirroring L1TSC4NGJetProducer. - Deletes the now-dead in-tree taus/{defines.h,tau_parameters.h,weights/, nnet_utils/} -- fully superseded by the external package. - Adds a CppUnit test (testNNPuppiTauModel.cppunit.cc) that loads the external model (including two instances at once, the scenario that used to clash on un-namespaced symbols) and sanity-checks predict() output. Co-authored-by: artlbv <arturlbv@gmail.com>
Mirrors the cms-hls4ml externals pattern used by L1TSC4NGJetModel/AXOL1TL: namespaces all hls4ml-generated weights, configs, and the dense/activation call sequence under hls4ml_nnpuppitaumodel_v1, and exposes only the extern "C" create_model()/destroy_model() factory pair across the .so boundary. This avoids ELF symbol clashes (w2/b2/.../w20/b20) when multiple hls4ml models are loaded into the same CMSSW process (cms-sw/cmssw#49632). NNPuppiTauModel_v1.{h,cpp} are a manual, logic-preserving extraction of TauNNIdHW::EvaluateNN()'s layer call sequence into a standalone namespaced free function, since this model's network body was never kept as a free function in CMSSW (unlike other hls4ml exports). Synced from artlbv/NNpuppitau.
Wires the new cms-hls4ml/NNPuppiTauModel package into the SCRAM build following the L1TSC4NGJetModel/AXOL1TL precedent, so TauNNIdHW can load its weights via hls4mlEmulator::ModelLoader instead of in-tree global arrays (avoids the symbol-clobbering bug class in cms-sw/cmssw#49632). Co-authored-by: artlbv <arturlbv@gmail.com>
Replaces the in-tree hls4ml weight arrays (w2/b2/.../w20/b20, global linkage, no namespace) with a call through hls4mlEmulator::Model, loaded at runtime via hls4mlEmulator::ModelLoader from the new cms-hls4ml/ NNPuppiTauModel external. This follows the same pattern already used for JetId/L1TSC4NGJetID, and avoids ELF symbol clashes when multiple hls4ml models are loaded into the same process (#49632). - TauNNIdHW takes a std::shared_ptr<hls4mlEmulator::Model> in its constructor; EvaluateNN() becomes a thin prepare_input/predict/ read_result wrapper instead of inlining the dense/relu/sigmoid layer sequence. - L1NNTauProducer gains a ModelLoader member and a configurable tauModelPath parameter (default "NNPuppiTauModel_v1"), mirroring L1TSC4NGJetProducer. - Deletes the now-dead in-tree taus/{defines.h,tau_parameters.h,weights/, nnet_utils/} -- fully superseded by the external package. - Adds a CppUnit test (testNNPuppiTauModel.cppunit.cc) that loads the external model (including two instances at once, the scenario that used to clash on un-namespaced symbols) and sanity-checks predict() output. Co-authored-by: artlbv <arturlbv@gmail.com>
Replaces the in-tree hls4ml weight arrays (w2/b2/.../w20/b20, global linkage, no namespace) with a call through hls4mlEmulator::Model, loaded at runtime via hls4mlEmulator::ModelLoader from the new cms-hls4ml/ NNPuppiTauModel external. This follows the same pattern already used for JetId/L1TSC4NGJetID, and avoids ELF symbol clashes when multiple hls4ml models are loaded into the same process (cms-sw#49632). - TauNNIdHW takes a std::shared_ptr<hls4mlEmulator::Model> in its constructor; EvaluateNN() becomes a thin prepare_input/predict/ read_result wrapper instead of inlining the dense/relu/sigmoid layer sequence. - L1NNTauProducer gains a ModelLoader member and a configurable tauModelPath parameter (default "NNPuppiTauModel_v1"), mirroring L1TSC4NGJetProducer. - Deletes the now-dead in-tree taus/{defines.h,tau_parameters.h,weights/, nnet_utils/} -- fully superseded by the external package. - Adds a CppUnit test (testNNPuppiTauModel.cppunit.cc) that loads the external model (including two instances at once, the scenario that used to clash on un-namespaced symbols) and sanity-checks predict() output. Co-authored-by: artlbv <arturlbv@gmail.com>
|
Superseded by #51263 |
PR description:
We have had a recurring issue with hls4ml emulator models that are defined as externals that they sometimes clobber each other's symbols (arrays for weights typically) [1, 2]. This has been fixed by enforcing that new models define a namespace, with the other variables defined within. However we have one model that is defined in CMSSW source, whose development predates the externals setup, whose weights are not in a namespace, so those symbols are in the global namespace. This PR wraps that model in a namespace.
PR validation:
First of all checking symbols.
In the release:
$ readelf -Ws /cvmfs/cms.cern.ch/el9_amd64_gcc12/cms/cmssw/CMSSW_15_1_0_pre6/lib/el9_amd 64_gcc12/pluginL1TriggerPhase2L1ParticleFlowAuto.so | grep w17 728: 00000000001794f0 20 OBJECT GLOBAL DEFAULT 25 w17 4466: 00000000001794f0 20 OBJECT GLOBAL DEFAULT 25 w17In this PR:
$ readelf -Ws ../lib/el9_amd64_gcc12/pluginL1TriggerPhase2L1ParticleFlowAut o.so | grep w17 1363: 00000000001794f0 20 OBJECT GLOBAL DEFAULT 25 _ZN20hls4ml_nnpuppitau_v03w17E 4300: 00000000001794f0 20 OBJECT GLOBAL DEFAULT 25 _ZN20hls4ml_nnpuppitau_v03w17EI also checked the behaviour by defining a proxy external library that just defines and prints out some of these problematic symbols, and compiling it and loading it in CMSSW analagously to an hls4ml external. Working with the release, I see that the values are "successfuly" clobbered by the NNPuppiTau weights, while with the fix of this branch they retain their locally defined values. I ran a configuration file defined in FastPUPPI that ntuplizes the NNPuppiTau here just to run the producer in the job. There was no change in the ntuple produced since the model here is the one clobbering the others' weights.