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.

Sign up for GitHub

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

Jump to bottom

ZDC data model change #10973

Merged
merged 25 commits into from
Apr 6, 2023
Merged

ZDC data model change #10973

merged 25 commits into from
Apr 6, 2023

Conversation

ddobrigk
Copy link
Contributor

@cortesep @coppedis @jgrosseo up for discussion / approval! Thanks :-D

aalkin
aalkin reviewed Mar 19, 2023
View reviewed changes
Detectors/AOD/src/AODProducerWorkflowSpec.cxx Outdated
@@ -2522,7 +2573,11 @@ DataProcessorSpec getAODProducerWorkflowSpec(GID::mask_t src, bool enableSV, boo
outputs.emplace_back(OutputLabel{"O2ambiguousMFTtrack"}, "AOD", "AMBIGUOUSMFTTR", 0, Lifetime::Timeframe);
outputs.emplace_back(OutputLabel{"O2ambiguousFwdtrack"}, "AOD", "AMBIGUOUSFWDTR", 0, Lifetime::Timeframe);
outputs.emplace_back(OutputLabel{"O2v0_001"}, "AOD", "V0_001", 0, Lifetime::Timeframe);
#ifdef O2_ZDC_NEWDATAMODEL
outputs.emplace_back(OutputLabel{"O2zdc_001"}, "AOD", "ZDC_001", 0, Lifetime::Timeframe);
Copy link
Member

@aalkin aalkin Mar 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is incorrect, it should be "ZDC", 1, the version for AOD tables is encoded in subspec.
Update: the same goes for V0 table above, the only reason this works is that AOD producer's output is not used directly by tasks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, this is wrong for all of them. See v0, cascade, collision etc. So it seems to work but is not good of course...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall we change this everywhere? Or we keep doing as is? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Probably better to do the rest in a separate PR, for bookkeeping reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the rest or all of them? What do you think? :-D

Copy link
Member

Choose a reason for hiding this comment

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

ZDC should be done correctly from the start here, I think. The rest can be fixed elsewhere so we do not mix them with ZDC-specific change.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we include in this PR all the fixes for the ZDC and then another request will be open for the remaining issues, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, the latest PR does as you suggested, @aalkin ! Can you please check if I did no do anything major wrong? (technically, though, it's in the part of the producer that is as-of-yet disabled, but I want to be sure it's ok already). If this is fine I would suggest we go ahead and merge so I can do a PR for the O2Physics part. Thanks!!!

jgrosseo
jgrosseo reviewed Mar 20, 2023
View reviewed changes
Copy link
Collaborator

@jgrosseo jgrosseo left a comment

Choose a reason for hiding this comment

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

I have posted a few comments for cosmetic changes but the rest is fine with me

@ddobrigk ddobrigk marked this pull request as ready for review April 3, 2023 15:41
@ddobrigk ddobrigk requested review from a team and shahor02 as code owners April 3, 2023 15:41
@ddobrigk
Copy link
Contributor Author

ddobrigk commented Apr 3, 2023

Marked as ready to review to trigger all checks already (while also waiting for @aalkin 's green light). Thanks!

@alibuild
Copy link
Collaborator

alibuild commented Apr 4, 2023

Error while checking build/O2/fullCI for a182c90 at 2023-04-04 03:35:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/10973-slc8_x86-64/0/PWGUD/DataModel/UDTables.h:333:41: error: use of class template 'zdc::EnergyZEM1' requires template arguments
/sw/SOURCES/O2Physics/10973-slc8_x86-64/0/PWGUD/DataModel/UDTables.h:332:19: error: unknown type name 'UDZdcs'
/sw/SOURCES/O2Physics/10973-slc8_x86-64/0/PWGUD/DataModel/UDTables.h:332:19: error: use of undeclared identifier 'UDZdcs'
/sw/SOURCES/O2Physics/10973-slc8_x86-64/0/PWGUD/DataModel/UDTables.h:332:19: error: use of undeclared identifier 'UDZdcs'
/sw/SOURCES/O2Physics/10973-slc8_x86-64/0/PWGUD/DataModel/UDTables.h:338:15: error: use of undeclared identifier 'UDZdcs'
Error: /sw/slc8_x86-64/ROOT/v6-28-02-1/bin/rootcling: compilation failure (/sw/BUILD/0c695b43eec01ddf408f683d421d845399d58b75/O2Physics/PWGUD/Core/G__O2PhysicsDGPIDSelector9b848d9579_dictUmbrella.h)
ninja: build stopped: subcommand failed.

Full log here.

jgrosseo
jgrosseo previously approved these changes Apr 4, 2023
View reviewed changes
@jgrosseo
Copy link
Collaborator

jgrosseo commented Apr 4, 2023

Is the PR for the converter already there (as draft)?

@ddobrigk
Copy link
Contributor Author

ddobrigk commented Apr 4, 2023

Is the PR for the converter already there (as draft)?

I don't think I PR-ed it but I have it in my fork here. I will do a PR as soon as this is merged; anyway now I see some CI errors that I have to look into for O2Physics. More soon...

aalkin
aalkin reviewed Apr 4, 2023
View reviewed changes
Framework/Core/include/Framework/AnalysisDataModel.h Outdated
@@ -728,12 +731,317 @@ DECLARE_SOA_COLUMN(TimeZNC, timeZNC, float); //!
DECLARE_SOA_COLUMN(TimeZPA, timeZPA, float); //!
DECLARE_SOA_COLUMN(TimeZPC, timeZPC, float); //!
} // namespace zdc
namespace zdc_001 // Revised table, required for dynamic in-place replacements
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same namespace, the versioning should only be done at the table level, otherwise you will have to rename the namespace later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Anton, as explained in the comment inside the code, I need to have different namespaces to have different types of columns with the same names: we are swapping out a regular column with a dynamic column in the new version of the table, and these have the same exact name. This cannot be done keeping the same namespace, unfortunately...

Copy link
Member

Choose a reason for hiding this comment

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

Since the columns will have different type, they may as well have a different name. In fact, you can rename old columns (only the type names, not the getters). This will be transparent for the users, since they are only using the getters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest commit should take care: it uses only the zdc namespace but changes the column names for the dynamic columns while keeping the same getters. Can you please take a look @aalkin ? Thanks a lot!

@alibuild
Copy link
Collaborator

alibuild commented Apr 4, 2023

Error while checking build/O2/fullCI for b8f8703 at 2023-04-04 15:10:

## sw/BUILD/o2codechecker-latest/log
100% tests passed, 0 tests failed out of 1


## sw/BUILD/O2-full-system-test-latest/log
Detected critical problem in logfile reco_NOGPU.log
reco_NOGPU.log:[ERROR] Workflow crashed - pid 9141 (gpu-reconstruction) was killed abnormally with exit code 128, could be out of memory killer, segfault, unhandled exception, SIGKILL, etc...
[9158:BadMapCalibSpec]: [13:03:11][ERROR] Insufficient statistics: 5 entries in lowE histo, do nothing
[ERROR] Workflow crashed - pid 9141 (gpu-reconstruction) was killed abnormally with exit code 128, could be out of memory killer, segfault, unhandled exception, SIGKILL, etc...


## sw/BUILD/o2checkcode-latest/log
--
++ echo '========== List of errors found =========='
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
++ GRERR=1
++ [[ 1 == 0 ]]
++ mkdir -p /sw/INSTALLROOT/5f1533df31df881f07b05e8163927c4b24e5c9eb/slc8_x86-64/o2checkcode/1.0-local1/etc/modulefiles
++ cat
--

Full log here.

@alibuild
Copy link
Collaborator

alibuild commented Apr 4, 2023

Error while checking build/O2/fullCI for aa608f9 at 2023-04-04 22:05:

## sw/BUILD/O2-full-system-test-latest/log
task timeout reached .. killing all processes
[31159:BadMapCalibSpec]: [19:32:45][ERROR] Insufficient statistics: 5 entries in lowE histo, do nothing


## sw/BUILD/o2checkcode-latest/log
--
++ echo '========== List of errors found =========='
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
++ GRERR=1
++ [[ 1 == 0 ]]
++ mkdir -p /sw/INSTALLROOT/c69f1e9dd9fa7b14964b97859252eb28d3a13ca3/slc8_x86-64/o2checkcode/1.0-local8/etc/modulefiles
++ cat
--

Full log here.

@ddobrigk
Copy link
Contributor Author

ddobrigk commented Apr 5, 2023

Okay, so the latest CI errors seem unrelated and the code does compile in the regular tests... Now I would say we just need a confirmation with @aalkin to see if everything is in order and then we can merge and proceed to the converter part: I will check if there is maybe any small update that may be required on that side and prepare a PR. Thanks a lot, everyone!!

@aalkin
Copy link
Member

aalkin commented Apr 5, 2023

I have no additional comments, @ddobrigk. Although you may want to produce some new format data files first and test them with O2Physics.

@ddobrigk
Copy link
Contributor Author

ddobrigk commented Apr 5, 2023

I have no additional comments, @ddobrigk. Although you may want to produce some new format data files first and test them with O2Physics.

Yes, this is a good idea. We did it already with the previous version, but we can redo it. For now, because the data model changes will only kick in after the define is uncommented (or, better, the code cleaned up from the previous versions), I suggest we merge and then do the test simultaneously; then the necessary condition to move to the new table in O2 (after also the O2Physics converter PR is merged) is to have the latest code tested.

@jgrosseo
Copy link
Collaborator

jgrosseo commented Apr 5, 2023

I think the CI is unrelated. @ktf can you confirm and then merge?

@ddobrigk
Copy link
Contributor Author

ddobrigk commented Apr 6, 2023 •
edited
Loading

All tests are successful and we should really move on - @shahor02 or @ktf , could you please help us out? I think this is ready to go and agreed upon. Thanks!

@shahor02
Copy link
Collaborator

shahor02 commented Apr 6, 2023

I'll run the fullCI locally, will merge if ok.

@shahor02
Copy link
Collaborator

shahor02 commented Apr 6, 2023

passed fullCI locally, merging

@shahor02 shahor02 merged commit 15e36f6 into AliceO2Group:dev Apr 6, 2023
@ddobrigk
Copy link
Contributor Author

ddobrigk commented Apr 6, 2023

Thank you very much @shahor02 ! I will prepare the O2Physics PR shortly. Have a nice easter break!

@ddobrigk ddobrigk deleted the zdcdevel branch April 8, 2023 18:51
@jgrosseo jgrosseo mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coppedis coppedis coppedis left review comments

@aalkin aalkin aalkin left review comments

@jgrosseo jgrosseo jgrosseo approved these changes

@shahor02 shahor02 Awaiting requested review from shahor02 shahor02 is a code owner

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ddobrigk @alibuild @jgrosseo @aalkin @shahor02 @coppedis @cortesep