[ntuple] various interface improvements #14454

jblomer · 2024-01-26T13:39:09Z

Change raw pointer methods to shared pointers or const ref. Remove unsafe and unnecessary methods.

Add RNTupleModel::GenerateBulk().

phsft-bot · 2024-01-26T13:39:18Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

enirolf

Thanks! I have left two small comments.

enirolf · 2024-01-26T14:28:10Z

tree/ntuple/v7/inc/ROOT/RNTupleModel.hxx

                                   std::function<std::string(const std::string &)> mapping);
-
-   template <typename T>
-   T *Get(std::string_view fieldName) const


Would it be an idea to have a Get<T> (perhaps named differently) that returns a shared pointer to the object? The use case would be for type-erased fields. Right now the (only) way to do it is through ntuple->GetModel().GetDefaultEntry()->GetPtr<T>("fieldname"), which feels a bit unwieldy.

That's a possibility, a getter acting as a shortcut for GetModel().GetDefaultEntry()->GetPtr<T>. On the other hand, callers can could also first get the entry and then call (multiple times) entry.GetPtr<T>(). How about we see if users stumble across it and defer the addition of the method until then?

Yes fair enough, sounds good!

enirolf · 2024-01-26T14:30:39Z

tree/ntuple/v7/inc/ROOT/RNTupleModel.hxx

   /// set memory addresses to be serialized / deserialized
   std::unique_ptr<REntry> CreateBareEntry() const;
-   REntry *GetDefaultEntry() const;
+   Detail::RFieldBase::RBulk GenerateBulk(std::string_view fieldName) const;


Perhaps add some documentation

github-actions · 2024-01-26T18:23:45Z

Test Results

10 files 10 suites 2d 0h 39m 48s ⏱️
2 497 tests 2 497 ✅ 0 💤 0 ❌
23 867 runs 23 867 ✅ 0 💤 0 ❌

Results for commit 4eb071d.

♻️ This comment has been updated with latest results.

phsft-bot · 2024-01-26T20:27:00Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

enirolf

It looks good to me, but following from the discussion on shared pointers on Friday maybe someone else should also have a look.

hahnjo · 2024-01-29T08:53:14Z

I already had a quick look last week and didn't have any further comments (except maybe that GenerateBulk is first used in the next commit titled "return const ref from RNTupleReader::GetModel()" 🙈)

smuzaffar · 2024-01-29T14:02:00Z

@jblomer , we are trying to test latest root master changes in CMSSW and our build failed with errors like [a]. Looks like this change is causing cmssw to fail. I guess we need to update cmssw to use model.GetDefaultEntry()->GetPtr<T>(name) instead of model.Get<T>(name) ... right? What about addField<T>() calls?

We also want same cmssw code to work for ROOT 6.26 and above. So should we add some #if ROOT_VERSION_CODE >= ROOT_VERSION(6,31,0) blocks ?

[a] https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6f60ec/37085/build-logs/PhysicsTools/NanoAOD/log.html

  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/TriggerOutputFields.cc:126:38: error: 'class ROOT::Experimental::RNTupleModel' has no member named 'Get'
   126 |   const auto* existing_field = model.Get<bool>(name);
      |                                      ^~~
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/TriggerOutputFields.cc:126:42: error: expected primary-expression before 'bool'
   126 |   const auto* existing_field = model.Get<bool>(name);
      |                                          ^~~~
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc: In member function 'void NanoAODRNTupleOutputModule::CommonEventFields::createFields(ROOT::Experimental::RNTupleModel&)':
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc:77:28: error: expected primary-expression before '>' token
    77 |       model.AddField<UInt_t>("run", &m_run);
      |                            ^
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc:77:30: error: left operand of comma operator has no effect [-Werror=unused-value]
    77 |       model.AddField<UInt_t>("run", &m_run);
      |                              ^~~~~
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc:78:28: error: expected primary-expression before '>' token
    78 |       model.AddField<UInt_t>("luminosityBlock", &m_luminosityBlock);
      |                            ^
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc:78:30: error: left operand of comma operator has no effect [-Werror=unused-value]
    78 |       model.AddField<UInt_t>("luminosityBlock", &m_luminosityBlock);
      |                              ^~~~~~~~~~~~~~~~~
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc:79:35: error: expected primary-expression before '>' token
    79 |       model.AddField<std::uint64_t>("event", &m_event);
      |                                   ^
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_0_ROOT6_X_2024-01-28-2300/src/PhysicsTools/NanoAOD/plugins/rntuple/NanoAODRNTupleOutputModule.cc:79:37: error: left operand of comma operator has no effect [-Werror=unused-value]
    79 |       model.AddField<std::uint64_t>("event", &m_event);
      |                                     ^~~~~~~

smuzaffar · 2024-01-30T16:15:18Z

@jblomer @hahnjo any suggestion about #14454 (comment) ?

hahnjo · 2024-01-30T16:29:18Z

I guess we need to update cmssw to use model.GetDefaultEntry()->GetPtr<T>(name) instead of model.Get<T>(name) ... right?

Yes, but with yesterday's changes I think it will be model.GetDefaultEntry().GetPtr<T>(name) because GetDefaultEntry returns a reference now. In general, there will be some more interface changes coming in the near future...

What about addField<T>() calls?

That one is a bit more complicated; after a quick look, I think CommonEventFields should call MakeField, store the returned std::shared_ptr and use that one in fill(const edm::EventID& id).

smuzaffar · 2024-02-01T13:46:39Z

thanks @hahnjo , I have opened cms-sw/cmssw#43834

jblomer added 8 commits January 26, 2024 14:30

[ntuple] return reference from RNTupleModel::GetDefaultEntry()

98b5f5e

[ntuple] remove RNTupleModel::Get() shortcut

53673b7

[ntuple] return const ref from RNTupleReader::GetDescriptor

06cd9d0

[ntuple] add RNTupleModel::GenerateBulk()

a3a737c

[ntuple] return const ref from RNTupleReader::GetModel()

56cbd3a

[ntuple] remove REntry::fValuePtrs

776a8cc

[ntuple] pass field ref to REntry::AddValue

a35fa03

[ntuple] remove raw pointer version of RNTupleModel::AddField

6a4186c

jblomer added the in:RNTuple label Jan 26, 2024

jblomer requested review from enirolf, hahnjo, pcanal and vepadulano January 26, 2024 13:39

jblomer self-assigned this Jan 26, 2024

jblomer requested a review from bellenot as a code owner January 26, 2024 13:39

enirolf requested changes Jan 26, 2024

View reviewed changes

[ntuple] add doc and error handling to RNTupleModel::GenerateBulk

4eb071d

jblomer requested a review from enirolf January 26, 2024 21:21

enirolf approved these changes Jan 29, 2024

View reviewed changes

jblomer merged commit d424fcb into root-project:master Jan 29, 2024

jblomer deleted the ntuple-interface-19 branch January 29, 2024 09:00

smuzaffar mentioned this pull request Jan 29, 2024

[ROOT6] Updated root to tip of branch master cms-sw/cmsdist#8973

Closed

smuzaffar mentioned this pull request Feb 1, 2024

ROOT6 changes needed for RNTupleModel cms-sw/cmssw#43834

Merged

[ntuple] various interface improvements #14454

[ntuple] various interface improvements #14454

Uh oh!

Conversation

jblomer commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Jan 26, 2024

Uh oh!

enirolf left a comment

Choose a reason for hiding this comment

Uh oh!

enirolf Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

jblomer Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

enirolf Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

enirolf Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

phsft-bot commented Jan 26, 2024

Uh oh!

enirolf left a comment

Choose a reason for hiding this comment

Uh oh!

hahnjo commented Jan 29, 2024

Uh oh!

smuzaffar commented Jan 29, 2024

Uh oh!

smuzaffar commented Jan 30, 2024

Uh oh!

hahnjo commented Jan 30, 2024

Uh oh!

smuzaffar commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jblomer commented Jan 26, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024 •

edited

Loading