-
Notifications
You must be signed in to change notification settings - Fork 18
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
nanoAOD_ttbar
latency
#280
Comments
ah ha, @grasph, looks like this is automatically "fixed" on Julia 1.10:
|
It might be a fun exercise to find out what can we do for v1.9, but notice unlike #279 (comment). This is NOT a case of inference failure causing boxing, because the total number of allocation and amount are almost identical: 11.09 M allocations: 1.500 GiB
11.57 M allocations: 1.527 GiB, |
What magic is 1.10 doing here, I wonder? |
Frames said this on Slack:
|
Thanks @Moelf |
Some more comments:
17x seems large but I don't think it's problematic when it's 0.1s vs. 1.4s. Considering ROOT takes so long to compile I'm not surprised. If anything, we can try to add more precompilation. I don't know how much wider does a root file get, NanoAOD has 1.5k branches. I would argue if user deals with 5k columns or whatever, at that point they ought know they should select columns. Nothing infinitely scales to all cases. The current design has 2 main objective:
The second point is coupled to the first point, because naively you may want to just return
I'm saying just wait. It's already Julia 1.10-beta3, so not that far from release. |
Finally, I'm happy to have a But what those 2 packages are NOT good at and don't care at all, is the user experience out of the box (the display, the iteration capability). We probably shouldn't 'do that. We also should think harder about what function names / apis are actually gonna be called, I think LazyTree is not perfect considering RNTuple will come eventually. You want to convey 2 things:
Alternatively, we can have:
|
Just curious, why would anyone make that many branches? Are they used to emulate what should be arrays, or so? I'm not saying we shouldn't be able to handle it well, I'm just surprised at the sheer number. |
they include systematics, there are O(10) different types of objects you reconstruct, each has a few dozens of systematics. And then there are all the L1 trigger and HLT triggers. Many branches are already jagged. |
And we can't really do hierarchies - I get it. |
@grasph I honestly think you also just want to tell them to select, like, it's absurd to just open all the branches unless you're first day on the job. I mean first of all tutorials should be given with slimmed files, and second, we don't have CMS nanoAOD systematics CP tools anyway, so it's pointless to read all branches |
Related issue: #272 These improvements with 1.10.x are great. Thanks, Jerry, for the investigations. I think we should still have a low-latency option especially to address the data-frame like analysis, for which fast row access is not required. It can be a parameter to LazyTree constructor to switch to this option. I made some tests showing that with some pre-compilations (for common column types) and untyped columns, we can reach a 0.3s setup time. Measurements (done with Julia 1.9.1) within the comments of the code below. using UnROOT
file = ROOTFile("nanoaod.root");
tree = file["Events"];
brnames = keys(tree);
function f1(file, t, brs)
res = Vector{LazyBranch}(undef, length(brs))
for (i,x) in enumerate(brs)
res[i] = LazyBranch(file, t[x])
end
res
end
function f4(file, t, brs)
res = Vector{Pair{Symbol, LazyBranch}}(undef, length(brs))
for (i,x) in enumerate(brs)
res[i] = (Symbol(x) => LazyBranch(file, t[x]))
end
res
end
#this list of branches cover all branch types found in the nanoaod.root test file
brwarmup = ["run","event","BeamSpot_type","BeamSpot_sigmaZ","nboostedTau","boostedTau_idAntiEle2018","boostedTau_jetIdx","boostedTau_charge","boostedTau_chargedIso","Electron_convVeto","TrigObj_id","L1Reco_step"]
# Timing provided in comments were measured with only one group
# of code between two #----... lines included. UnROOT v0.10.15
#----------------------------------------------------------------------
# 0.78s
@time f1(file, tree, brwarmup)
# 0.27s
@time brs = f1(file, tree, brnames);
#----------------------------------------------------------------------
#----------------------------------------------------------------------
##1.05 s
#@time brs = f1(file, tree, brnames);
##0.27 s
#@time brs = f1(file, tree, brnames);
#----------------------------------------------------------------------
#----------------------------------------------------------------------
#using DataFrames
## 1.04s
#@time "dict warmup" wu = Dict(f4(file, tree, brwarmup));
##0.95 seconds (1.09 M allocations: 73.972 MiB, 2.18% gc time, 99.83% compilation time)
#@time "warmup df wrapping" df = DataFrame(wu, copycols=false);
## 0.27s
#@time "dict" brs = Dict(f4(file, tree, brnames));
## 0.0015s (0.9 seconds without the dataframe-wrapping warmup)
#@time "df wrapping" df = DataFrame(brs, copycols=false);
## 0.15s
#@time "sum" sum(df.nMuon)
#----------------------------------------------------------------------
#----------------------------------------------------------------------
#using DataFrames
## 1.9s
#@time "DataFrame warmup" DataFrame(f4(file, tree, brwarmup), copycols=false);
## 0.28s
#@time "DataFrame" df = DataFrame(f4(file, tree, brnames), copycols=false);
## 0.15s
#@time "sum" sum(df.nMuon)
#----------------------------------------------------------------------
#----------------------------------------------------------------------
##1.34 s
#@time "Dict warmup" brs = Dict(f4(file, tree, brnames));
##0.29s
#@time "Dict" brs = Dict(f4(file, tree, brnames));
##0.21s
#@time "sum" sum(brs[:nMuon])
#---------------------------------------------------------------------- |
Thanks a lot! Can this wait? We can discuss with @tamasgal at juliahep workshop maybe |
It would be good to have a solution before the workshop and we can discuss about improvements there. The main contributors to the number of branches of the CMS nanoad are actually the trigger line flags: it has one branch per line and trigger many lines. Although the setup overhead is large compared to the time to read all branches, the main point is not about reading all of them, but to have access to all the branches from the LazyTree object. The issue with the display comes also from the compilation time: it took me 279s to call show(::LazyTree) with UnROOT v.0.10.17 and Julia 1.9.1 and 148s with UnROOT v.0.10.17 and Julia 1.10.0beta3. So 1.10.x does NOT solve the issue for the display and when the LazyTree is instantiated interactively without an ending ';'. The issue is twofold:
Current solution for (1) is to pass a LazyTree to the constructor of the Table of choice. It's inefficient if LazyTree instantiation takes time. This can be solved by providing a method to fill any container following the Tables interface ala CSV.read or a method like the f4() method of my previous comment. It is simple to implement and quite independent from the existing code. (2) can be addressed by providing two options for LazyTree: with typed (current NamedTuple option) and untyped columns. The code is more involved, but I have already a prototype. |
I definitely tend to towards the first approach. Something like a generic interface which can be turned into LazyTree or I have not spent enough time on this problem yet to be honest. I use UnROOT on a daily basis but only a with a handful (but fairly large and complex) branches and for those of course it works totally fine. |
ok what about this temporary stopgap solution: we add a new keyword argument so something like: LazyTree(file, treename; sink = ...) if This has two advantages:
|
You would expect the LazyTree() function to returns a LazyTree instance, so I think it's cleaner to have a new function e.g., getTree(), that will take a sink type. If the sink type is LazyTree() then it will do the same as LazyTree(). You mentioned breaking changes, all the proposals I've made so far preserve backward compatibility. |
(f4() is also a good option) |
I agree. But I don't know what we should name the I think effectively this is a |
We can pick up a name and write in the documentation that it can change. An alternative name can be readTree(), although the reading is deferred. |
again, If we pick up a name and make it public API, that's potentially one more thing to break on 1.0 |
same as for LazyTree ;) |
Anyway, let's just add a sink option to LazyTree as you proposed. I've updated the code in my fork sink-option branch. Let me know if I should make a PR. |
There is still an issue with the master (b59a63). Below the LazyTree call during with sink-opt patch on top of v0.10.15 and master for my nano AOD file.
|
But we already have LazyTree, so adding a sink options is not breaking, we will just recommend a new interface at 1.0. But if you add getTree() and remove it at 1.0, that's an additional breaking. Can you show more complete code for what you're doing with Data frame? I can investigate soon |
The issue came from the merge. I merged with origin/master instead of origin/main. Now it's fixed. I'll do the PR. PS: by investigating, I've found out why the TDirectory cache, now removed, was not working properly. |
Originally posted by @grasph in #273 (comment)
The text was updated successfully, but these errors were encountered: