Skip to content
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

Remove incremental #17973

Open
wants to merge 369 commits into
base: master
Choose a base branch
from
Open

Remove incremental #17973

wants to merge 369 commits into from

Conversation

couet
Copy link
Member

@couet couet commented Mar 12, 2025

Remove incremental to build ROOT

ferdymercury and others added 30 commits November 21, 2024 12:00
* [tree] valid branch names or filenames does not ensure valid C++ variable names

So convert wherever need in produced selector code

Fixes https://its.cern.ch/jira/browse/ROOT-8775

* simplify

thanks to silverweed

* [tree] reuse map-cpp-name function from clingutils

as suggested by dpiparo

* [tree] clarify variable names

as suggested by pcanal

* [tree] clarify variable names

as suggested by pcanal

* [core] clarify warning

(cherry picked from commit 832b0c0)
…t#16998)

This Ensure non-repeated converted ids and valid return if empty input

Suppose a `TTree` with valid branch names: "1_coef", "2_coef".
With `TTree::MakeSelector`, the converted cpp names now would lead to a duplication of variables. Fix this by prepending a char instead of removing the digits.

Also, for the corner case that one passes an empty string, return the "minimally valid variable name in C++", ie just a "_". int _ is a valid variable name.

(cherry picked from commit 0ca0432)
(cherry picked from commit af1718a)
  1. If you are using the `[[unlikely]]` attribute for your `if` of
     `else`, the curly brackets need to be written. Otherwise, the
     attribute will refer to the first statement in the code branch,
     which is meaningless because the statement will always be executed
     once the code branch is taken.

  2. The `if constexpr` can be tricky with unused variables, because if
     the branch is not taken, it's clear at compile time and all
     variables only used in that branch are effectively unused. There
     was a case where the `if constexpr` branch used one variable, and
     the corresponding `else` used another variable of the same type.
     To avoid redundant computations and compiler warnings, only a
     single variable is now defined to what is needed respecively.

  3. The return value of `fread` was unchecked. This commit suggest to
     do what is also done in tutorials for that function:
     https://www.tutorialspoint.com/c_standard_library/c_function_fread.htm
The second argument was only relevant for CINT.
If only one of the pdfs in the RooSimultaneous can't provide an expected
number of events, calculating the relative number of events is futile
and will result in exceptions when trying to ask these pdfs for the
expected number of events.

This fixes a bug reported by @gartrog from ATLAS (crash when printing
workspaces with such pdfs).
Fixes warnings about duplicate names.

(cherry picked from commit 0cd3bc8)
Python gives up ownership if the TTree or TH1 is attached to a TFile,
which is the owner in that case.

(cherry picked from commit 23c893d)
This feature arguably causes more harm than good.

The idea of the memory regulator was to fix user errors like this:

```
ROOT.gInterpreter.Declare("""
void consume(TTree & t) { delete &t; }
""")

t = ROOT.TTree("tree", "tree")
ROOT.consume(t)
print(t)
```
Instead of a segfault because `t` was deleted, currently ROOT will
print:
```
None
```

Basically, it detected if any object registered in `gROOT` was deleted
somewhere on the C++ side to notify CPyCppyy, which reacts by swapping
a cppyy-specific "None" type into the Python proxy. However, such
proxies are in a state that causes the garbage collector to segfault in
specific environments, and this is not the only problem the memory
regulator has. It was also the source of many poorly understood crashes
in the past.

One other problem is that it only works for types that are registered in
`gROOT`, like TTree, TFile, TH1, etc. If a user tries to access any
other C++ class via Python after it was deleted, it will still segfault.
So the memory regulator only sporadically solves the problem and results
in an inconsistent user experience: the "ROOT-tracked" classes behave
even more different than they already do.

Therefore, this commit suggests to disable the PyROOT memory regulator.

(cherry picked from commit 880d681)
Accessing deleted objects is not supported anymore, so it should not be
shown.

(cherry picked from commit b7956d2)
In absense of the memory regulator, one needs to be more correct with
object ownership and closing files.

(cherry picked from commit c4b53f8)
This mode used in ping.cxx test.
In some cases chrome browser does not close gracefully or
start crash reported.
Usage of posix_spawn allows kill chrome

Should solve problem with webgui-ping-test on Mac
Also Linux will be improved
For some reasons chrome does not exit normally after the test,
therefore disable it until problem is identified
`math/minuit2/inc/Minuit2/FunctionMinimizer.h` was removed in 91bb4d7
but there is still a reference to `FunctionMinimizer.h` in
`math/minuit2/src/CMakeLists.txt`. This appears to be a simple
oversight, as similar references were removed in the aforementioned
commit. This is causing the build to fail, so this removes the
reference to fix the `Cannot find source file` error.
Until the DNS issue is fully understood.
In distributed RDataFrame, transformation operations (such as Define)
from which other parts of the API depend upon (such as GetColumnNames or
GetColumnType) are executed not only in the distributed tasks but also
locally, to ensure those other functions can be called. Before this
commit, this was done by simply appending the call to the local
RDataFrame instance attached to the head node. This behaviour was
limited, for example in case the same column name was used in two
separate branches of the computation graph, one could see errors such
as:
```
  File "/cvmfs/sft-nightlies.cern.ch/lcg/views/dev3cuda/Tue/x86_64-el8-gcc11-opt/lib/DistRDF/Proxy.py", line 275, in get_proxy_for
    _update_internal_df_with_transformation(node, operation)
  File "/cvmfs/sft-nightlies.cern.ch/lcg/views/dev3cuda/Tue/x86_64-el8-gcc11-opt/lib/DistRDF/Proxy.py", line 63, in _update_internal_df_with_transformation
    node.get_head()._localdf = rdf_operation(*operation.args, **operation.kwargs)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/cvmfs/sft-nightlies.cern.ch/lcg/views/dev3cuda/Tue/x86_64-el8-gcc11-opt/lib/ROOT/_pythonization/_rdf_pyz.py", line 394, in _PyDefine
    return rdf._OriginalDefine(col_name, callable_or_str)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: Template method resolution failed:
  ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void> ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void>::Define(string_view name, string_view expression) =>
    runtime_error: RDataFrame::Define: cannot define column "nj4". A column with that name has already been Define'd. Use Redefine to force redefinition.
  ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void> ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void>::Define(string_view name, string_view expression) =>
    runtime_error: RDataFrame::Define: cannot define column "nj4". A column with that name has already been Define'd. Use Redefine to force redefinition.
  Failed to instantiate "Define(ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void>&,std::string,std::string)"
  Failed to instantiate "Define(ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void>*,std::string,std::string)"
  Failed to instantiate "Define(ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager,void>,std::string,std::string)"
```

With this commit, the order of node creation in the computation graph is
respected and transformations are chained to the correct parent node.

This improvement highlighted some minor issues in unittests, which are
now improved as well. A new test for this specific issue has also been
added.

(cherry picked from commit 945a3c1)
This makes it more efficient to explore files with RooFit workspaces
in ipython and the jupyter notebook.
On some platforms, the low-level runtime library provided by GCC will
also be loaded when importing ROOT. Therefore, it needs to be added to
the whitelist such that the ROOT Python module tests don't fail because
of it.

Fixes a test failure that I saw on my workstation with NixOS.
In case VariationsFor is called on a branch of the computation graph
without a Vary, the resulting map should be created just with the
nominal value. When interacting with GetMergeableValue, it was instead
creating a corrupted mergeable value for the map of variations, leading
to segfaults. This commit cleanes the function logic and ensures that a
correct, empty RMergeableVariations<T> is created when there are no
variations in the branch, so that the nominal value can be added right
after.
dpiparo and others added 14 commits March 10, 2025 20:28
instread of reading them remotely.

(cherry picked from commit f707eba)
the documentation is enough.

Logical backport of feb2c08
…eDavix`

(cherry picked from commit df69849)

The 6.36 RN file was removed.
to avoid access to the web.

(cherry picked from commit b975189)
as it reads remotely a rather big file and it does not convey a
unique message.
and make it download from the web the input file. The C++ version
depends on the Python version.

(cherry picked from commit 4f02bb5)

The name of the tutorial was adapted to the new tutorials directory structure
in order for Davix to be able to read through https

(cherry picked from commit 5747e42)
* [jsroot] 7.8.x 10/03/2025

Fix - hidden canvas in Jupyter Lab, https://root-forum.cern.ch/t/63097/

* Use proper JSROOT version as fallback in the Jupyter

For ROOT 6.34 JSROOT 7.8 has to be used.
@dpiparo
Copy link
Member

dpiparo commented Mar 12, 2025

something wrong with the PR?

@dpiparo
Copy link
Member

dpiparo commented Mar 13, 2025

moreover, 6.34 works again...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.