[RF] Don't keep the resolution model as a server of RooAbsAnaConvPdf

The resolution model passed to a RooAbsAnaConvPdf (RooDecay, RooBDecay,
...) is only a *configuration* object: it specifies which model to
convolve the basis functions with, and the pdf evaluates its own internal
convolutions, not the model itself. Yet it was kept as a (non-value,
non-shape) server, which polluted the computation graph and dragged the
model into RooWorkspace imports and HS3/JSON exports for no reason. It
also broke the HS3 round-trip, since the internal convolution objects
leaked into the JSON with non-conforming names.

The model is now an owned, non-server member instead. This deliberately
mirrors how RooResolutionModel already manages its basis function: a
raw owned pointer (_basis / _ownBasis) that is not a server and whose
redirection is forwarded in redirectServersHook(). See
RooResolutionModel.{h,cxx}. The HS3 exporter now lists its real
dependents explicitly (t, tau, model) instead of auto-exporting servers.

This follows up on 478ad2b, which noted that the model server could
not simply be removed without a server/proxy desync; removing the proxy
entirely avoids that desync by construction.

Schema evolution from class version 3 to 4 is handled with the standard
RooFit mechanism: a versioned `#pragma read` rule in LinkDef.h recovers
the model pointer from the old RooRealProxy, and a RooAbsAnaConvPdf::
ioStreamerPass2() override severs the stale server link once the graph is
live. This is the same two-stage pattern already used for the v5->v6
proxy-list migration (RooAbsArg::ioStreamerPass2) and for the read rules
of RooProduct / RooSuperCategory in roofitcore/inc/LinkDef.h. A unit test
reads back a v3 workspace fixture and checks the server structure,
getModel() and the evaluated values.

Closes #20245.

🤖 Done with the help of [Claude Code](https://claude.com/claude-code) (Claude Opus 4.8)

Author: guitargeek

README/ReleaseNotes/v642/index.md

@@ -71,6 +71,27 @@ Users are strongly encouraged to switch to the vectorized CPU backend if they ar
 
 If the vectorized backend does not work for a given use case, **please report it by opening an issue on the ROOT GitHub repository**.
 
+### Resolution models are no longer imported into the workspace as standalone objects
+
+When a `RooResolutionModel` (like `RooGaussModel` or `RooTruthModel`) is used with a
+`RooAbsAnaConvPdf` (like `RooDecay`, `RooBDecay`, etc.), it acts as a *configuration* object that
+specifies which model to convolve the basis functions with, rather than as a node of the pdf's
+computation graph. The `RooAbsAnaConvPdf` builds its own internal basis-function convolutions from
+it and evaluates *those*.
+
+Until now, the resolution model was nevertheless kept as a (non-value, non-shape) server of the
+`RooAbsAnaConvPdf`. As a side effect, importing such a pdf into a `RooWorkspace` also imported the
+original resolution model as a standalone workspace object, and it leaked into HS3/JSON exports,
+even though it played no role in the computation.
+
+Starting with ROOT 6.42, a resolution model that is only used as the configuration of a
+`RooAbsAnaConvPdf` is no longer a server of that pdf, and is therefore not imported into the
+workspace on its own anymore. The model remains accessible via `RooAbsAnaConvPdf::getModel()`.
+
+This is not expected to affect typical usage, since the resolution model was never part of the
+actual likelihood. Workspaces written with older ROOT versions are read back correctly via schema
+evolution.
+
 ## Graphics and GUI
 
 ## Geometry

roofit/hs3/src/JSONFactories_RooFitCore.cxx

@@ -921,7 +921,12 @@ class RooPoissonStreamer : public RooFit::JSONIO::Exporter {
 class RooDecayStreamer : public RooFit::JSONIO::Exporter {
 public:
    std::string const &key() const override;
-   bool exportObject(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem) const override
+   // The RooDecay servers are the internal resModel-times-basis convolutions,
+   // which are implementation details that should not leak into the JSON. We
+   // only export the actual dependents (t, tau and the original resolution
+   // model) explicitly.
+   bool autoExportDependants() const override { return false; }
+   bool exportObject(RooJSONFactoryWSTool *tool, const RooAbsArg *func, JSONNode &elem) const override
    {
       auto *pdf = static_cast<const RooDecay *>(func);
       elem["type"] << key();
@@ -930,6 +935,10 @@ class RooDecayStreamer : public RooFit::JSONIO::Exporter {
       elem["resolutionModel"] << pdf->getModel().GetName();
       elem["decayType"] << pdf->getDecayType();
 
+      tool->queueExport(pdf->getT());
+      tool->queueExport(pdf->getTau());
+      tool->queueExport(pdf->getModel());
+
       return true;
    }
 };

roofit/roofitcore/inc/LinkDef.h

@@ -93,6 +93,15 @@
 #pragma link C++ class RooDirItem+ ;
 #pragma link C++ class RooDLLSignificanceMCSModule+ ;
 #pragma link C++ class RooAbsAnaConvPdf+ ;
+// Schema evolution: up to version 3, the original resolution model was stored
+// in a RooRealProxy (evolved to RooTemplateProxy<RooAbsReal>) and was a server
+// of the pdf. As of version 4 it is an owned, non-server RooResolutionModel*.
+// Recover the model pointer from the old proxy here; the stale server link is
+// cleaned up afterwards in RooAbsAnaConvPdf::ioStreamerPass2().
+#pragma read sourceClass="RooAbsAnaConvPdf" targetClass="RooAbsAnaConvPdf" version="[-3]" \
+  include="RooResolutionModel.h" \
+  source="RooTemplateProxy<RooAbsReal> _model" target="_model,_ownModel" \
+  code="{ _model = static_cast<RooResolutionModel*>(onfile._model.absArg()); _ownModel = false; }"
 #pragma link C++ class RooAddPdf+ ;
 #pragma link C++ class RooEfficiency+ ;
 #pragma link C++ class RooEffProd+ ;

roofit/roofitcore/inc/RooAbsAnaConvPdf.h

@@ -24,8 +24,8 @@
 #include "RooAICRegistry.h"
 #include "RooObjCacheManager.h"
 #include "RooAbsCacheElement.h"
+#include "RooResolutionModel.h"
 
-class RooResolutionModel ;
 class RooRealVar ;
 class RooConvGenContext ;
 
@@ -81,7 +81,10 @@ class RooAbsAnaConvPdf : public RooAbsPdf {
   }
 
   /// Get the resolution model.
-  RooAbsReal const &getModel() const { return _model.arg(); }
+  /// Note that the resolution model is only a configuration object specifying
+  /// the model to convolve the basis functions with; it is not a server of this
+  /// pdf and is not part of its computation graph (see the class documentation).
+  RooAbsReal const &getModel() const { return *_model; }
 
   std::unique_ptr<RooAbsArg> compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext & ctx) const override;
 
@@ -92,11 +95,24 @@ class RooAbsAnaConvPdf : public RooAbsPdf {
 
   double evaluate() const override ;
 
+  bool redirectServersHook(const RooAbsCollection &newServerList, bool mustReplaceAll, bool nameChange,
+                           bool isRecursive) override;
+
+  void ioStreamerPass2() override;
+
   void makeCoefVarList(RooArgList&) const ;
 
   friend class RooConvGenContext ;
 
-  RooRealProxy _model ;   ///< Original model
+  // The original resolution model is intentionally *not* a server of the
+  // RooAbsAnaConvPdf: it is only used to build the convolutions (stored in
+  // _convSet) and for some operations like generation. Keeping it as a server
+  // would pollute the computation graph (and any RooWorkspace or JSON export)
+  // with an object that is never evaluated. It is owned and kept in sync with
+  // the rest of the graph via redirectServersHook(), analogous to how
+  // RooResolutionModel handles its basis function.
+  RooResolutionModel *_model = nullptr; ///< Original resolution model (not a server)
+  bool _ownModel = false;               ///< Flag indicating ownership of _model
   RooRealProxy _convVar ; ///< Convolution variable
 
   RooArgSet* parseIntegrationRequest(const RooArgSet& intSet, Int_t& coefCode, RooArgSet* analVars=nullptr) const ;
@@ -120,7 +136,7 @@ class RooAbsAnaConvPdf : public RooAbsPdf {
 
   mutable RooAICRegistry _codeReg ;         ///<! Registry of analytical integration codes
 
-  ClassDefOverride(RooAbsAnaConvPdf,3) // Abstract Composite Convoluted PDF
+  ClassDefOverride(RooAbsAnaConvPdf, 4) // Abstract Composite Convoluted PDF
 };
 
 #endif

roofit/roofitcore/src/RooAbsAnaConvPdf.cxx

@@ -33,7 +33,8 @@
 ///
 ///  Classes derived from RooResolutionModel implement
 ///  \f[
-///     R_k(x,\bar{b},\bar{c}) = \int \mathrm{basis}_k(x', \bar{b}) \cdot \mathrm{resModel}(x-x',\bar{c}) \; \mathrm{d}x',
+///     R_k(x,\bar{b},\bar{c}) = \int \mathrm{basis}_k(x', \bar{b}) \cdot \mathrm{resModel}(x-x',\bar{c}) \;
+///     \mathrm{d}x',
 ///  \f]
 ///
 ///  which RooAbsAnaConvPdf uses to construct the pdf for [ Phys (x) R ] :
@@ -59,6 +60,40 @@
 ///  advertise the coefficient integration capabilities and implement them respectively.
 ///  Please see RooAbsPdf for additional details. Advertised analytical integrals must be
 ///  valid for all coefficients.
+///
+///  ### The resolution model is a configuration object, not a graph node
+///
+///  The resolution model passed to the constructor is **not** a node of the
+///  computation graph of the RooAbsAnaConvPdf. It is never evaluated directly;
+///  it only serves as a *configuration* object that specifies which resolution
+///  model should be convolved with the basis functions. From it, the
+///  RooAbsAnaConvPdf builds its own internal \f$ \mathrm{basis}_k \otimes
+///  \mathrm{resModel} \f$ convolution objects (one per declared basis function),
+///  and it is *those* convolutions that are the actual value servers of the pdf
+///  and that get evaluated.
+///
+///  Consequently, the resolution model itself is not a server of the
+///  RooAbsAnaConvPdf. It remains accessible via getModel() (for example for
+///  serialization), but it does not appear in the pdf's `servers()` list, in
+///  `getParameters()` / `getVariables()`, or in the printed computation graph.
+///
+///  \note **Behavior change in ROOT 6.42:** in earlier releases the resolution
+///  model was kept as a (non-value, non-shape) server of the RooAbsAnaConvPdf.
+///  As a side effect, importing a RooAbsAnaConvPdf into a RooWorkspace also
+///  dragged the original resolution model into the workspace (and into HS3/JSON
+///  exports), even though it played no role in the computation. As of ROOT 6.42
+///  this is no longer the case: a resolution model that is only used as the
+///  configuration of a RooAbsAnaConvPdf is not imported into the workspace on
+///  its own anymore.
+///
+///  Objects written with older ROOT versions are read back correctly via schema
+///  evolution: the resolution model is dropped as a server, so the *pdf's*
+///  computation graph is the same as for a freshly constructed one. Note,
+///  however, that if such an old file already stored the resolution model as a
+///  standalone RooWorkspace member (which used to happen on import), that member
+///  is *not* retroactively removed from the workspace on read-back -- it simply
+///  is no longer wired into the RooAbsAnaConvPdf. Only newly created workspaces
+///  are guaranteed to be free of the standalone resolution model.
 
 #include "RooAbsAnaConvPdf.h"
 
@@ -93,33 +128,37 @@ RooAbsAnaConvPdf::RooAbsAnaConvPdf() :
 /// Constructor. The supplied resolution model must be constructed with the same
 /// convoluted variable as this physics model ('convVar')
 
-RooAbsAnaConvPdf::RooAbsAnaConvPdf(const char *name, const char *title,
-               const RooResolutionModel& model, RooRealVar& cVar) :
-  RooAbsPdf(name,title), _isCopy(false),
-  _model("!model","Original resolution model",this,(RooResolutionModel&)model,false,false),
-  _convVar("!convVar","Convolution variable",this,cVar,false,false),
-  _convSet("!convSet","Set of resModel X basisFunc convolutions",this),
-  _coefNormMgr(this,10),
-  _codeReg(10)
+RooAbsAnaConvPdf::RooAbsAnaConvPdf(const char *name, const char *title, const RooResolutionModel &model,
+                                   RooRealVar &cVar)
+   : RooAbsPdf(name, title),
+     _isCopy(false),
+     _model{static_cast<RooResolutionModel *>(model.clone(model.GetName()))},
+     _ownModel{true},
+     _convVar("!convVar", "Convolution variable", this, cVar, false, false),
+     _convSet("!convSet", "Set of resModel X basisFunc convolutions", this),
+     _coefNormMgr(this, 10),
+     _codeReg(10)
 {
-  _model.absArg()->setAttribute("NOCacheAndTrack") ;
+   _model->setAttribute("NOCacheAndTrack");
 }
 
 
 
 ////////////////////////////////////////////////////////////////////////////////
 
-RooAbsAnaConvPdf::RooAbsAnaConvPdf(const RooAbsAnaConvPdf& other, const char* name) :
-  RooAbsPdf(other,name), _isCopy(true),
-  _model("!model",this,other._model),
-  _convVar("!convVar",this,other._convVar),
-  _convSet("!convSet",this,other._convSet),
-  _coefNormMgr(other._coefNormMgr,this),
-  _codeReg(other._codeReg)
+RooAbsAnaConvPdf::RooAbsAnaConvPdf(const RooAbsAnaConvPdf &other, const char *name)
+   : RooAbsPdf(other, name),
+     _isCopy(true),
+     _model{other._model ? static_cast<RooResolutionModel *>(other._model->clone(other._model->GetName())) : nullptr},
+     _ownModel{true},
+     _convVar("!convVar", this, other._convVar),
+     _convSet("!convSet", this, other._convSet),
+     _coefNormMgr(other._coefNormMgr, this),
+     _codeReg(other._codeReg)
 {
   // Copy constructor
-  if (_model.absArg()) {
-    _model.absArg()->setAttribute("NOCacheAndTrack") ;
+  if (_model) {
+     _model->setAttribute("NOCacheAndTrack");
   }
   other._basisList.snapshot(_basisList);
 }
@@ -140,8 +179,53 @@ RooAbsAnaConvPdf::~RooAbsAnaConvPdf()
     }
   }
 
+  if (_ownModel) {
+     delete _model;
+  }
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// Forward server redirection to the original resolution model. The resolution
+/// model is not a server of this pdf (it is only used to build the convolutions
+/// and for generation), so it is not redirected by the standard machinery. We
+/// keep its servers in sync here, analogous to RooResolutionModel forwarding
+/// the redirection to its basis function.
+
+bool RooAbsAnaConvPdf::redirectServersHook(const RooAbsCollection &newServerList, bool mustReplaceAll, bool nameChange,
+                                           bool isRecursive)
+{
+   if (_model) {
+      // Pass mustReplaceAll=false: the model may legitimately reference servers
+      // that are not part of this particular redirection, and it is never
+      // evaluated as part of the computation graph anyway.
+      _model->redirectServers(newServerList, false, nameChange);
+   }
+
+   return RooAbsPdf::redirectServersHook(newServerList, mustReplaceAll, nameChange, isRecursive);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+/// Second-pass schema evolution, called by the RooWorkspace after reading.
+///
+/// In class version <= 3, the original resolution model was held in a
+/// RooRealProxy and was therefore a (non-value, non-shape) server of this pdf.
+/// The schema evolution read rule (see LinkDef.h) already recovered the model
+/// pointer into _model, but the stale server link is also restored from the
+/// file via the RooAbsArg server list. We remove it here, once the full graph
+/// is live, so that an object read from an old file has the same clean server
+/// structure as a freshly constructed one. This is safe because there is no
+/// longer a proxy that could resurrect the server link on copy.
+
+void RooAbsAnaConvPdf::ioStreamerPass2()
+{
+   RooAbsPdf::ioStreamerPass2();
+
+   // Force removal (the link may have been added with a reference count > 1):
+   // the model must be completely severed from the server list.
+   if (_model && findServer(*_model)) {
+      removeServer(*_model, true);
+   }
+}
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Declare a basis function for use in this physics model. The string expression
@@ -165,11 +249,10 @@ Int_t RooAbsAnaConvPdf::declareBasis(const char* expression, const RooArgList& p
   }
 
   // Resolution model must support declared basis
-  if (!(static_cast<RooResolutionModel*>(_model.absArg()))->isBasisSupported(expression)) {
-    coutE(InputArguments) << "RooAbsAnaConvPdf::declareBasis(" << GetName() << "): resolution model "
-           << _model.absArg()->GetName()
-           << " doesn't support basis function " << expression << std::endl ;
-    return -1 ;
+  if (!_model->isBasisSupported(expression)) {
+     coutE(InputArguments) << "RooAbsAnaConvPdf::declareBasis(" << GetName() << "): resolution model "
+                           << _model->GetName() << " doesn't support basis function " << expression << std::endl;
+     return -1;
   }
 
   // Instantiate basis function
@@ -188,7 +271,7 @@ Int_t RooAbsAnaConvPdf::declareBasis(const char* expression, const RooArgList& p
   basisFunc->setOperMode(operMode()) ;
 
   // Instantiate resModel x basisFunc convolution
-  RooAbsReal* conv = static_cast<RooResolutionModel*>(_model.absArg())->convolution(basisFunc.get(),this);
+  RooAbsReal *conv = _model->convolution(basisFunc.get(), this);
   _basisList.addOwned(std::move(basisFunc));
   if (!conv) {
     coutE(InputArguments) << "RooAbsAnaConvPdf::declareBasis(" << GetName() << "): unable to construct convolution with basis function '"
@@ -229,14 +312,15 @@ bool RooAbsAnaConvPdf::changeModel(const RooResolutionModel& newModel)
   _convSet.removeAll() ;
   _convSet.addOwned(std::move(newConvSet));
 
-  const std::string attrib = std::string("ORIGNAME:") + _model->GetName();
-  const bool oldAttrib = newModel.getAttribute(attrib.c_str());
-  const_cast<RooResolutionModel&>(newModel).setAttribute(attrib.c_str());
-
-  redirectServers(RooArgSet{newModel}, false, true);
-
-  // reset temporary attribute for server redirection
-  const_cast<RooResolutionModel&>(newModel).setAttribute(attrib.c_str(), oldAttrib);
+  // Replace the stored original resolution model. Since it is not a server of
+  // this pdf, it cannot (and need not) be redirected via redirectServers(): we
+  // simply own a fresh clone of the new model.
+  if (_ownModel) {
+     delete _model;
+  }
+  _model = static_cast<RooResolutionModel *>(newModel.clone(newModel.GetName()));
+  _ownModel = true;
+  _model->setAttribute("NOCacheAndTrack");
 
   return false ;
 }
@@ -255,7 +339,7 @@ RooAbsGenContext* RooAbsAnaConvPdf::genContext(const RooArgSet &vars, const RooD
                       const RooArgSet* auxProto, bool verbose) const
 {
   // Check if the resolution model specifies a special context to be used.
-  RooResolutionModel* conv = dynamic_cast<RooResolutionModel*>(_model.absArg());
+  RooResolutionModel *conv = _model;
   assert(conv);
 
   std::unique_ptr<RooArgSet> modelDep {_model->getObservables(&vars)};
@@ -297,9 +381,8 @@ bool RooAbsAnaConvPdf::isDirectGenSafe(const RooAbsArg& arg) const
 {
 
   // All direct generation of convolution arg if model is truth model
-  if (!TString(_convVar.absArg()->GetName()).CompareTo(arg.GetName()) &&
-      dynamic_cast<RooTruthModel*>(_model.absArg())) {
-    return true ;
+  if (!TString(_convVar.absArg()->GetName()).CompareTo(arg.GetName()) && dynamic_cast<RooTruthModel *>(_model)) {
+     return true;
   }
 
   return RooAbsPdf::isDirectGenSafe(arg) ;

roofit/roofitcore/src/RooResolutionModel.cxx

@@ -60,6 +60,14 @@
  *  The choice of basis returned by basisCode() is guaranteed not to change
  *  during the lifetime of a RooResolutionModel object.
  *
+ *  When a resolution model is passed to a RooAbsAnaConvPdf (e.g. RooDecay), it
+ *  acts as a *configuration* object rather than as a node of that pdf's
+ *  computation graph: the RooAbsAnaConvPdf builds its own internal
+ *  basis-function convolutions from it and does not keep it as a server. As of
+ *  ROOT 6.42, such a "config-only" resolution model is therefore not imported
+ *  into a RooWorkspace together with the RooAbsAnaConvPdf that consumes it. See
+ *  RooAbsAnaConvPdf for details.
+ *
  */
 
 #include "TClass.h"

roofit/roofitcore/test/CMakeLists.txt

@@ -86,7 +86,8 @@ ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit
 ROOT_ADD_GTEST(testRooMulti testRooMulti.cxx LIBRARIES RooFitCore RooFit)
 ROOT_ADD_GTEST(testRooRombergIntegrator testRooRombergIntegrator.cxx LIBRARIES MathCore RooFitCore)
 ROOT_ADD_GTEST(testRooSimultaneous testRooSimultaneous.cxx LIBRARIES RooFitCore RooFit)
-ROOT_ADD_GTEST(testRooTruthModel testRooTruthModel.cxx LIBRARIES RooFitCore RooFit)
+ROOT_ADD_GTEST(testRooTruthModel testRooTruthModel.cxx LIBRARIES RooFitCore RooFit
+  COPY_TO_BUILDDIR ${CMAKE_CURRENT_SOURCE_DIR}/rooAbsAnaConvPdf_classV3.root)
 
 if (roofit_multiprocess)
   ROOT_ADD_GTEST(testTestStatisticsPlot TestStatistics/testPlot.cxx LIBRARIES RooFitMultiProcess RooFitCore RooFit