Skip to content

AggregatedRunInfo struct proposal#13555

Merged
sawenzel merged 1 commit intoAliceO2Group:devfrom
sawenzel:swenzel/aggregatedRunInfo
Oct 3, 2024
Merged

AggregatedRunInfo struct proposal#13555
sawenzel merged 1 commit intoAliceO2Group:devfrom
sawenzel:swenzel/aggregatedRunInfo

Conversation

@sawenzel
Copy link
Copy Markdown
Collaborator

@sawenzel sawenzel commented Oct 2, 2024

We are often in the need to collect global properties of a run like run-duration, run-start, firstOrbit, lastOrbit, ...

No single CCDB source delivers all of these information. Moreover we sometimes have multiple sources for the same information (GRPECS, RCT/Info/RunInformation) which may not be at sync.

The proposal of this commit is to:

(a) introduce an aggregator struct "AggregatedRunInfo" in which we can
deliver all important information describing a Run.

(b) implement an authoritative and agreed-on method to fill this struct.

The goal is to set everyone (reco, MC, analysis) on the same foot/contract.

For now the struct is minimal for demonstration purposes.

We are often in the need to collect global properties of a run like
run-duration, run-start, firstOrbit, lastOrbit, ...

No single CCDB source delivers all of these information. Moreover we sometimes
have multiple sources for the same information (GRPECS, RCT/Info/RunInformation)
which may not be at sync.

The proposal of this commit is to:

(a) introduce an aggregator struct "AggregatedRunInfo" in which we can
    deliver all important information describing a Run.

(b) implement an authoritative and agreed-on method to fill this struct.

The goal is to set everyone (reco, MC, analysis) on the same foot/contract.

For now the struct is minimal for demonstration purposes.
@sawenzel sawenzel requested a review from shahor02 as a code owner October 2, 2024 12:01
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 2, 2024

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-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0

@sawenzel
Copy link
Copy Markdown
Collaborator Author

sawenzel commented Oct 2, 2024

Relates to discussion and issue described here: https://its.cern.ch/jira/browse/O2-5369

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Oct 2, 2024

Filling of this object will need to be implemented separately for the CCDBManager and DPL fetcher. For this latter I can simply expand the GRPGeomTGeo to provide this object.

@alibuild
Copy link
Copy Markdown
Collaborator

alibuild commented Oct 2, 2024

Error while checking build/O2/fullCI for 56eea56 at 2024-10-02 14:45:

## 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/13555-slc8_x86-64/0/DataFormats/Parameters/src/AggregatedRunInfo.cxx:53:39: error: non-constant-expression cannot be narrowed from type 'std::tuple_element<0, std::pair<long, long>>::type' (aka 'long') to 'uint64_t' (aka 'unsigned long') in initializer list [clang-diagnostic-c++11-narrowing]
/sw/SOURCES/O2/13555-slc8_x86-64/0/DataFormats/Parameters/src/AggregatedRunInfo.cxx:53:44: error: non-constant-expression cannot be narrowed from type 'std::tuple_element<1, std::pair<long, long>>::type' (aka 'long') to 'uint64_t' (aka 'unsigned long') in initializer list [clang-diagnostic-c++11-narrowing]
/sw/SOURCES/O2/13555-slc8_x86-64/0/DataFormats/Parameters/src/AggregatedRunInfo.cxx:53:63: error: non-constant-expression cannot be narrowed from type 'int64_t' (aka 'long') to 'uint64_t' (aka 'unsigned long') in initializer list [clang-diagnostic-c++11-narrowing]
/sw/SOURCES/O2/13555-slc8_x86-64/0/DataFormats/Parameters/src/AggregatedRunInfo.cxx:53:77: error: non-constant-expression cannot be narrowed from type 'int64_t' (aka 'long') to 'uint64_t' (aka 'unsigned long') in initializer list [clang-diagnostic-c++11-narrowing]
/sw/SOURCES/O2/13555-slc8_x86-64/0/DataFormats/Parameters/src/AggregatedRunInfo.cxx:53:87: error: non-constant-expression cannot be narrowed from type 'int64_t' (aka 'long') to 'uint64_t' (aka 'unsigned long') in initializer list [clang-diagnostic-c++11-narrowing]
++ [[ 0 == 0 ]]
++ exit 1
--

Full log here.


// determine a good timestamp to query OrbitReset for this run
// --> the middle of the run is very appropriate and safer than just sor
auto run_mid_timestamp = sor + (eor - sor) / 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not just (sor + eor)/2 ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah sure

@sawenzel
Copy link
Copy Markdown
Collaborator Author

sawenzel commented Oct 2, 2024

Filling of this object will need to be implemented separately for the CCDBManager and DPL fetcher. For this latter I can simply expand the GRPGeomTGeo to provide this object.

Mm... It's not really a CCDB object by itself ... but we could still ship it such of course. But couldn't the DPL fetcher simply call the existing code? In principle it would be nice not to have separate implementations.

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Oct 2, 2024

Filling of this object will need to be implemented separately for the CCDBManager and DPL fetcher. For this latter, I can simply expand the GRPGeomTGeo to provide this object.

Mm... It's not really a CCDB object by itself ... but we could still ship it such of course. But couldn't the DPL fetcher simply call the existing code? In principle, it would be nice not to have separate implementations.

Sure, I don't refer to a new CCDB object: with the DPL fetcher every device which needs your new structure would need to request a set of existing CCDB objects and then put them together. The GRPGeomTGeo allows requesting with a single line a set of CCDB objects: the utility takes care of adding the DPL inputs, extracting them and doing some pre-proprocessing. A particular case of such a pre-processing can be filling this new structure.

@sawenzel
Copy link
Copy Markdown
Collaborator Author

sawenzel commented Oct 3, 2024

@shahor02 : Thanks for the clarification. I agree, although the name GRPGeomTGeo for this tool is slighty confusing. Maybe we could come up with some improved one.

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Oct 3, 2024

Actually, it is GRPGeomHelper, not GRPGeomTGeo, the name reflects the set of objects it helps to fetch:
https://github.com/AliceO2Group/AliceO2/blob/dev/Detectors/Base/include/DetectorsBase/GRPGeomHelper.h

The DPL fetcher itself does not extract the objects from CCDB files (except for the OrbitReset), it just sends them as binary blobs to requestor devices.

@sawenzel
Copy link
Copy Markdown
Collaborator Author

sawenzel commented Oct 3, 2024

@shahor02 : Ok good. Are you going to take care of finishing the development and adding the directions you indicated?

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Oct 3, 2024

@sawenzel yes, will add it once you merge this PR.

@sawenzel sawenzel changed the title **WIP**: AggregatedRunInfo struct proposal AggregatedRunInfo struct proposal Oct 3, 2024
@sawenzel
Copy link
Copy Markdown
Collaborator Author

sawenzel commented Oct 3, 2024

Merging following @shahor02 verbal approval.

@sawenzel sawenzel merged commit dbfe3c6 into AliceO2Group:dev Oct 3, 2024
@sawenzel sawenzel deleted the swenzel/aggregatedRunInfo branch October 3, 2024 12:50
@sawenzel
Copy link
Copy Markdown
Collaborator Author

sawenzel commented Oct 3, 2024

Really sorry. I merged this too early not looking at the CI results. Will revert.

@alcaliva alcaliva added the async-2024-pp-apass1 Request porting to async-2024-pp-apass1 label Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

async-2024-pp-apass1 Request porting to async-2024-pp-apass1

Development

Successfully merging this pull request may close these issues.

5 participants