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.

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

Lazy multithread RDataFrame::Snapshot cause unnessary warning and break gDirectory #14132

Open
1 task done
karuboniru opened this issue Nov 27, 2023 · 3 comments · May be fixed by #16550
Open
1 task done

Lazy multithread RDataFrame::Snapshot cause unnessary warning and break gDirectory #14132

karuboniru opened this issue Nov 27, 2023 · 3 comments · May be fixed by #16550
Assignees

Comments

@karuboniru
Copy link

karuboniru commented Nov 27, 2023

Check duplicate issues.

  • Checked for duplicates

Description

I am getting some Warning in <Snapshot>: A lazy Snapshot action was booked but never triggered. warning at the end of the program while found that all output files I expected to be present are present. What make things worse is that snapshot operation in this case can cause TH1D::SaveAs to stop working.

I did some test regarding the issue and found following minimal reproduce.

Reproducer

Create toy.cpp with content:

#include <ROOT/RDataFrame.hxx>
#include <TRandom.h>

void toy() {
  // comment this line out (i.e. single thread) won't reproduce the problem.
  ROOT::EnableImplicitMT();
  ROOT::RDataFrame emptydf{10000};
  auto df = emptydf.Define("x", []() { return gRandom->Gaus(); }, {});
  ROOT::RDF::RSnapshotOptions opts;
  opts.fLazy = true;
  auto act = df.Snapshot("treeout", "test3.root", "", opts);
  // this should triggr the snapshot
  *act;
  return;
}

int main() {
  toy();
  return 0;
}

Run it by rool -l toy.cpp or compile and run by c++ toy.cpp -o t $(root-config --cflags --glibs) && ./t

Result: Warning in <Snapshot>: A lazy Snapshot action was booked but never triggered. warning is given but test3.root is present.


Create toy.cpp with content:

#include <ROOT/RDataFrame.hxx>
#include <TRandom.h>

void toy() {
  // comment this line out (i.e. single thread) won't reproduce the problem.
  ROOT::EnableImplicitMT();
  ROOT::RDataFrame emptydf{10000};
  auto df = emptydf.Define("x", []() { return gRandom->Gaus(); }, {});
  ROOT::RDF::RSnapshotOptions opts;
  opts.fLazy = true;
  auto act = df.Snapshot("treeout", "test3.root", "", opts);
  // create a histogram and trigger the event loop
  df.Histo1D("x")->SaveAs("hist.root");
  return;
}

int main() {
  toy();
  return 0;
}

Run it by rool -l toy.cpp or compile and run by c++ toy.cpp -o t $(root-config --cflags --glibs) && ./t

Result: Warning in <Snapshot>: A lazy Snapshot action was booked but never triggered. warning is given, test3.root is present.

But hist.root that should come from TH1D::SaveAs is missing.

GDB suggest that control flow have reached TObject::SaveAs, I have no idea how to dig into the issue, but it looks to me as if

if (gDirectory) gDirectory->SaveObjectAs(this,filename,"");

is skipped due to gDirectory being casted to false as TDirectory::GetSharedLocalCurrentDirectory().get()._M_b._M_p is nullptr in this case.

ROOT version

Reproduced on

ROOT Version: 6.28/08
Built for linuxx8664gcc on Oct 13 2023, 09:48:14
From tags/v6-28-08@v6-28-08

and

ROOT Version: 6.30/00
Built for linuxx8664gcc on Nov 26 2023, 23:25:40
From heads/v6-30-patches@tags/v6-30-00

Installation method

6.28/08 from Fedora 39, self-compiled 6.30/00

Operating system

Fedora release 39 (Thirty Nine)

Additional context

No response

@dpiparo
Copy link
Member

dpiparo commented Nov 30, 2023

Thanks for this report. I can reproduce. There is something to be figured out here.
The warning is triggered here

std::all_of(fOutputFiles.begin(), fOutputFiles.end(), [](const auto &f) { return !f; }) /* never run */)
.
The condition

std::all_of(fOutputFiles.begin(), fOutputFiles.end(), [](const auto &f) { return !f; })

Is always true if Finalize is called before: fOutputFiles is always empty:

fOutputFiles.clear();
.

@fnechans
Copy link

Was this followed up on? It's not a critical issue but the extra warnings are still annoying.

@dpiparo
Copy link
Member

dpiparo commented Sep 28, 2024

Was this followed up on? It's not a critical issue but the extra warnings are still annoying.

I agree, and this is followed up in this PR, being tested now: #16550

dpiparo pushed a commit to karuboniru/root-1 that referenced this issue Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants