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

DPL Analysis: provide metadata to workflow construction #13130

Merged
merged 1 commit into from
May 16, 2024

Conversation

ktf
Copy link
Member

@ktf ktf commented May 13, 2024

DPL Analysis: provide metadata to workflow construction

@ktf ktf requested a review from a team as a code owner May 13, 2024 07:48
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass3
async-2023-pbpb-apass4
async-2022-pp-apass6-2023-PbPb-apass2
async-2022-pp-apass4
async-2022-pp-apass4-accepted
async-2022-pp-apass6-2023-PbPb-apass2-accepted
async-2023-pbpb-apass3-accepted
async-2023-pbpb-apass4-accepted
async-2023-pp-apass4
async-2023-pp-apass4-accepted
async-2024-pp-apass1
async-2024-pp-apass1-accepted
async-2022-pp-apass7
async-2022-pp-apass7-accepted
async-2024-pp-cpass0
async-2024-pp-cpass0-accepted

@jgrosseo
Copy link
Collaborator

Very nice! :-)

@ktf
Copy link
Member Author

ktf commented May 13, 2024

Tested locally with the simple test and indeed it now does not load ROOT once it finds out the options and injects them in the topology. Now testing on hyperloop.

@ktf ktf force-pushed the pr13130 branch 2 times, most recently from d500f5b to 38f76e0 Compare May 14, 2024 09:30
@alibuild
Copy link
Collaborator

alibuild commented May 15, 2024

Error while checking build/O2/fullCI for 38f76e0 at 2024-05-15 16:33:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
/sw/SOURCES/O2/13130-slc8_x86-64/0/Framework/Core/include/Framework/ASoA.h:3115:81: error: invalid use of incomplete type 'typename table_t::RowViewFiltered<Filtered<Join<aod::Tracks, aod::TracksExtra>>, TracksExtension, Table<Index<0, -1>, CollisionId, TrackType, X, Alpha, Y, Z, Snp, Tgl, Signed1Pt, IsWithinBeamPipe<X>, Px<Signed1Pt, Snp, Alpha>, Py<Signed1Pt, Snp, Alpha>, Pz<Signed1Pt, Tgl>, PVector<Signed1Pt, Snp, Alpha, Tgl>, Energy<Signed1Pt, Tgl>, Rapidity<Signed1Pt, Tgl>, Sign<Signed1Pt>, Marker<1>>, TracksExtra_001Extension, Table<TPCInnerParam, Flags, ITSClusterSizes, TPCNClsFindable, TPCNClsFindableMinusFound, TPCNClsFindableMinusCrossedRows, TPCNClsShared, TRDPattern, ITSChi2NCl, TPCChi2NCl, TRDChi2, TOFChi2, TPCSignal, TRDSignal, Length, TOFExpMom, PIDForTracking<Flags>, IsPVContributor<Flags>, HasITS<DetectorMap>, HasTPC<DetectorMap>, HasTRD<DetectorMap>, HasTOF<DetectorMap>, TPCNClsFound<TPCNClsFindable, TPCNClsFindableMinusFound>, TPCNClsCrossedRows<TPCNClsFindable, TPCNClsFindableMinusCrossedRows>, ITSClusterMap<ITSClusterSizes>, ITSNCls<ITSClusterSizes>, ITSNClsInnerBarrel<ITSClusterSizes>, ITSClsSizeInLayer<ITSClusterSizes>, TPCCrossedRowsOverFindableCls<TPCNClsFindable, TPCNClsFindableMinusCrossedRows>, TPCFoundOverFindableCls<TPCNClsFindable, TPCNClsFindableMinusFound>, TPCFractionSharedCls<TPCNClsShared, TPCNClsFindable, TPCNClsFindableMinusFound>, TrackEtaEMCAL, TrackPhiEMCAL, TrackTime, TrackTimeRes>>' (aka 'RowViewBase<o2::soa::FilteredIndexPolicy, o2::soa::Filtered<o2::soa::Join<aod::Tracks, aod::TracksExtra>>, o2::aod::TracksExtension, o2::soa::Table<o2::soa::Index<0, -1>, o2::aod::track::CollisionId, o2::aod::track::TrackType, o2::aod::track::X, o2::aod::track::Alpha, o2::aod::track::Y, o2::aod::track::Z, o2::aod::track::Snp, o2::aod::track::Tgl, o2::aod::track::Signed1Pt, o2::aod::track::IsWithinBeamPipe<o2::aod::track::X>, o2::aod::track::Px<o2::aod::track::Signed1Pt, o2::aod::track::Snp, o2::aod::track::Alpha>, o2::aod::track::Py<o2::aod::track::Signed1Pt, o2::aod::track::Snp, o2::aod::track::Alpha>, o2::aod::track::Pz<o2::aod::track::Signed1Pt, o2::aod::track::Tgl>, o2::aod::track::PVector<o2::aod::track::Signed1Pt, o2::aod::track::Snp, o2::aod::track::Alpha, o2::aod::track::Tgl>, o2::aod::track::Energy<o2::aod::track::Signed1Pt, o2::aod::track::Tgl>, o2::aod::track::Rapidity<o2::aod::track::Signed1Pt, o2::aod::track::Tgl>, o2::aod::track::Sign<o2::aod::track::Signed1Pt>, o2::soa::Marker<1>>, o2::aod::TracksExtra_001Extension, o2::soa::Table<o2::aod::track::TPCInnerParam, o2::aod::track::Flags, o2::aod::track::ITSClusterSizes, o2::aod::track::TPCNClsFindable, o2::aod::track::TPCNClsFindableMinusFound, o2::aod::track::TPCNClsFindableMinusCrossedRows, o2::aod::track::TPCNClsShared, o2::aod::track::TRDPattern, o2::aod::track::ITSChi2NCl, o2::aod::track::TPCChi2NCl, o2::aod::track::TRDChi2, o2::aod::track::TOFChi2, o2::aod::track::TPCSignal, o2::aod::track::TRDSignal, o2::aod::track::Length, o2::aod::track::TOFExpMom, o2::aod::track::PIDForTracking<o2::aod::track::Flags>, o2::aod::track::IsPVContributor<o2::aod::track::Flags>, o2::aod::track::HasITS<o2::aod::track::v001::DetectorMap>, o2::aod::track::HasTPC<o2::aod::track::v001::DetectorMap>, o2::aod::track::HasTRD<o2::aod::track::v001::DetectorMap>, o2::aod::track::HasTOF<o2::aod::track::v001::DetectorMap>, o2::aod::track::TPCNClsFound<o2::aod::track::TPCNClsFindable, o2::aod::track::TPCNClsFindableMinusFound>, o2::aod::track::TPCNClsCrossedRows<o2::aod::track::TPCNClsFindable, o2::aod::track::TPCNClsFindableMinusCrossedRows>, o2::aod::track::v001::ITSClusterMap<o2::aod::track::ITSClusterSizes>, o2::aod::track::v001::ITSNCls<o2::aod::track::ITSClusterSizes>, o2::aod::track::v001::ITSNClsInnerBarrel<o2::aod::track::ITSClusterSizes>, o2::aod::track::v001::ITSClsSizeInLayer<o2::aod::track::ITSClusterSizes>, o2::aod::track::TPCCrossedRowsOverFindableCls<o2::aod::track::TPCNClsFindable, o2::aod::track::TPCNClsFindableMinusCrossedRows>, o2::aod::track::TPCFoundOverFindableCls<o2::aod::track::TPCNClsFindable, o2::aod::track::TPCNClsFindableMinusFound/sw/SOURCES/O2/13130-slc8_x86-64/0/Framework/Core/include/Framework/AnalysisDataModel.h:709:18: note: in instantiation of template class 'o2::soa::Join<o2::aod::MFTTracks_001Extension, o2::soa::Table<o2::soa::Index<0, -1>, o2::aod::fwdtrack::CollisionId, o2::aod::fwdtrack::X, o2::aod::fwdtrack::Y, o2::aod::fwdtrack::Z, o2::aod::fwdtrack::Phi, o2::aod::fwdtrack::Tgl, o2::aod::fwdtrack::Signed1Pt, o2::aod::fwdtrack::v001::NClusters<o2::aod::fwdtrack::MFTClusterSizesAndTrackFlags>, o2::aod::fwdtrack::MFTClusterSizesAndTrackFlags, o2::aod::fwdtrack::IsCA<o2::aod::fwdtrack::MFTClusterSizesAndTrackFlags>, o2::aod::fwdtrack::Px<o2::aod::fwdtrack::Pt, o2::aod::fwdtrack::Phi>, o2::aod::fwdtrack::Py<o2::aod::fwdtrack::Pt, o2::aod::fwdtrack::Phi>, o2::aod::fwdtrack::Pz<o2::aod::fwdtrack::Pt, o2::aod::fwdtrack::Tgl>, o2::aod::fwdtrack::Sign<o2::aod::fwdtrack::Signed1Pt>, o2::aod::fwdtrack::Chi2, o2::aod::fwdtrack::TrackTime, o2::aod::fwdtrack::TrackTimeRes>>' requested here
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.

@ktf ktf force-pushed the pr13130 branch 2 times, most recently from 26ad8f3 to 1bb68dd Compare May 16, 2024 13:21
@ktf
Copy link
Member Author

ktf commented May 16, 2024

This version seems to behave on hyperloop as well. Will merge if the tests pass.

@ktf
Copy link
Member Author

ktf commented May 16, 2024

Merging this. Works on hyperloop and it should not have any effect if the metadata is not used. Moreover, changing one line (the list of capability plugins) should be enough to completely disable the new behavior.

@ktf ktf merged commit 52d98c8 into AliceO2Group:dev May 16, 2024
15 of 17 checks passed
@mhemmer-cern
Copy link
Contributor

Why the change from aod-file to aod-file-private?
Was this communicated somewhere because I did not see anything except @ddobrigk asking about this change in one of the hyperloop channels.

@ddobrigk
Copy link
Contributor

Hi everyone, let me also support @mhemmer-cern 's question... this is quite a complicated change because - at least at my workplace - quite a few people lost quite some time to figure out that this had happened (and in fact nobody replied to my message in the analysis channel :-( )...

@jgrosseo
Copy link
Collaborator

jgrosseo commented May 31, 2024

On Hyperloop, we still use aod-file. We didn't change anything. This private must have some internal reason and it is a hack to use it externally. But I let Giulio comment...

@mhemmer-cern
Copy link
Contributor

Hey @jgrosseo thanks for the reply. So the problem that I faced with this change is for local testing, which should affect pretty much everyone for local testing, since our json config files had aod-file as an option which no longer exists and needs to be changed to the new aod-file-private.
On Hyperloop when you run a train test the newly generated dpl-config.json has that change in it as well. That was how we found the solution.

@ddobrigk
Copy link
Contributor

Hi @jgrosseo my guess is you dodged that because the aod-file without private still works but only in the command line - in the json you have to write "private" or it will not work. My guess is the dpl-config is simply the system dump and since the system changed it started to dump the right thing after the change....

@jgrosseo
Copy link
Collaborator

@mhemmer-cern We do not use dpl-config.json on Hyperloop, it is generated by the running. And indeed, we use it on the command line.
@ktf should really comment...

@ktf
Copy link
Member Author

ktf commented Jun 3, 2024

indeed, the private you should ignore, why is it an issue? Can you point me to the a reproducer which breaks?

@mhemmer-cern
Copy link
Contributor

Hey @ktf,
the problem is with local test scripts that people used, where in the json config file one needs to change the aod-file to aod-file-private which was not public knowledge so people where confused and struggled with this.

@ktf
Copy link
Member Author

ktf commented Jun 3, 2024

While I cannot comment on how this private script itself works, I suspect what is happening here is indeed what @jgrosseo suggested. If the config gets cached across different releases there is no guarantee that will actually work: it might, it might not or it might silently misbehave. This is in particular true for workflow options like --aod-file. I will make sure I add that to the documentation, if missing.

My recommendation would be to change your script to always generate the config from scratch when switching releases. If you point me to the actual script I can probably be more precise.

That said, for this specific case I think I can now get rid of the aod-file-private option completely and in case I do, I will make sure I will advertise it appropriately in the WP4 - WP14 meeting.

@ktf ktf deleted the pr13130 branch June 3, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants