[WIP][Cling] Refactor CIFactory and add incremental action support - #232
Closed
smuzaffar wants to merge 224 commits into
Closed
[WIP][Cling] Refactor CIFactory and add incremental action support#232smuzaffar wants to merge 224 commits into
smuzaffar wants to merge 224 commits into
Conversation
|
A new Pull Request was created by @smuzaffar for branch cms/master/6bf5355cc45. @akritkbehera, @cmsbuild, @iarspider, @raoatifshad, @smuzaffar can you please review it and eventually sign? Thanks. |
|
cms-bot internal usage |
Author
|
test parameters:
|
Author
|
please test |
2 tasks
|
-1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6599f5/54197/summary.html Failed External BuildI found compilation error when building: + echo 'test $?PY3_NUMPY_ROOT != 0 || source /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el9_amd64_gcc13/external/py3-numpy/1.26.4-6c7401133d71aa0fdd19f4799dadfbdf/etc/profile.d/init.csh'
+ echo fi
+ touch /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/BUILDROOT/4b26fd0c9f290a660109640c68a0e0e0/opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-4b26fd0c9f290a660109640c68a0e0e0/etc/profile.d/.autodependencies
Processing files: lcg+root+6.41.1-4b26fd0c9f290a660109640c68a0e0e0-1-1.x86_64
warning: absolute symlink: /opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-4b26fd0c9f290a660109640c68a0e0e0/lib/cuda.pcm.lock -> /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/BUILDROOT/4b26fd0c9f290a660109640c68a0e0e0/opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-4b26fd0c9f290a660109640c68a0e0e0/lib/cuda.pcm.lock-fd641802
error: Symlink points to BuildRoot: /opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-4b26fd0c9f290a660109640c68a0e0e0/lib/cuda.pcm.lock -> /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/BUILDROOT/4b26fd0c9f290a660109640c68a0e0e0/opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-4b26fd0c9f290a660109640c68a0e0e0/lib/cuda.pcm.lock-fd641802
RPM build warnings:
Macro expanded in comment on line 497: %{pkginstroot}/lib
Macro expanded in comment on line 498: %{pkginstroot}
|
|
Why does that try to build the cuda module again? cc: @SahilPatidar. |
Author
|
@vgvassilev , yes we do try to build cuda module ( see https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_20_1_X/master/root.spec#L182 ) and looks like it fails |
Author
|
@vgvassilev , looks like |
|
So, what's the right behavior nowadays, to build cuda.pcm or to not build it now? |
Author
|
In past we had jobs which were generating cuda modules on fly and causing issue ( see cms-sw/cmssw#47400 ). So we started building |
|
Are you sure that failure comes with this new code changes? Eg. not some temporal infrastructure issue? |
Author
|
please test lets me try again |
Author
|
@vgvassilev , yesterday we tested root-project@6bf5355cc45 in cmssw and generating cuda pcm did not fails This Pr is testing these changes on top of root commit root-project@6bf5355cc45 |
|
Ok, so that's a real regression -- what would be the way for Sahil to reproduce it? He has no HEP/CERN affiliation. |
|
-1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6599f5/54215/summary.html Failed External BuildI found compilation error when building: + echo 'test $?PY3_NUMPY_ROOT != 0 || source /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/el9_amd64_gcc13/external/py3-numpy/1.26.4-6c7401133d71aa0fdd19f4799dadfbdf/etc/profile.d/init.csh'
+ echo fi
+ touch /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/BUILDROOT/b9a5bd8aca6afd91000923d55aa0acd5/opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-b9a5bd8aca6afd91000923d55aa0acd5/etc/profile.d/.autodependencies
Processing files: lcg+root+6.41.1-b9a5bd8aca6afd91000923d55aa0acd5-1-1.x86_64
warning: absolute symlink: /opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-b9a5bd8aca6afd91000923d55aa0acd5/lib/cuda.pcm.lock -> /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/BUILDROOT/b9a5bd8aca6afd91000923d55aa0acd5/opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-b9a5bd8aca6afd91000923d55aa0acd5/lib/cuda.pcm.lock-a2314b96
error: Symlink points to BuildRoot: /opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-b9a5bd8aca6afd91000923d55aa0acd5/lib/cuda.pcm.lock -> /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/BUILDROOT/b9a5bd8aca6afd91000923d55aa0acd5/opt/cmssw/el9_amd64_gcc13/lcg/root/6.41.1-b9a5bd8aca6afd91000923d55aa0acd5/lib/cuda.pcm.lock-a2314b96
RPM build warnings:
Macro expanded in comment on line 497: %{pkginstroot}/lib
Macro expanded in comment on line 498: %{pkginstroot}
|
Author
|
In order to reproduce it, you just need to build root locally with cuda enabled and just run |
9 tasks
Disable tests failing due to unresolved symbols with Visual Studio 2026
The axis parser for RooGeneric/RooFormulaVar binning read "edges", "min" and "max" via JSONNode::val_double() without first checking the node was actually a number. For a non-numeric JSON value (e.g. a string) this either threw a backend-specific exception that is not a std::runtime_error (aborting past the tool's error handling) or, with the stringstream-based backend, silently coerced the value to 0 and imported a wrong binning. Add a JSONNode::is_number() predicate (native in the nlohmann backend, with a val()-parsing fallback in the base interface) and guard the edge and min/max conversions with it, so malformed bounds are rejected with a descriptive RooJSONFactoryWSTool::error() like every other malformed axis case. Full precision is preserved because the value is still read through val_double() once validated.
readPositiveInteger parsed "nbins" with a strict std::from_chars over the node's textual value, which rejected an integer bin count encoded as a JSON float: e.g. 1000000.0 renders as "1e+06", where from_chars stops at 'e' and the whole import fails with a misleading "must be a positive integer" error. Other nbins readers in HS3 use the lenient val_int(). Read the value through val_double() instead and require it to be finite, >= 1, integral and within int range. This accepts integer-valued floats like 1e6 while still rejecting fractional (2.5), non-positive (0) and non-numeric values, so the existing rejection tests keep passing. Drops the now-unused <charconv> include.
The separate roofit/jsoninterface package made sense when there was also a YAML backend, but since only the nlohmann/json backend remains, it was just unnecessary abstraction boilerplate as a standalone library. Move the RooFit::Detail::JSONInterface header into RooFitHS3 and merge the implementation (JSONInterface.cxx, JSONParser.h, JSONParser.cxx) into a single translation unit, with the TJSONTree implementation class now in an anonymous namespace. This remains the only translation unit that includes nlohmann/json.hpp, so the JSON engine could still be swapped out in the future by changing only this one file. Also remove the unused writeYML() interface method, a leftover from the YAML backend, and move testJSONInterface into the RooFitHS3 test suite.
ROOT requires C++17 since a while already.
ROOT requires C++17 since a while already.
ROOT requires C++17 since a while already.
because geom=OFF, e.g. for minimal builds.
Histo1D only supports numeric columns; there was no way to fill a histogram from a string/categorical column (e.g. particle type, detector region labels) without manual workarounds. This adds a BarChart() lazy action that fills a label-binned, auto-extending TH1D from either std::string columns or classic C-style char[] tree branches (inferred as RVec<char> by RDataFrame). - ActionTags::BarChart + a dedicated BuildAction overload - BarChartHelper, since FillHelper<TH1D> cannot compile against either std::string or RVec<char> (no implicit conversion to the const char* Fill overload) - RInterface::BarChart(vName, name, title) - Test coverage in dataframe_histomodels.cxx (bin ordering/counts) and dataframe_simple.cxx (auto name/title derivation) Closes root-project#17057
This reverts commit 34ad3b3.
Per @vepadulano's review on root-project#22802, a new BarChart() RDataFrame method tied to TH1D would risk deprecation once ROOT's new histogram system (RCategoricalAxis in hist/histv7) gains plotting support and becomes the preferred way to handle alphanumeric bins. This tutorial instead shows the same result using RDataFrame's existing generic Fill() action with a small adapter class, requiring no core API changes. Verified against the same cernstaff.root/Nation dataset used in hist006_TH1_bar_charts.C: 15 bins, labels and counts match TTree::Draw("Nation","","hbar2").
Replace the cernstaff.root/tree500_cernbuild.C dependency with a small writeData() helper that generates a synthetic categorical column via RDataFrame::Define + Snapshot, removing the need to explain classic char[] branch handling, which was out of scope for this tutorial. Rename AlphaNumHist's TH1D member and the RResultPtr variable from the terse 'h' to 'histo'/'result' for readability, and replace the raw 'new TCanvas' with a std::unique_ptr<TCanvas> to avoid showing a leak in a user-facing tutorial. Addresses review comments from vepadulano on PR root-project#22802.
Visible in builds where NDEBUG is not defined.
SahilPatidar
force-pushed
the
incr_action
branch
from
July 29, 2026 05:26
3a0fe8b to
a92dd4e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR to run cmssw tests for root-project#21903