[ntuple] DAOS object class customization (1/2): refactor `RDaosXxx` classes by jalopezg-git · Pull Request #8402 · root-project/root

jalopezg-git · 2021-06-10T14:29:55Z

The first in the series of two pull-requests to allow users to customize the DAOS object class.

This PR applies the required changes to the RDaosXxx classes. Also, libdaos_mock is extended to provide an implementation for the object class name to ID conversion functions.
Finally, unneeded OC_xxx constants were stripped from libdaos_mock/daos.h; this header should only contain declarations needed for testing.

Related to issue #8205.

phsft-bot · 2021-06-10T14:30:01Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

jblomer

Nice improvement!

tree/ntuple/v7/inc/ROOT/libdaos_mock/daos.h

phsft-bot · 2021-06-11T10:19:02Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-11T11:23:03Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-11T11:39:57Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-12T16:24:36Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

jalopezg-git · 2021-06-12T16:35:46Z

I made additional changes to one of the commits in the follow-up pull request. As this was not merged yet, I also touched the corresponding part here.

@jblomer, could you please review commit 6efaf8e, i.e. the latest one? Let me know if you have any suggestion.

phsft-bot · 2021-06-12T16:36:10Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-06-12T16:36:05.301Z] FAILED: tree/ntuple/CMakeFiles/ROOTNTuple.dir/v7/src/RDaos.cxx.o
[2021-06-12T16:36:06.803Z] /home/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RDaos.hxx:106:90: error: default member initializer for ‘ROOT::Experimental::Detail::RDaosObject::ObjClassId::fCid’ required before the end of its enclosing class

phsft-bot · 2021-06-12T16:44:46Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-06-12T16:44:42.096Z] FAILED: tree/ntuple/CMakeFiles/ROOTNTuple.dir/v7/src/RDaos.cxx.o
[2021-06-12T16:44:42.813Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RDaos.hxx:72:7: error: default member initializer for 'fCid' needed within definition of enclosing class 'RDaosObject' outside of member functions
[2021-06-12T16:44:42.813Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RDaos.hxx:75:74: error: implicit instantiation of undefined template 'std::__1::basic_string<char>'
[2021-06-12T16:44:43.940Z] FAILED: tree/ntuple/CMakeFiles/ROOTNTuple.dir/v7/src/RPageStorageDaos.cxx.o
[2021-06-12T16:44:43.940Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RDaos.hxx:72:7: error: default member initializer for 'fCid' needed within definition of enclosing class 'RDaosObject' outside of member functions

phsft-bot · 2021-06-12T16:50:04Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-12T17:10:56Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-06-12T17:10:50.260Z] FAILED: tree/ntuple/CMakeFiles/ROOTNTuple.dir/v7/src/RDaos.cxx.o
[2021-06-12T17:10:50.567Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RDaos.hxx:74:74: error: implicit instantiation of undefined template 'std::__1::basic_string<char>'

phsft-bot · 2021-06-12T17:21:29Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-12T19:29:00Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-12T20:42:42Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.gui.webdisplay.test.test_webgui_ping

phsft-bot · 2021-06-14T11:49:06Z

Didn't recognize ROOT-debian-i386/cxx14 aborting build.

jalopezg-git · 2021-06-14T11:50:20Z

@phsft-bot build just on ROOT-debian10-i386/cxx14

phsft-bot · 2021-06-14T11:50:26Z

Starting build on ROOT-debian10-i386/cxx14
How to customize builds

jblomer

I left a review on 6efaf8e

jalopezg-git · 2021-06-14T16:50:41Z

That's pretty dangerous. Who know how long class names will be in the future. The implementation just seems to use strcpy. I think that's something we can discuss with Johann -- if there is a DAOS defined limit that we can use. I'd very much prefer that. Lacking such a limit, I'd use a much larger one for ours, e.g. 64 or 128.

I completely agree, that's why I left a comment and asked you to re-review ;-) I will ask Johann, or otherwise use a larger buffer.

…name2id,id2name}` Provides mocks for the oclass name to id conversion functions.

libdaos_mock/daos.h shall be kept minimal, i.e. it should only contain the declarations required by the unit tests.

These changes affect the functions to read/write a single attribute key. In particular, DAOS-specific arguments appear last in the function signature. Other arguments, e.g. object class may be added in the future.

Helper functions to read/write data, e.g. {Read,Write}SingleAkey allow for specifying an object class ID. A default oclass may be specified for an RDaosContainer intance via the `SetDefaultObjectClass()` function.

phsft-bot · 2021-06-15T09:43:13Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-06-15T10:55:00Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2021-06-15T10:50:23.692Z] C:\build\workspace\root-pullrequests-build\build\test\Release\stressRooFit.exe : fatal error LNK1120: 1 unresolved externals [C:\build\workspace\root-pullrequests-build\build\test\stressRooFit.vcxproj]
[2021-06-15T10:53:11.745Z] C:\build\workspace\root-pullrequests-build\build\roofit\roofitcore\test\Release\testRooAbsPdf.exe : fatal error LNK1120: 1 unresolved externals [C:\build\workspace\root-pullrequests-build\build\roofit\roofitcore\test\testRooAbsPdf.vcxproj]
[2021-06-15T10:54:11.569Z] C:\build\workspace\root-pullrequests-build\build\bin\libRooStats.dll : fatal error LNK1120: 1 unresolved externals [C:\build\workspace\root-pullrequests-build\build\roofit\roostats\RooStats.vcxproj]

jblomer

Could you pinpoint what's the change since the last review? In general, I think it's easier if you add on commits rather than force push an updated history.

jalopezg-git · 2021-06-16T07:20:46Z

Could you pinpoint what's the change since the last review? In general, I think it's easier if you add on commits rather than force push an updated history.

I have updated the value (and documentation) of the kOCNameMaxLength constant after exchanging a couple of e-mails. Sorry, I squashed the commits as if the PR history was going to be merged, but I agree it is harder for you to review! For subsequent PRs, I will add independent commits and will do the interactive rebase only after the review. :-)

jalopezg-git · 2021-06-16T12:37:30Z

@phsft-bot build just on windows10/cxx14

phsft-bot · 2021-06-16T12:37:35Z

Starting build on windows10/cxx14
How to customize builds

jalopezg-git added improvement in:RNTuple labels Jun 10, 2021

jalopezg-git requested review from jblomer and mxxo June 10, 2021 14:29

jalopezg-git self-assigned this Jun 10, 2021

jalopezg-git requested a review from pcanal as a code owner June 10, 2021 14:29

jblomer requested changes Jun 11, 2021

View reviewed changes

tree/ntuple/v7/inc/ROOT/libdaos_mock/daos.h Show resolved Hide resolved

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from 768058f to 5152644 Compare June 11, 2021 10:18

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from c6a5887 to 041ae5d Compare June 11, 2021 11:39

jalopezg-git changed the title ~~[ntuple] DAOS object class customization (1/2): refactor RDaosXxx classes~~ [ntuple] DAOS object class customization (1/2): refactor RDaosXxx classes Jun 11, 2021

jalopezg-git requested a review from jblomer June 11, 2021 12:00

jblomer approved these changes Jun 11, 2021

View reviewed changes

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from 041ae5d to 97c90bb Compare June 12, 2021 16:24

jalopezg-git requested a review from jblomer June 12, 2021 16:27

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from 97c90bb to 8166f35 Compare June 12, 2021 16:49

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from 8166f35 to 521d771 Compare June 12, 2021 17:21

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from 521d771 to 6efaf8e Compare June 12, 2021 19:28

jblomer requested changes Jun 14, 2021

View reviewed changes

jalopezg-git added 4 commits June 15, 2021 11:24

[ntuple,daos] libdaos_mock: provide implementation for `daos_oclass_{…

f3fb720

…name2id,id2name}` Provides mocks for the oclass name to id conversion functions.

[ntuple] Remove unused OC_xxx constants from libdaos_mock/daos.h

6e4cab3

libdaos_mock/daos.h shall be kept minimal, i.e. it should only contain the declarations required by the unit tests.

[ntuple,daos] Minor changes in the RDaosContainer class interface

0a3184a

These changes affect the functions to read/write a single attribute key. In particular, DAOS-specific arguments appear last in the function signature. Other arguments, e.g. object class may be added in the future.

[ntuple,daos] RDaosContainer: allow specifying an object class

cdc9381

Helper functions to read/write data, e.g. {Read,Write}SingleAkey allow for specifying an object class ID. A default oclass may be specified for an RDaosContainer intance via the `SetDefaultObjectClass()` function.

jalopezg-git force-pushed the ntuple-daos-oclass-refactor branch from 6efaf8e to cdc9381 Compare June 15, 2021 09:43

jalopezg-git requested a review from jblomer June 15, 2021 10:03

jblomer reviewed Jun 15, 2021

View reviewed changes

jblomer approved these changes Jun 16, 2021

View reviewed changes

jalopezg-git merged commit 4bc21b8 into root-project:master Jun 16, 2021

jalopezg-git deleted the ntuple-daos-oclass-refactor branch June 16, 2021 19:08

jalopezg-git mentioned this pull request Jun 16, 2021

[ntuple] DAOS object class customization (2/2): provide RNTupleWriteOptionsDaos #8452

Merged

Conversation

jalopezg-git commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Jun 10, 2021

Uh oh!

jblomer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phsft-bot commented Jun 11, 2021

Uh oh!

phsft-bot commented Jun 11, 2021

Uh oh!

phsft-bot commented Jun 11, 2021

Uh oh!

phsft-bot commented Jun 12, 2021

Uh oh!

jalopezg-git commented Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Jun 12, 2021

Errors:

Uh oh!

phsft-bot commented Jun 12, 2021

Errors:

Uh oh!

phsft-bot commented Jun 12, 2021

Uh oh!

phsft-bot commented Jun 12, 2021

Errors:

Uh oh!

phsft-bot commented Jun 12, 2021

Uh oh!

phsft-bot commented Jun 12, 2021

Uh oh!

phsft-bot commented Jun 12, 2021

Failing tests:

Uh oh!

phsft-bot commented Jun 14, 2021

Uh oh!

jalopezg-git commented Jun 14, 2021

Uh oh!

phsft-bot commented Jun 14, 2021

Uh oh!

jblomer left a comment

Choose a reason for hiding this comment

Uh oh!

jalopezg-git commented Jun 14, 2021

Uh oh!

phsft-bot commented Jun 15, 2021

Uh oh!

phsft-bot commented Jun 15, 2021

Errors:

Uh oh!

jblomer left a comment

Choose a reason for hiding this comment

Uh oh!

jalopezg-git commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jalopezg-git commented Jun 16, 2021

Uh oh!

phsft-bot commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jalopezg-git commented Jun 10, 2021 •

edited

Loading

jalopezg-git commented Jun 12, 2021 •

edited

Loading

jalopezg-git commented Jun 16, 2021 •

edited

Loading