Replies: 8 comments 75 replies
-
|
To answer to the bulleted questions:
On the last one, once you get the histograms back you can manipulate and sum them. You can get an idea of the multi-dataset way to do things here, sorry it's just the tests for now. I'll make something more expository after the break: Which uses some of the functions we've put together for handling multiple datasets, test runs, recovering the set of failed files and re-running on that. |
Beta Was this translation helpful? Give feedback.
-
|
OK - a bit of profiling makes things clear. The majority of time is spent opening only the first file in each dataset and fetching the TTree metadata. This was hidden in the past since opening the files was done once per chunk on the remote worker, but since the dask task-graph has to be generated on the client side we must open the file and generate the metadata. Still I'm surprised that's so slow. We can figure out a way to make it faster for sure, either by caching the read schemas or doing the parsing in parallel. How many datasets have you defined? 6 minutes seems quite a lot since from below that would correspond to ~300 datasets, are you reading over xrootd? Here's the profile for posterity (this is opening the same file 20 times): |
Beta Was this translation helpful? Give feedback.
-
|
Here's the profile just using |
Beta Was this translation helpful? Give feedback.
-
|
Ok, thanks Lindsey. I have 182 datasets, and yes I am accessing via xrd. Sorry if I've just missed it, but I'm still not sure why can't we just put the |
Beta Was this translation helpful? Give feedback.
-
|
So the reason for that comes down to how dask task-graphs work, which is broadly split into two phases: building and execution. For building you need to have the information about the structure of the input file (so you have to process the metadata of at least one file in that specific dataset), then you start performing operations on that representation to build up the task graph of stuff you want to do (histograms, skims). Compared to coffea 0.7 where arrays you wanted were provided when you asked for this, arrays are never read from a file until you go to the execution stage, and that requires that you open at least one file in the dataset where you are building the task graph to generate the initial file structure (and you can't just do it later as with coffea 0.7). Another way to put that is that When executed, that taskgraph is optimized and the tasks defined within it are mapped over the partitions of the data to be read in, using the already determined metadata to figure out exactly what needs to be read from the input file. So, as opposed to previously where the file would be opened and its metadata determined later we have to do it eagerly on the client side. I think fixing it is just a matter of organizing things correctly or otherwise making some portions of that initial determination more lazily evaluated. I also just checked and opening to get the metadata is slower in uproot4. So overall it's a pretty significant improvement, it's just organized such that it appears slower to the user right now. |
Beta Was this translation helpful? Give feedback.
-
|
tl;dr - it's slower because we have to do something that's slow we were hiding in the jobs before on the client, but it's not something we can't fix. |
Beta Was this translation helpful? Give feedback.
-
|
Ok, thanks @lgray! I've created an This seems like it might be something obvious, but I'm not sure I understand it, sorry. Wondering if you'd have any tips? Is it related to the array being a np array not a dask array? |
Beta Was this translation helpful? Give feedback.
-
|
Also in your particular implementation of You can see the reduction from 20 columns to 16 in the one that Yi-mu made. https://github.com/CoffeaTeam/coffea/blob/master/tests/test_ml_tools.py#L169-L174 |
Beta Was this translation helpful? Give feedback.
-
|
Ah, okay, thank you! So I think I'd been misunderstanding. The elements in that ak_events array do not necessarily need to correspond to "one element event" but more like one element per object. Then at the end I guess we'd still need to unflatten as before. Ok, so I think I've gotten the xgboost stuff in my code more or less working here. But I still have a few questions:
|
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
|
Ok, thank you! One more quick question on 2, I am still not sure if I understand what situation it is saying unflatten would fail in... what does "partitionwise-compatible" mean in the |
Beta Was this translation helpful? Give feedback.
-
|
Yes it more or less means don't mix counts and data that come from differently partitioned sources, which is a thing you can do if you're not careful. |
Beta Was this translation helpful? Give feedback.
-
|
Also have another question about another I've run into further into the processor (I've just commenting the xgboost stuff for now.. will try to figure it out later). It's on this line: The error is this: So I guess I cannot use Wondering if @lgray or @nsmith- would have any tips on how to work around this? Is there maybe some different version of |
Beta Was this translation helpful? Give feedback.
-
|
Ahh, I am sorry. Please ignore my previous message (for some reason it was not showing me your "so all you have do do" message before I sent my message). |
Beta Was this translation helpful? Give feedback.
-
|
Yeah they both use the same axes, which are outside the distinction of delayed or eager execution, really. |
Beta Was this translation helpful? Give feedback.
-
|
Wow, it actually ran and made a histogram! Was just for a single root file (and the xgboost stuff is still disabled), but anyway it ran :) I'll keep working on the xgboost stuff, and once that's figured out, I think will be pretty much done. Thank you so much @lgray for all of the help. I hope you have a very nice Holiday break. |
Beta Was this translation helpful? Give feedback.
-
|
Sure thing! Have a good break as well! If you can summarize your process with a few examples it'll help a lot for making documentation! |
Beta Was this translation helpful? Give feedback.
-
|
Thanks! And sounds good, once I get the xgboost stuff finished and everything else cleaned up (and once I've validated that I'm getting the same results with the old and new versions) I'll try to summarize things and post it here. |
Beta Was this translation helpful? Give feedback.
-
|
I have a quick question, with coffea2023 is anything like this needed for processors (using @property
def columns(self):
return self._columnsI'm actually not really sure what (if anything) it does (even in coffea 0.7)... but I guess I must have copy pasted it from some example at some point. |
Beta Was this translation helpful? Give feedback.
-
|
No that's not needed at all! |
Beta Was this translation helpful? Give feedback.
-
|
Ok, thanks! |
Beta Was this translation helpful? Give feedback.
-
|
Though that reminds me you should check |
Beta Was this translation helpful? Give feedback.
-
|
Happy New Year! I have a question about the structure of the output histogram object for the coffea 2023 version of my processor. The way I'd previously been running with coffea 0.7, the output histogram object had a StrCategory axis for samples (e.g. ttH, ttW, etc). This structure was convenient for manipulating and plotting (to do e.g. summing and grouping etc). However, in my initial attempt to migrate to coffea 2023, I'm now passing each dataset one by one to my process function, so I eventually pass an object like The way I can think of to do this with coffea2023 would be to pass something like Anyway, sorry for the long question (and sorry that it's not very well formulated), but I'm wondering if you'd have any thoughts or advice on this? Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
a fileset looks like: {
"ZJets": {
"files": {
"tests/samples/nano_dy.root": {
"object_path": "Events",
"steps": [
[0, 5], [5, 10], [10, 15], [15, 20], [20, 25], [25, 30], [30, 35], [35, 40],
],
}
},
"metadata": {...},
...
},
"Data": {
"files": {
"tests/samples/nano_dimuon.root": "Events",
"tests/samples/nano_dimuon_not_there.root": "Events",
},
"metadata": {...},
...
},
}i.e. it's just a list of datasets where each dataset has files, possibly with chunks defined, and metadata. It shouldn't be related to post-process at all. I'd have to see your exact code to understand why you are getting a tuple, given what you said and the code you linked that should not be so. I'll patch out the need for postprocess. Nominally you don't really need to inherit from |
Beta Was this translation helpful? Give feedback.
-
|
Oh, maybe you're talking about the output of |
Beta Was this translation helpful? Give feedback.
-
|
Ah, okay, I did not realize the output of dask.compute would always be a tuple. Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
Alternatively, |
Beta Was this translation helpful? Give feedback.
-
|
and I am still learning things about python syntax... |
Beta Was this translation helpful? Give feedback.
-
|
I have a quick question (probably something obvious that I'm just missing). When I try to print e.g. |
Beta Was this translation helpful? Give feedback.
-
|
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
Thank you again @lgray for all of the help with this. I think my analysis code is finally pretty much fully migrated. I should clean some parts up, but as of now everything seems to be working fine for my tests with a single file anyway (yields for the single file agree, and my CI is passing). Next up I will try to run at scale. Just for reference, with coffea 0.7 scaled out with Work Queue, this analysis (which is still fairly preliminary so does not have any systematics yet) was able to turn around in ~20m using ~500 cores. I will talk to @btovar and @cmoore24-24 about how to run with TaskVine, and once we have it running I can post any interesting performance numbers here. But I have one quick question before trying to scale up. @lgray I am wondering if you could explain a bit more about how/where the |
Beta Was this translation helpful? Give feedback.
-
|
Hmm, they appear to be exactly the same (testing set equality in the python terminal). There's definitely some stuff your code isn't asking for yet (like the jet indices) that are getting touched despite not being needed. One thing you can do is go through the code and do This may expose yet another overtouching bug we need to fix. |
Beta Was this translation helpful? Give feedback.
-
|
@Jailbone please make a new discussion! Your comment comes a bit in the weeds of a very long discussion and I'd want to make sure it's easy for people to find. |
Beta Was this translation helpful? Give feedback.
-
|
Printing out So I guess it must be something inside of get_topmva_score_ele() that's the culprit (at least for the electron related variables). I'll keep digging... Edit: Sorry, the link was to the master branch. This is the function on the coffea2023 branch here. |
Beta Was this translation helpful? Give feedback.
-
|
Actually, it seems that as soon as I do Wondering if this is expected, or if at this point we would not expect to see any columns since we have not used any particular columns (e.g. |
Beta Was this translation helpful? Give feedback.
-
|
Yes if you ask for the columns necessary for the whole electron object it'll need to grab all the data. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @lgray I'm wondering if there is any type of progress bar that would be available with coffea 2023 (e.g. something similar to the extremely useful progress bars available in coffea 0.7)? |
Beta Was this translation helpful? Give feedback.
-
|
https://docs.dask.org/en/stable/diagnostics-local.html#progress-bar |
Beta Was this translation helpful? Give feedback.
-
|
Though, really, the dashboard (localhost:8787) that the distributed client creates is faaaar more useful than a basic progress bar. |
Beta Was this translation helpful? Give feedback.
-
|
Ok, thank you |
Beta Was this translation helpful? Give feedback.
-
Hello @lgray, @nsmith- , all,
I'm attempting to migrate this analysis code to coffea 2023. Just for future reference, here is a link to the repo at the specific current commit (so that we have a "pre coffea 2023" to refer against). Thank you to @btovar for pointing me to @cmoore24-24's coffea 2023 processor here. This, along with the references from @lgray (here and here) are what I'm trying to base this migration off of.
Ok, so in the current version of my code, this is where the processor was run. Here are the relevant lines for the iterative executor:
When I naively just attempt to run with coffea2023, the first thing that breaks is from that block (with an error
AttributeError: module 'coffea.processor' has no attribute 'IterativeExecutor'). So is seems like this block is a good thing to try to focus on first. So, attempting to rewrite this block for coffea2023, I think I need to do something like:(Though of course eventually will have to change things in the process function too, but I wanted to just focus on trying to call it the right way first.) If that's the right direction, then I think what I am stuck on next is
events. It is not clear to me what this is supposed to correspond to. In the pre-coffea2023 version, we never directly call the processor'sprocessfunction ourselves, but I think itseventsargument just corresponds to the nanoevents object for just the events in the particular chunk.In @lgray's announcement of coffea 2023 (on mattermost here) it was mentioned that the arrays correspond to an entire dataset, not a chunk of a dataset. So I think my questions for now would be:
eventsargument should correspond to all of the events in just one dataset? Or would it correspond to all of the events that we are processing in total?events?)Sorry for the long message, and thank you in advance for any help or tips!
Beta Was this translation helpful? Give feedback.
All reactions