Use JANA's new first-class PODIO integration #578

wdconinc · 2023-03-28T21:25:09Z

Briefly, what does this PR introduce?

This PR allows eicrecon to use PODIO associations correctly, resolving a major source of confusion, entropy, and bugs.
As of v2.1.0, the JANA2 framework has first-class support for PODIO. This means that users can directly read and
write PODIO collections. Even if users continue to use the classic JANA vector-of-pointers style, under the hood the PODIO objects are always registered to a collection and that collection is registered to a frame. JANA understands and abides by PODIO's memory ownership semantics, including supporting subset collections.

This PR also converts three problematic factories into multifactories, which support multiple outputs. These factories used direct object ID rewriting in order to set up valid associations, and/or had particularly thorny PODIO ownership issues. They are now models for how to write factories in the future. These factories are: MatchClusters_factory, ParticlesWithTruthPID_factory, and TrackingResult_factory.

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue #__)
Documentation update
Other: __

Please check if this PR fulfills the following:

Tests for the changes have been added
Documentation has been added / updated
Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

The way that users should write future factories has changed. Users should look to TrackingResult_factory as an example of how to write factories with multiple outputs.
Output files are now written using Podio's Frame writer, which is incompatible with Podio's older EventStore writer.

Does this PR change default behavior?

JEventProcessorPODIO used to maintain a list of factories that have thrown an exception. After it crashed once, it would never be called again. Now, it will be called again on every event. The reason for this change in behavior is that PODIO v0.16.3 has a bug where collection IDs depend on insertion order, which causes the PODIO frame writer to crash. Excepting factories still produce an empty collection (thus preserving the insertion order), which the JEventProcessorPODIO then persists.

…to segvios when reading back association objects from podio file. will all be replaced by PR eic#578 but this helps keep things working in advance of that.

Merge with tracking

Squash me with MatchClusters

This means that we won't need to modify 100+ files again

veprbl · 2023-04-14T16:45:09Z

Fails clang-tidy check:

/home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:11:10: error: 'datamodel_glue.h' file not found [clang-diagnostic-error]
#include "datamodel_glue.h"
         ^~~~~~~~~~~~~~~~~~

wdconinc · 2023-04-14T16:51:21Z

Hmm, strange, how did it handle that correctly before? We've always had a glue header. ..

nathanwbrei · 2023-04-14T16:52:36Z

I'm guessing clang-tidy is complaining about the "" instead of doing the full #include <services/io/podio/datamodel_glue.h>

c-dilks · 2023-04-16T21:23:01Z

src/services/io/podio/CMakeLists.txt

@@ -34,7 +34,7 @@ plugin_glob_all(${PLUGIN_NAME})
 # but that never really worked quite right. Thus, we just force-run it
 # whenever cmake is run.
 execute_process (
-        COMMAND python3 ${PROJECT_SOURCE_DIR}/make_datamodel_glue.py WORKING_DIR=${PROJECT_BINARY_DIR} EDM4HEP_INCLUDE_DIR=${EDM4HEP_INCLUDE_DIR} EDM4EIC_INCLUDE_DIR=${EDM4EIC_INCLUDE_DIR}
+        COMMAND python3 ${PROJECT_SOURCE_DIR}/make_datamodel_glue.py WORKING_DIR=${PROJECT_SOURCE_DIR} EDM4HEP_INCLUDE_DIR=${EDM4HEP_INCLUDE_DIR} EDM4EIC_INCLUDE_DIR=${EDM4EIC_INCLUDE_DIR}


What's the reason for this change? I'm wondering why re-building is now very slow, but reverting this change seems to help speed things up (only tested on one of my development branches).

@nathanwbrei in case he didn't get this (I created the PR so I may be the only one getting the notifications)

It's because every factory now needs datamodel_glue.h for the PodioTypeMap that defines the relationship between podio types and their collections. The build dir for the podio plugin isn't on the include path for the other plugins, and the source dir is. I understand that using datamodel_glue all over the place is undesirable because it makes compile times longer and is just generally ugly. Luckily this is temporary. By adding one using MyPodioObject::collection_t = MyPodioObjectCollection to the generated EDM headers we can get rid of this. That requires a new release of both JANA and an approved PODIO PR.

Contrary to this report, the compilation times don't change for me when I try to compile revisions before this PR.

### Briefly, what does this PR introduce? No need to keep `hello world` examples around in the source tree. ### What kind of change does this PR introduce? - [x] Bug fix (issue: debt) - [ ] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? No.

wdconinc marked this pull request as draft March 28, 2023 21:25

nathanwbrei force-pushed the nbrei_jana_podio branch from 404d6c4 to 4f50384 Compare March 29, 2023 02:32

nathanwbrei force-pushed the nbrei_jana_podio branch from f3daa2a to d0a603c Compare April 12, 2023 19:58

nathanwbrei force-pushed the nbrei_jana_podio branch from 9c5b74a to f3c8290 Compare April 12, 2023 20:43

steinber mentioned this pull request Apr 12, 2023

Fixes associated with forward HCal digitization, clustering and truth associations. #600

Closed

7 tasks

nathanwbrei force-pushed the nbrei_jana_podio branch from f3c8290 to f176c96 Compare April 13, 2023 13:54

nathanwbrei changed the title ~~Nbrei jana podio~~ Use JANA's new first-class PODIO integration Apr 13, 2023

nathanwbrei requested review from DraTeots and faustus123 April 13, 2023 15:05

nathanwbrei marked this pull request as ready for review April 13, 2023 15:06

wdconinc mentioned this pull request Apr 13, 2023

feat: associate MC photons in PhotoMultiplierHitDigi output #599

Merged

7 tasks

nathanwbrei force-pushed the nbrei_jana_podio branch from 01f373b to e767de6 Compare April 14, 2023 03:11

wdconinc enabled auto-merge (squash) April 14, 2023 03:12

nathanwbrei force-pushed the nbrei_jana_podio branch from 3ee7a97 to 9fa09ff Compare April 14, 2023 04:00

nathanwbrei requested review from FriederikeBock and nathanwbrei April 14, 2023 13:08

nathanwbrei added 9 commits April 14, 2023 10:56

Excise old EventStore-based readers and writers

0988a8b

Add new Frame-based PODIO sources and processors

2c0e9dc

Factories use JFactoryPodioTFixed instead of JFactoryT

b86b039

TrackerHitCollector_factory produces a subset collection

8ddf991

Update JChainFactory, add JChainMultifactory

d97b7d1

{TrackingResult, ParticlesWithTruthPID}_factories become multifactory

6ca4934

Merge with tracking

TrackingTest_processor no longer uses ParticlesWithAssociation

28cf5c3

MatchClusters_factory becomes multifactory

6de8efd

Squash me with MatchClusters

Rename JFactoryPodioTFixed, strip out datamodel_glue

3e6440f

This means that we won't need to modify 100+ files again

nathanwbrei force-pushed the nbrei_jana_podio branch from 9fa09ff to 3e6440f Compare April 14, 2023 14:57

nathanwbrei approved these changes Apr 14, 2023

View reviewed changes

wdconinc merged commit e14e81b into main Apr 14, 2023

wdconinc deleted the nbrei_jana_podio branch April 14, 2023 15:07

c-dilks reviewed Apr 16, 2023

View reviewed changes

nathanwbrei mentioned this pull request Apr 18, 2023

PODIO multithreading #176

Closed

7 tasks

c-dilks mentioned this pull request Apr 27, 2023

cmake: don't regenerate datamodel_{glue,includes}.h on each cmake run #634

Merged

7 tasks

nathanwbrei mentioned this pull request Apr 28, 2023

Fixes for Dmitry R.'s platform #637

Closed

7 tasks

c-dilks mentioned this pull request May 1, 2023

Revive AnalysisEpicPodio and use PODIO Frames eic/epic-analysis#272

Open

wdconinc added a commit that referenced this pull request Jul 28, 2023

fix: rm unused and empty since #578

ae0307b

wdconinc added a commit that referenced this pull request Jul 28, 2023

fix: rm unused and empty since #578

781eb5c

wdconinc added a commit that referenced this pull request Jul 29, 2023

fix: rm unused and empty since #578

f74502e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use JANA's new first-class PODIO integration #578

Use JANA's new first-class PODIO integration #578

wdconinc commented Mar 28, 2023 •

edited by nathanwbrei

Loading

veprbl commented Apr 14, 2023

wdconinc commented Apr 14, 2023

nathanwbrei commented Apr 14, 2023

c-dilks Apr 16, 2023

wdconinc Apr 16, 2023 •

edited

Loading

nathanwbrei Apr 16, 2023 •

edited

Loading

veprbl Apr 27, 2023

Use JANA's new first-class PODIO integration #578

Use JANA's new first-class PODIO integration #578

Conversation

wdconinc commented Mar 28, 2023 • edited by nathanwbrei Loading

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

veprbl commented Apr 14, 2023

wdconinc commented Apr 14, 2023

nathanwbrei commented Apr 14, 2023

c-dilks Apr 16, 2023

Choose a reason for hiding this comment

wdconinc Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

nathanwbrei Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

veprbl Apr 27, 2023

Choose a reason for hiding this comment

wdconinc commented Mar 28, 2023 •

edited by nathanwbrei

Loading

wdconinc Apr 16, 2023 •

edited

Loading

nathanwbrei Apr 16, 2023 •

edited

Loading