root icon indicating copy to clipboard operation
root copied to clipboard

Change in behaviour in `TTreeReaderValueBase::GetSetupStatus()`

Open ktf opened this issue 6 months ago • 15 comments

Check duplicate issues.

  • [x] Checked for duplicates

Description

We have another regression in 6-36-00, where an exception is not raised anymore when trying to open a TTree with no entries. Before an std::invalid_argument exception was raised, now we get:

381/552 Test #375: Detectors/MUON/MCH/Raw/Encoder/Digit/testDigitTreeReader.cxx .........................***Failed    3.14 sec
Error in <TTreeReaderValueBase::CreateProxy()>: The tree does not have a branch called MCHDigit. You could check with TTree::Print() for available branches.
Error in <TTreeReaderValueBase::CreateProxy()>: The tree does not have a branch called MCHROFRecords. You could check with TTree::Print() for available branches.
Error in <TTreeReaderValueBase::CreateProxy()>: The tree does not have a branch called MCHROFRecords. You could check with TTree::Print() for available branches.
Error in <TTreeReaderValueBase::CreateProxy()>: The tree does not have a branch called MCHDigit. You could check with TTree::Print() for available branches.
Error in <TTreeReaderValueBase::CreateProxy()>: The branch MCHDigit contains data of type vector<o2::mch::ROFRecord>. It cannot be accessed by a TTreeReaderValue<vector<o2::mch::Digit>>
Error in <TTreeReaderValueBase::CreateProxy()>: The branch MCHROFRecords contains data of type vector<o2::mch::Digit>. It cannot be accessed by a TTreeReaderValue<vector<o2::mch::ROFRecord>>
Running 10 test cases...
/Volumes/build/alice-ci-workdir/alidist-o2/sw/SOURCES/O2/5895/0/Detectors/MUON/MCH/Raw/Encoder/Digit/testDigitTreeReader.cxx:129: [1;31;49merror: in "DigitTreeReaderMustThrowIfNoEntry": exception std::invalid_argument expected but not raised[0;39;49m

Is this expected?

Reproducer

Create a ttree with two empty branches and try to read them.

ROOT version


| Welcome to ROOT 6.36.000 https://root.cern | | (c) 1995-2025, The ROOT Team; conception: R. Brun, F. Rademakers | | Built for macosxarm64 on Jun 03 2025, 21:57:49 | | From tags/6-36-000@6-36-000 | | With Apple clang version 17.0.0 (clang-1700.0.13.5) | | Try '.help'/'.?', '.demo', '.license', '.credits', '.quit'/'.q' |

Installation method

aliBuild

Operating system

macOS

Additional context

No response

ktf avatar Jun 03 '25 22:06 ktf

All the test does is:

    5     TTree noEntry("noEntry", "All branches correct but no entry");
    4     std::vector<o2::mch::Digit> digits;
    3     std::vector<o2::mch::ROFRecord> rofs;
    2     noEntry.Branch("MCHDigit", &digits);
    1     noEntry.Branch("MCHROFRecords", &rofs);
  129     BOOST_CHECK_THROW(DigitTreeReader dtr(&noEntry), std::invalid_argument);
    1     noEntry.Print();

ktf avatar Jun 03 '25 22:06 ktf

Hi @ktf ,

Thanks for the report. One thing that is puzzling for me is that I don't believe there's ever been a std::invalid_argument raised in tree/treeplayer (or in tree/tree) for that matter. So where was that exception coming from previously? Meanwhile I will try to run your reproducer (with a simple TTreeReader instead of DigitTreeReader) and see if I can see anything similar to what you report.

vepadulano avatar Jun 04 '25 06:06 vepadulano

As a side note, there are quite a few cases I'm aware of where it can happen that a TTree with the right dataset schema but 0 entries is processed in the pipeline and this shouldn't break the execution

vepadulano avatar Jun 04 '25 07:06 vepadulano

Indeed, I guess the invalid_argument comes from the DigitReader reacting at the empty file. While I agree that a 0 entry TTree should not be particularly dramatic, if something external previously raised an exception and now it does not anymore, I would consider it a change in behaviour of the API. I am investigating what actually triggers the exception.

ktf avatar Jun 04 '25 07:06 ktf

The following snippet

#include <TTree.h>
#include <TTreeReader.h>
#include <TTreeReaderValue.h>

int main()
{
    TTree t{"test", "test"};
    std::vector<int> b1;
    std::vector<float> b2;
    t.Branch("b1", &b1);
    t.Branch("b2", &b2);

    TTreeReader tr{&t};
    TTreeReaderValue<std::vector<int>> v1{tr, "b1"};
    TTreeReaderValue<std::vector<float>> v2{tr, "b2"};

    tr.Next();
}

Works in the same way with a build of v6-32-00-patches and master, i.e. nothing is printed on screen, no exceptions are thrown. Adding a third TTreeReaderValue corresponding to a non-existing branch, e.g. TTreeReaderValue<double> v3{tr, "b3"}; prints to screen an error similar to some of the ones in your issue description

Error in <TTreeReaderValueBase::CreateProxy()>: The tree does not have a branch called b3. You could check with TTree::Print() for available branches.

And no exception is thrown. Note that I see this printout only in master, and not in the build of v6-32-00-patches.

vepadulano avatar Jun 04 '25 07:06 vepadulano

The exeception was invoked because value.GetSetupStatus() < 0 was previously true, now it's not anymore.

ktf avatar Jun 04 '25 07:06 ktf

Thanks for the extra info, I have added the GetSetupStatus call to my snippet and I see the following:

  • v6-32-00-patches: returns -7 == kSetupNotSetup = -7 "No initialization has happened yet."
  • master: returns 0 == kSetupMatch = 0 "This branch has been set up, branch data type and reader template type match, reading should succeed."

EDIT: messages taken from the data member's comments at https://github.com/root-project/root/blob/0d14d67b1fa97bceb1171e0c1f0e6f003ac57f2f/tree/treeplayer/inc/TTreeReaderValue.h#L48

So indeed there is a change in behaviour there, which needs to be investigated. My personal opinion is that both v6-32-00-patches and master are actually wrong. It is simply false that "no initialization has happened yet", because the branch exists. At the same time, it is also false that "reading should succeed", because there is no entry to read from. So I think we need a new status e.g. kSetupMatchButNoEntryAvailable. I don't know if this should be represented as a negative integer or positive integer though, so I will appreciate your thoughts. Let me also add @pcanal to the conversation.

vepadulano avatar Jun 04 '25 08:06 vepadulano

For what I am concerned, we (ALICE) have at least one bit of code which relayed on the API returning a negative number, so I would appreciate if that behaviour was re-established. Wether this is -7 or some other number, at the moment, I don't have any preference.

ktf avatar Jun 04 '25 08:06 ktf

If an exception is desired when the TTree is empty, wouldn't it make more sense to catch it explicitly? I.e.

    5     TTree noEntry("noEntry", "All branches correct but no entry");
    4     std::vector<o2::mch::Digit> digits;
    3     std::vector<o2::mch::ROFRecord> rofs;
    2     noEntry.Branch("MCHDigit", &digits);
    1     noEntry.Branch("MCHROFRecords", &rofs);
if (noEntry.GetEntriesFast() == 0)
   throw Exception("not entries here... ");
  129     BOOST_CHECK_THROW(DigitTreeReader dtr(&noEntry), std::invalid_argument);
    1     noEntry.Print();

or more exactly doing the check in the first line of DigitTreeReader's constructor? This sounds like it would allow for a clearer exception (message) and would work both for old and new version of ROOT.

pcanal avatar Jun 11 '25 20:06 pcanal

Probably yes, however it's not my code and I have no idea (nor I want to know) in how many places we rely on that. Given the push for RNTuple, I would claim that the TTree API needs to remain stable.

ktf avatar Jun 12 '25 05:06 ktf

What am I doing wrong in the following attempt to reproduce?: With:

// tester.C
#include <TTree.h>
#include <TTreeReader.h>
#include <TTreeReaderValue.h>

int tester()
{
    TTree t{"test", "test"};
    std::vector<int> b1;
    std::vector<float> b2;
    t.Branch("b1", &b1);
    t.Branch("b2", &b2);

    TTreeReader tr{&t};
    TTreeReaderValue<std::vector<int>> v1{tr, "b1"};
    TTreeReaderValue<std::vector<float>> v2{tr, "b2"};

    std::cout << "The setup status for v1 is: " << v1.GetSetupStatus() << '\n';
    return (v1.GetSetupStatus() < 0) ? 0 : 1;
}

I get

root.exe -b -l -q tester.C

Processing tester.C...
The setup status for v1 is: -7
(int) 0

pcanal avatar Jun 13 '25 18:06 pcanal

Hi @pcanal , if you're using latest ROOT master you should get 0. w.r.t. my previous example in this issue at https://github.com/root-project/root/issues/18955#issuecomment-2939052784, you're missing the call to TTreeReader::Next.

vepadulano avatar Jun 16 '25 12:06 vepadulano

Indeed with Next I now get the behavior. It is still a bit odd to me that the original code is not checking for error code from Next (which returns 0/false when there is no (further) entries available).

That said, since it is also understandable that this existing (user) code might be de facto deprecated (not reused for RNTuple) and it can indeed be argued that 'not entries at all' could be consider an error case, we could indeed add kSetupMatchButNoEntryAvailable with a negative number (it is a bit weird but oh well :) )

pcanal avatar Jun 16 '25 22:06 pcanal

If it can make things more clear, I am also ok with changing the API to GetSetupStatus(bool errorOnEmpty = true) and act accordingly.

ktf avatar Jun 17 '25 05:06 ktf

Fair enough. Thanks.

pcanal avatar Jun 17 '25 19:06 pcanal