Immediately safe_import CMSHistFuncWrapper

[maxgalli]

A huge part of the execution time when running text2workspace.py for template models seems to be spent in importing into the workspace instances of CMSHistFuncWrapper. This import uses ROOT.Roofit.RecycleConflictNodes, which traverses the dependency graph and seems to grow non-linearly with the workspace size.

Instead of importing all the instances of CMSHistFuncWrapper at the end of doCombination, this PR moves the safe import right after the creation of each instance, when the workspace is less large, effectively reducing the time spent in traversing the graph and the overall runtime of text2workspace.py.
I tested this on the pT datacard of HIG-20-015 (see here) and the overall time to run the text2workspace.py command decreased from 375.81s to 264s (measured with /usr/bin/time). In this case, the command created ~4000 of such wrappers when running t2w.

Considering that it seems to much gain for such an easy fix, I expect that there is something that I overlooked and something to break somewhere else, so let's see if the tests are green.

Summary by CodeRabbit

• Refactor
  • Streamlined import handling in model processing operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Refactored import handling in ShapeBuilder to eliminate deferred batch imports. The self.extraImports list was removed, and imports are now executed immediately within doIndividualModels using safe_import() instead of being collected and processed later in doCombination.

Changes

Cohort / File(s)	Change Summary
Import processing refactor python/ShapeTools.py	Removed self.extraImports initialization; replaced deferred import accumulation with immediate safe_import() calls in doIndividualModels; removed subsequent import loop from doCombination

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Verify that immediate imports produce identical behavior to deferred batch imports
• Ensure no edge cases where deferred processing was relied upon for ordering or conflicts
• Confirm safe_import() call parameters are correct

Poem

🐰 Imports no longer wait in queue,
They happen now, fresh and true,
No list to store, no loop to run—
Immediate imports, clean and done! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Immediately safe_import CMSHistFuncWrapper' directly describes the main optimization: moving the safe import of CMSHistFuncWrapper to happen immediately rather than deferred, which is the core change that achieves the performance improvement.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 20.69%. Comparing base (1df3a9f) to head (8c544ee).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1194   +/-   ##
=======================================
  Coverage   20.69%   20.69%           
=======================================
  Files         195      195           
  Lines       26156    26156           
  Branches     3923     3923           
=======================================
  Hits         5413     5413           
  Misses      20743    20743           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maxgalli]

@guitargeek @anigamova do you see any potential problems with this?

[guitargeek]

Well, I don't see a problem if the CI doesn't see a problem! The import order to workspaces doesn't matter

[anigamova]

That's interesting, because in this commit it was changed in the opposite direction but also for optimization reasons. @guitargeek do you recall if there is something that changed in the import behavior to give such an effect?

[guitargeek]

Yes, I changed many things in the implementation some years ago, to make some ATLAS workspace imports faster. It's very likely that this has shifted the performance bottlenecks, so now importing early is faster.

Maybe you can check performance with the different ROOT versions that Combine supports?

[anigamova]

OK, anyway I think it makes sense