Custom streamer for std::vector<o2::tpc::PadFlags> #14427
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
Member
Author
|
@pzhristov This is my best attempt so far to fix the ARM issue / new ROOT streamer. |
Collaborator
|
How many ENUMs did we actually store in ROOT files? |
Member
Author
|
As far as I can tell, the explicit load is needed for non virtual classes. I do the vector, rather CalDet/CalArray because those classes are more complicated to stream by hand (and still non virtual classes). |
Member
Author
|
Regarding V2, it was said they are too many to convert. |
Member
Author
|
Regarding ROOT, i think that's what they do, and why it now fails: they have no information on what they are actually deserialising. When i check for the class, i get a vector , where two shorts have been put into one int. The only place where that information is available is in the user code. |
Collaborator
|
I will check again with Costin to discuss the conversion. I think it will be possible, even if there are many objects. |
ktf
force-pushed
the
pr14427
branch
2 times, most recently
from
8e0af1a to
afee20a
Compare
Member
Author
|
@pcanal any better way than this to register the custom streamer? |
Collaborator
|
@ktf : But I do not fully understand. Apparently, in the new ROOT they store the size, or they take it from the variable type in the dictionary. They must also store the ROOT version in the file, no? So for all files stored with an older ROOT version, they could just use 32bit instead of the new behavior. Then it should be backward-compatible? @wiechula : The, I know it is annoying, but I would prefer this over writing custom streamers. |
Member
Author
|
@davidrohr I think the issue in the old way of doing things is that there is no information at the reading level whether you are reading N 16 bit enums or N/2 32 bit enums, hence the issue, both in terms of unaligned writes to memory and extra bytes being written for odd values of N. At least that's my understanding. Indeed I do not quite get why we cannot simply read the stored buffer and recast it to the correct type later. @pcanal can you please comment here? |
Collaborator
@pcanal @ktf : So if I understand correctly, in old versions ROOT wrote 32 bits unconditionally (which might be misaligned, or write extra bytes, but it was always 32 bits). New ROOT versions know the size of the enum, and can read / write the correct number of bytes. So what I would do:
|
You can/should use a I/O customization rule, see for example: root-project/root#17009 (comment). Nowadays there is almost never a good reason to use a custom streamer :)
We did: root-project/root#17009 :)
Unfortunately the way the old file are written depends on the custom in memory size. By definition, it is not 32 bits and the file has no clue what that size (only the process that wrote it knows ... and the code at the time, so the information must come from the O2 code ...)
Essentially this is what the I/O customization rule needs to do (i.e. that function needs to encode what the size of the enums was when they were written).
This is almost correct. Unfortunately the ROOT file version is not a good marker. For example if you take an old file and new file and fast merge with hadd, the resulting file will appear as a new file but will have some of the data written in the old/broken format and some in the new format. The discriminant is actually the (T)Class layout checksum or (if you are assigning them) the Class version (in which case you have must increase the number as you start using the new version of ROOT). I strongly recommend to not use this PR (as I see it now, since it does something like |
Member
Author
|
Does the happy solution allow for reading new data with the old releases? |
Collaborator
|
How can we increase the class number for At least I do agree that just checking the file version is broken, since it will not work if the file is created with the old version but modified with the new version. Unfortunately, I assume we will need to support old and new O2 versions which means old and new ROOT versions for some time. But actually, I am against trying to fix this in O2. I would as I wrote before just replace all such CCDB objects by new ones with V2 and change the name in O2 to the V2 version in parallel to bumping ROOT. This is painful now once, but at least it is a clean solution without the need of adding bogus custom streamers. |
Of course not as is ... but the old releases can be patched with a I/O customization rule to read the new class layout/files. |
Member
Author
Then I frankly think the sad solution is "Sad but True". |
Member
Author
This means old code can only use old objects for the calibration. If that is fine, I have no objections. |
You can not. You would update the line |
Collaborator
|
OK, but afaik we are storing the From my side, I think it is fine if old code uses only old objects. In any case, this is only one object, correct? Worst case, one can still create a new object with the old O2. |
Collaborator
Collaborator
|
For reference, I just tried with root v6-36-00-alice4 rebased to v6-36-02 and with AliceO2 with this PR locally merged, and my async reco test crashes when loading the PadFlags vector with segfault. Stack trace is: |
Member
Author
|
looks like it does not try to call my class. i will have a look at i.
Sent from Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: David Rohr ***@***.***>
Sent: Wednesday, July 16, 2025 3:31:57 PM
To: AliceO2Group/AliceO2 ***@***.***>
Cc: Giulio Eulisse ***@***.***>; Mention ***@***.***>
Subject: Re: [AliceO2Group/AliceO2] Add streamer for std::vector<o2::tpc::PadFlags> (PR #14427)
[https://avatars.githubusercontent.com/u/9131638?s=20&v=4]davidrohr left a comment (AliceO2Group/AliceO2#14427)<#14427 (comment)>
For reference, I just tried with root v6-36-00-alice4 rebased to v6-36-02 and with AliceO2 with this PR locally merged, and my async reco test crashes when loading the PadFlags vector with segfault. Stack trace is:
#0 TBufferFile::ReadFastArray (this=0x7ffe5ecdab90, h=0x7f68e5eef010, n=1152519426) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TBufferFile.cxx:1327
#1 0x00007f74bb16889b in TEmulatedCollectionProxy::ReadBuffer (this=0x5556db4dc290, b=..., obj=<optimized out>) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TEmulatedCollectionProxy.cxx:620
#2 0x00007f74bb116418 in TClass::Streamer (this=0x5556d80cad70, obj=<optimized out>, b=..., onfile_class=0x5556c191f1a0) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/core/meta/inc/TClass.h:623
#3 TBufferFile::ReadFastArray (this=0x7ffe5ecdab90, start=<optimized out>, cl=0x5556d80cad70, n=<optimized out>, streamer=<optimized out>, onFileClass=0x5556c191f1a0) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TBufferFile.cxx:1649
#4 0x00007f74bb444127 in TStreamerInfo::ReadBuffer<char**> (this=0x5556d3a5db70, b=..., ***@***.***: 0x5556d9facaa0, ***@***.***=0x5556db3e1a88, ***@***.***=0, ***@***.***=1, narr=360, eoffset=0, arrayMode=3) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TStreamerInfoReadBuffer.cxx:1353
#5 0x00007f74bb25c399 in TStreamerInfoActions::VectorLooper::GenericRead (buf=..., start=<optimized out>, end=0x5556db8eb820, loopconfig=<optimized out>, config=0x5556db3e1a70) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TStreamerInfoActions.cxx:2209
#6 0x00007f74bb112fa4 in TStreamerInfoActions::TConfiguredAction::operator() (this=0x5556db43a670, buffer=..., start_collection=0x5556db8e5e20, end_collection=0x5556db8eb820, loopconf=0x5556d5781330) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/inc/TStreamerInfoActions.h:131
#7 TBufferFile::ApplySequence (this=0x7ffe5ecdab90, sequence=..., start_collection=0x5556db8e5e20, end_collection=0x5556db8eb820) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TBufferFile.cxx:3813
#8 0x00007f74bb30317b in TStreamerInfoActions::ReadSTLMemberWiseSameClass (buf=..., addr=<optimized out>, ***@***.***=0x5556db4306b0, vers=<optimized out>) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TStreamerInfoActions.cxx:805
#9 0x00007f74bb3033ce in TStreamerInfoActions::ReadSTL<&TStreamerInfoActions::ReadSTLMemberWiseSameClass, &TStreamerInfoActions::ReadSTLObjectWiseFastArray> (buf=..., addr=0x5556d956b9c0, conf=0x5556db4306b0) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TStreamerInfoActions.cxx:4524
#10 0x00007f74bb113b8d in TStreamerInfoActions::TConfiguredAction::operator() (this=0x5556d2bb1400, buffer=..., object=0x5556d956b9c0) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/inc/TStreamerInfoActions.h:123
#11 TBufferFile::ApplySequence (this=0x7ffe5ecdab90, sequence=..., obj=0x5556d956b9c0) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TBufferFile.cxx:3747
#12 0x00007f74bb11c099 in TBufferFile::ReadClassBuffer (this=0x7ffe5ecdab90, cl=0x5556c198ad00, pointer=0x5556d956b9c0, onFileClass=<optimized out>) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TBufferFile.cxx:3666
#13 0x00007f74bb1b1e76 in TClass::Streamer (this=0x5556c198ad00, obj=0x5556d956b9c0, b=..., onfile_class=0x0) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/core/meta/inc/TClass.h:623
#14 TKey::ReadObjectAny (this=0x5556d3a5da40, expectedClass=<optimized out>) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TKey.cxx:1118
#15 0x00007f74bb16b6b2 in TDirectoryFile::GetObjectChecked (this=0x7ffe5ecdb5d0, namecycle=<optimized out>, expectedClass=0x5556c198ad00) at /var/tmp/portage/sci-physics/root-6.36.02/work/root-6.36.02/io/io/src/TDirectoryFile.cxx:1115
#16 0x00007f74bfb4ebe2 in o2::framework::(anonymous namespace)::extractFromTFile (what=0x7f74bfde9642 "ccdb_object", file=..., cl=0x5556c198ad00) at /home/qon/alice/sw/SOURCES/O2/dev/0/Framework/Core/src/DataRefUtils.cxx:33
#17 o2::framework::DataRefUtils::decodeCCDB (ref=..., tinfo=...) at /home/qon/alice/sw/SOURCES/O2/dev/0/Framework/Core/src/DataRefUtils.cxx:106
#18 0x00007f74c7a9a79e in o2::framework::DataRefUtils::as<o2::framework::CCDBSerialized<o2::tpc::CalDet<o2::tpc::PadFlags>, void> > (ref=...) at /home/qon/alice/sw/SOURCES/O2/dev/0/Framework/Core/include/Framework/DataRefUtils.h:174
#19 o2::framework::InputRecord::get<o2::tpc::CalDet<o2::tpc::PadFlags>*, char const*> (this=<optimized out>, ***@***.***=0x7f74c7aa4989 "tpcidcpadflags", ***@***.***=0) at /home/qon/alice/sw/SOURCES/O2/dev/0/Framework/Core/include/Framework/InputRecord.h:415
#20 0x00007f74c7a8d480 in o2::gpu::GPURecoWorkflowSpec::fetchCalibsCCDBTPC<o2::gpu::GPUCalibObjectsTemplate<o2::gpu::ConstPtr> > ***@***.***=0x5556bb090e30, pc=..., newCalibObjects=..., oldCalibObjects=...) at /home/qon/alice/sw/SOURCES/O2/dev/0/Framework/Core/include/Framework/ProcessingContext.h:37
#21 0x00007f74c7a52678 in o2::gpu::GPURecoWorkflowSpec::doCalibUpdates ***@***.***=0x5556bb090e30, pc=..., oldCalibObjects=...) at /home/qon/alice/sw/SOURCES/O2/dev/0/GPU/Workflow/src/GPUWorkflowSpec.cxx:1093
—
Reply to this email directly, view it on GitHub<#14427 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAACSMAV2PX5H4IVOSY6QSD3IZH43AVCNFSM6AAAAAB7WX2QE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTANZYGY2DANBTGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Contributor
|
This PR did not have any update in the last 30 days. Is it still needed? Unless further action in will be closed in 5 days. |
Member
Author
|
Updated with the last attempt. |
Collaborator
|
Error while checking build/O2/fullCI_slc9 for c10da95 at 2025-09-08 20:49: Full log here. |
pcanal
reviewed
Sep 8, 2025
pcanal
reviewed
Sep 8, 2025
Collaborator
|
Error while checking build/O2/fullCI_slc9 for f54d259 at 2025-09-09 00:26: Full log here. |
ktf
commented
Sep 9, 2025
pcanal
reviewed
Sep 9, 2025
Collaborator
|
Error while checking build/O2/fullCI_slc9 for 45b40ee at 2025-09-09 14:33: Full log here. |
Collaborator
|
Error while checking build/O2/fullCI_slc9 for 70d812e at 2025-09-12 06:59: Full log here. |
Collaborator
|
Error while checking build/O2/fullCI_slc9 for 7db8df1 at 2025-09-22 22:03: Full log here. |
Collaborator
|
Error while checking build/O2/fullCI_slc9 for fb1b397 at 2025-09-24 18:45: Full log here. |
Member
Author
|
Apparently, this only works with the new ROOT (6.36.04) and not with the production on (6.32.06). Any idea why that would be the case? |
Collaborator
|
Error while checking build/O2/fullCI_slc9 for b247f5c at 2025-09-28 13:16: Full log here. |
* Add support for EXTRA_PATCH in root dictionary generation
Member
Author
|
Code is disabled for the current version of ROOT. |
sawenzel
added a commit
to sawenzel/AliceO2
that referenced
this pull request
Nov 18, 2025
This commit expands on AliceO2Group#14427 and fixes the issue brought up in https://its.cern.ch/jira/browse/O2-4671. After debugging/testing it turns out that the approach taken via a customer streamer for std::vector<o2::tpc::PadFlags> does not take effect in the ROOT/IO because apparently ROOT still prefers to use the CollectionProxy for std::vector and does not employ the custom streamer. Instead, after discussion with @pcanal, this commit proposes to implement a custom stream just for the mData data member of CalArray<o2::tpc::PadFlags>. This is the only place where we use o2::tpc::PadFlags in IO and it fixes the problem when reading CCDB objects with containing such data. I have verified that the following code ``` o2-ccdb-downloadccdbfile -p TPC/Calib/IDC_PadStatusMap_A -t 1731274461770 -d ./ -o tpc_idc.root --no-preserve-path root tpc_idc.root gFile->Get<o2::tpc::CalDet<o2::tpc::PadFlags>>("ccdb_object") ``` correctly executes the custom streamer function. Note that there is also no need to make the code ROOT version dependent. We need to fix the reading in any case and the writing will just stay the same. Concerning situations, where future classes will write data containing std::vector<o2::tpc::PadFlags> we should be protected by the fact that this bug has been fixed >= ROOT 6.36 in any case. This commit relates also to root-project/root#17009
sawenzel
added a commit
to sawenzel/AliceO2
that referenced
this pull request
Nov 18, 2025
This commit expands on AliceO2Group#14427 and fixes the issue brought up in https://its.cern.ch/jira/browse/O2-4671. After debugging/testing it turns out that the approach taken via a customer streamer for std::vector<o2::tpc::PadFlags> does not take effect in the ROOT/IO because apparently ROOT still prefers to use the CollectionProxy for std::vector and does not employ the custom streamer. Instead, after discussion with @pcanal, this commit proposes to implement a custom stream just for the mData data member of CalArray<o2::tpc::PadFlags>. This is the only place where we use o2::tpc::PadFlags in IO and it fixes the problem when reading CCDB objects containing such data. I have verified that the following code ``` o2-ccdb-downloadccdbfile -p TPC/Calib/IDC_PadStatusMap_A -t 1731274461770 -d ./ -o tpc_idc.root --no-preserve-path root tpc_idc.root gFile->Get<o2::tpc::CalDet<o2::tpc::PadFlags>>("ccdb_object") ``` correctly executes the custom streamer function. Note that there is also no need to make the code ROOT version dependent. We need to fix the reading in any case and the writing will just stay the same. Concerning situations, where future classes will write data containing std::vector<o2::tpc::PadFlags> we should be protected by the fact that this bug has been fixed >= ROOT 6.36 in any case. This commit relates also to root-project/root#17009 The commit also re-enables dictionary creation of related classes and adds a dictionary for CalArray<o2::tpc::PadFlags> previously missing.
sawenzel
added a commit
to sawenzel/AliceO2
that referenced
this pull request
Nov 18, 2025
This commit expands on AliceO2Group#14427 and fixes the issue brought up in https://its.cern.ch/jira/browse/O2-4671. After debugging/testing it turns out that the approach taken via a customer streamer for std::vector<o2::tpc::PadFlags> does not take effect in the ROOT/IO because apparently ROOT still prefers to use the CollectionProxy for std::vector and does not employ the custom streamer. Instead, after discussion with @pcanal, this commit proposes to implement a custom stream just for the mData data member of CalArray<o2::tpc::PadFlags>. This is the only place where we use o2::tpc::PadFlags in IO and it fixes the problem when reading CCDB objects containing such data. I have verified that the following code ``` o2-ccdb-downloadccdbfile -p TPC/Calib/IDC_PadStatusMap_A -t 1731274461770 -d ./ -o tpc_idc.root --no-preserve-path root tpc_idc.root gFile->Get<o2::tpc::CalDet<o2::tpc::PadFlags>>("ccdb_object") ``` correctly executes the custom streamer function. Note that there is also no need to make the code ROOT version dependent. We need to fix the reading in any case and the writing will just stay the same. Concerning situations, where future classes will write data containing std::vector<o2::tpc::PadFlags> we should be protected by the fact that this bug has been fixed >= ROOT 6.36 in any case. This commit relates also to root-project/root#17009 The commit also re-enables dictionary creation of related classes and adds a dictionary for CalArray<o2::tpc::PadFlags> previously missing.
sawenzel
added a commit
that referenced
this pull request
Nov 19, 2025
This commit expands on #14427 and fixes the issue brought up in https://its.cern.ch/jira/browse/O2-4671. After debugging/testing it turns out that the approach taken via a customer streamer for std::vector<o2::tpc::PadFlags> does not take effect in the ROOT/IO because apparently ROOT still prefers to use the CollectionProxy for std::vector and does not employ the custom streamer. Instead, after discussion with @pcanal, this commit proposes to implement a custom stream just for the mData data member of CalArray<o2::tpc::PadFlags>. This is the only place where we use o2::tpc::PadFlags in IO and it fixes the problem when reading CCDB objects containing such data. I have verified that the following code ``` o2-ccdb-downloadccdbfile -p TPC/Calib/IDC_PadStatusMap_A -t 1731274461770 -d ./ -o tpc_idc.root --no-preserve-path root tpc_idc.root gFile->Get<o2::tpc::CalDet<o2::tpc::PadFlags>>("ccdb_object") ``` correctly executes the custom streamer function. Note that there is also no need to make the code ROOT version dependent. We need to fix the reading in any case and the writing will just stay the same. Concerning situations, where future classes will write data containing std::vector<o2::tpc::PadFlags> we should be protected by the fact that this bug has been fixed >= ROOT 6.36 in any case. This commit relates also to root-project/root#17009 The commit also re-enables dictionary creation of related classes and adds a dictionary for CalArray<o2::tpc::PadFlags> previously missing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.