Add compatibility with new GRP scheme by njacazio · Pull Request #976 · AliceO2Group/O2Physics

njacazio · 2022-07-12T10:11:35Z

alibuild · 2022-07-12T10:56:11Z

Error while checking build/O2Physics/o2 for cb8739e at 2022-07-12 12:56:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/976-slc7_x86-64/0/Common/TableProducer/trackPropagation.cxx:115:161: error: 'timestamp' was not declared in this scope
ninja: build stopped: subcommand failed.

Full log here.

- This is nasty because it was hiding in the LOGF(info, ..) function

njacazio · 2022-07-14T10:07:12Z

Hi @ddobrigk could you have a look please? I think that the approvers might be on leave, how should we merge this?

mfaggin · 2022-07-18T07:11:29Z

@jgrosseo @iarsene @ktf are there news about this PR? This looks fundamental to work with on AO2Ds with LHC22e. Thank you.

ktf · 2022-07-18T11:21:28Z

+        if (!grpmag) {
+          LOG(fatal) << "Got nullptr from CCDB for path " << grpmagPath << " of object GRPMagField and " << grpPath << " of object GRPObject for timestamp " << bc.timestamp();
+        }
+        LOGF(info, "Setting magnetic field to current %d A for run %d from its GRPMagField CCDB object", grpmag->getL3Current(), bc.runNumber());


I would use the "detail" level for log by default, unless you really care about having the message during real production.

I agree, however this is the same level of info that was present before, this is also important information that should be printed once per run i.e. once per job ideally

ktf · 2022-07-18T11:22:15Z

This looks good on my side.

ktf · 2022-07-18T11:23:19Z

  Configurable<std::string> grpPath{"grpPath", "GLO/GRP/GRP", "Path of the grp file"};
+  Configurable<std::string> grpmagPath{"grpmagPath", "GLO/Config/GRPMagField", "CCDB path of the GRPMagField object"};
  Configurable<std::string> mVtxPath{"mVtxPath", "GLO/Calib/MeanVertex", "Path of the mean vertex file"};
+  Configurable<bool> checkGRPObject{"checkGRPObject", false, "Flag to check the CCDB for GRPObject before asking for the GRPMagField"};


actually, why not simply making it empty by default and use it if not?

Because I want to have the feature that the CCDB will not be checked and will get directly the new structure.
This is because the old GRP structure will be translated to the new one

njacazio

Thanks @ktf for the suggestions, however, I would not change the code. Can we merge it as it is now? This is a blocker for other projects

njacazio · 2022-07-21T07:23:01Z

  Configurable<std::string> grpPath{"grpPath", "GLO/GRP/GRP", "Path of the grp file"};
+  Configurable<std::string> grpmagPath{"grpmagPath", "GLO/Config/GRPMagField", "CCDB path of the GRPMagField object"};
  Configurable<std::string> mVtxPath{"mVtxPath", "GLO/Calib/MeanVertex", "Path of the mean vertex file"};
+  Configurable<bool> checkGRPObject{"checkGRPObject", false, "Flag to check the CCDB for GRPObject before asking for the GRPMagField"};


Because I want to have the feature that the CCDB will not be checked and will get directly the new structure.
This is because the old GRP structure will be translated to the new one

njacazio · 2022-07-21T07:23:59Z

+        if (!grpmag) {
+          LOG(fatal) << "Got nullptr from CCDB for path " << grpmagPath << " of object GRPMagField and " << grpPath << " of object GRPObject for timestamp " << bc.timestamp();
+        }
+        LOGF(info, "Setting magnetic field to current %d A for run %d from its GRPMagField CCDB object", grpmag->getL3Current(), bc.runNumber());


I agree, however this is the same level of info that was present before, this is also important information that should be printed once per run i.e. once per job ideally

mfaggin · 2022-07-25T07:09:41Z

Thanks @ktf for the suggestions, however, I would not change the code. Can we merge it as it is now? This is a blocker for other projects

thanks @njacazio , do you think we can proceed @ktf ?

mfaggin · 2022-07-26T12:25:20Z

Thanks @ktf for the suggestions, however, I would not change the code. Can we merge it as it is now? This is a blocker for other projects

thanks @njacazio , do you think we can proceed @ktf ?

A gentle reminder

njacazio · 2022-07-26T12:25:50Z

@jgrosseo @iarsene if you're back could you please check/approve?

jgrosseo · 2022-07-26T12:41:17Z

When will the old scheme be deprecated?

njacazio · 2022-07-26T12:42:16Z

I think it might be already, I have to check with Victor

mfaggin · 2022-07-26T12:43:12Z

When will the old scheme be deprecated?

@victor-gonzalez can confirm, but if I am not wrong this is already deprecated, apart for Run 2 converted data

victor-gonzalez · 2022-07-26T13:12:41Z

For Run 2 converted datasets the old scheme will stay
For Run 3 it seems the new schema has been populated with OCT Pilot beam and with the simulations
JUN Pilot beam and Run 3 datasets are only under the new schema

jgrosseo · 2022-07-26T13:41:24Z

Then I think the config parameter should be renamed to choose run 2 or 3...

njacazio · 2022-07-26T13:42:36Z

agreed

Add compatibility with new GRP scheme

b5c44bc

njacazio requested review from a team, alibuild, iarsene, jgrosseo and ktf as code owners July 12, 2022 10:11

Avoid fatal on non existing ccdb

cb8739e

njacazio added 2 commits July 12, 2022 14:06

Fix

dd8887d

Fix error invalid type specifier

b327d3c

- This is nasty because it was hiding in the LOGF(info, ..) function

ktf reviewed Jul 18, 2022

View reviewed changes

njacazio commented Jul 21, 2022

View reviewed changes

jgrosseo approved these changes Jul 26, 2022

View reviewed changes

jgrosseo merged commit bd48994 into AliceO2Group:master Jul 26, 2022

njacazio deleted the nj-newmag branch July 26, 2022 12:41

lbariogl pushed a commit to CF-tutorials/O2Physics that referenced this pull request Jul 28, 2022

Add compatibility with new GRP scheme (AliceO2Group#976)

23fb532

ktf self-requested a review August 3, 2022 07:16

Uh oh!

Conversation

njacazio commented Jul 12, 2022

Uh oh!

alibuild commented Jul 12, 2022

Uh oh!

njacazio commented Jul 14, 2022

Uh oh!

mfaggin commented Jul 18, 2022

Uh oh!

ktf Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

njacazio Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

ktf commented Jul 18, 2022

Uh oh!

ktf Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

njacazio Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

njacazio left a comment

Choose a reason for hiding this comment

Uh oh!

njacazio Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

njacazio Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

mfaggin commented Jul 25, 2022

Uh oh!

mfaggin commented Jul 26, 2022

Uh oh!

njacazio commented Jul 26, 2022

Uh oh!

jgrosseo commented Jul 26, 2022

Uh oh!

njacazio commented Jul 26, 2022

Uh oh!

mfaggin commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

victor-gonzalez commented Jul 26, 2022

Uh oh!

jgrosseo commented Jul 26, 2022

Uh oh!

njacazio commented Jul 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

mfaggin commented Jul 26, 2022 •

edited

Loading