Skip to content

Reduce memory footprint of parsed datacard#791

Open
nsmith- wants to merge 3 commits into
mainfrom
cardmemory
Open

Reduce memory footprint of parsed datacard#791
nsmith- wants to merge 3 commits into
mainfrom
cardmemory

Conversation

@nsmith-

@nsmith- nsmith- commented Sep 1, 2022

Copy link
Copy Markdown
Collaborator

Previously we were storing nbins*nproc*nsyst of mostly zeros for the nuisance parameter effect info (errline)
Python floats also take 24 bytes, due to object boxing

>>> sys.getsizeof(0.0)
24

With this change, the parsed datacard for STXSStage1p2full now takes 90MB vs. previous 2.7GB to store the errlines

Previously we were storing nbins*nproc*nsyst of mostly zeros
Python floats also take 24 bytes, due to object boxing
@nsmith-

nsmith- commented Sep 1, 2022

Copy link
Copy Markdown
Collaborator Author

Other significant memory usage in t2w comes from extracting the shapes from input files. In particular:

if (file, wname) not in self.wspnames:
self.wspnames[(file, wname)] = file.Get(wname)
self.wsp = self.wspnames[(file, wname)]

caches all input workspaces (for good reason, they may be expensive to continuously re-open) from which various objects may be read, and
ret = file.Get(objname)
if not ret:
if allowNoSyst:
return None
raise RuntimeError("Failed to find %s in file %s (from pattern %s, %s)" % (objname, finalNames[0], names[1], names[0]))
ret.SetName("shape%s_%s_%s%s" % (postFix, process, channel, "_" + syst if syst else ""))
if self.options.verbose > 2:
print("import (%s,%s) -> %s\n" % (finalNames[0], objname, ret.GetName()))
_cache[(channel, process, syst)] = ret

caches all histograms extracted from the input. This latter case I think is optional, because these histograms are likely read once and the data is anyway copied into the RooFit/Combine object that goes into the output workspace.

@nsmith-

nsmith- commented Sep 2, 2022

Copy link
Copy Markdown
Collaborator Author

Ok the initial implementation is apocalyptically slow. Trying a new one

@nsmith- nsmith- changed the base branch from 112x to main January 18, 2023 04:04
@kcormi

kcormi commented May 15, 2023

Copy link
Copy Markdown
Collaborator

Hi Nick,

Did you manage to speed up the implementation? Or is this still a work in progress?
If you've got something which is saving a lot of memory/speeding things up significantly for large models, it would be good to get it merged.

@nsmith-

nsmith- commented May 15, 2023

Copy link
Copy Markdown
Collaborator Author

I haven't had a chance to revisit, but where I left off I found it very challenging to maintain the current performance while migrating away from dictionaries to even just defaultdict or something else that doesn't store millions of 0.0 entries. It turns out it is very hard to beat dictionary performance in python! Perhaps if everywhere we switched to a errline.get(val, default) instead of unchecked access then it may stay performant. On the other hand, a general datacard parser rewrite is long overdue.

@kcormi

kcormi commented May 16, 2023

Copy link
Copy Markdown
Collaborator

Yes, maybe using dict.get with a default could work. We are trying to balance still making smaller piece-by-piece improvements with larger overall changes and rewrites. If you think its worth trying a couple of more things to make this work (e.g. dict.get) then that would be great. But also fine if you think this is just better suited to something we try to handle in a larger rewrite (though that unfortunately means it will take longer to be implemented).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants