Clone Solution objects used by Reactor / ReactorSurface

[speth]

Changes proposed in this pull request

• Partially implements changes envisioned in Eliminate traps associated with accessing ThermoPhase objects in use by Reactors enhancements#201
• Provides user interface side of Improve ReactorNet handling of bulk phase interactions enhancements#202
• Currently, breaks some things that used to work (see various tweaks in the test suite)

If applicable, fill in the issue number this pull request is fixing

Closes #

If applicable, provide an example illustrating new features this pull request is introducing

Checklist

• The pull request includes a clear description of this code change
• Commit messages have short titles and reference relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• The pull request is ready for review

[codecov]

Codecov Report

Attention: Patch coverage is 82.43243% with 26 lines in your changes missing coverage. Please review.

Project coverage is 74.30%. Comparing base (b105c52) to head (f410da9).

Files with missing lines	Patch %	Lines
src/zeroD/ReactorSurface.cpp	75.00%	3 Missing and 4 partials ⚠️
src/oneD/MultiNewton.cpp	0.00%	5 Missing ⚠️
src/kinetics/Kinetics.cpp	88.88%	0 Missing and 4 partials ⚠️
interfaces/cython/cantera/reactor.pyx	89.28%	3 Missing ⚠️
src/base/Solution.cpp	88.88%	1 Missing and 2 partials ⚠️
src/zeroD/ReactorBase.cpp	50.00%	2 Missing ⚠️
include/cantera/transport/Transport.h	0.00%	1 Missing ⚠️
src/thermo/ThermoPhase.cpp	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1921      +/-   ##
==========================================
+ Coverage   74.28%   74.30%   +0.01%     
==========================================
  Files         448      448              
  Lines       55744    55862     +118     
  Branches     9190     9211      +21     
==========================================
+ Hits        41411    41507      +96     
- Misses      11232    11248      +16     
- Partials     3101     3107       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[speth]

With this implementation now largely working, I have a couple of observations in relation to the discussion in Cantera/enhancements#201:

• We cannot rely on always cloning the Solution objects used in a reactor network. The most severe issue with this is that CustomRate objects implemented as Python lambdas cannot be serialized. This is currently the reason the custom_reactions.py example fails to run.
• We cannot make cloning Solution objects the default behavior in this Cantera version. Based on the changes to various test cases and examples, it is clear that there is a fair amount of existing code that relies on the incidental sharing of Solution states. While identifying these cases in the test suite is relatively easy, it is not so easy elsewhere and would be a significant breaking change.

As such, I think the next steps for this PR are:

• Implement an additional clone argument to the Reactor and ReactorSurface constructors, which will default to None, In Cantera 3.2, if no value for this argument is provided, the Solution will not be cloned and a warning will be issued. In Cantera 3.3, we can make clone=True the default, while clone=False will still be available for special cases.
• Add a check during ReactorNet::initialize to identify whether any Solution objects are being used multiple times within the network. In Cantera 3.2, doing so will result in a warning, which can then be converted to an error in Cantera 3.3.

[ischoegl]

@speth ... thanks for your work on this, as well as the thoughts for the roadmap, which looks 👍.

I was somewhat surprised that CustomRate objects with Python lambdas cause failures, which is not among the consequences that I would have expected. On second thought, I assume that it has to do with those not being part of the YAML serialization, and thus cannot be cloned? If this were the case, I'd suggest simply throwing an error if there's an attempt to clone a mechanism that includes CustomRate (I have not started a review, so it may already be there). I assume this is purely limited to Python lambdas, as C++ has no way of ensuring that Python objects persist in memory? I.e., CustomRate with Func1 objects should work?

Beyond, it appears that #1923 fixes other sample tests that are currently failing.

[speth]

I was somewhat surprised that CustomRate objects with Python lambdas cause failures, which is not among the consequences that I would have expected. On second thought, I assume that it has to do with those not being part of the YAML serialization, and thus cannot be cloned?

I didn't anticipate this either, but it's pretty clear in retrospect. Indeed, the issue is that such lambdas cannot be serialized.

If this were the case, I'd suggest simply throwing an error if there's an attempt to clone a mechanism that includes CustomRate (I have not started a review, so it may already be there). I assume this is purely limited to Python lambdas, as C++ has no way of ensuring that Python objects persist in memory? I.e., CustomRate with Func1 objects should work?

This affects all CustomRate objects, including ones built on Func1, since the CustomFunc1Rate::getParameters method is not implemented (or rather, explicitly raises a NotImplementedError). In principle, we could implement serialization of Func1-based rates, but it's not strictly necessary. The fallback is for the user to instantiate independent Solution objects and call the Reactor constructor with the clone=False option.

Likewise, ExtensibleRate objects that don't implement the set_parameters / get_parameters pair will require this alternative approach, as will any other user-defined model that doesn't fully implement serialization.

---

The "next steps" I mentioned before are now completed, but I'm currently struggling with how to get this to play nice with the generated CLib. I added a new method, shared_ptr<Solution> ReactorBase::solution() for accessing the Solution object used by a Reactor. Depending on whether a clone was created or not, this is either the new cloned object or just the same object that was passed into the constructor. In the first case, I understand why CLib is struggling -- that cloned Solution isn't already in the cabinet, but I'm not sure what the right way to update the code generation for that is. In the second case, this accessor also doesn't seem to be working, and I can't figure out why.

[ischoegl]

Thanks, @speth for further context.

[...] In principle, we could implement serialization of Func1-based rates, but it's not strictly necessary. The fallback is for the user to instantiate independent Solution objects and call the Reactor constructor with the clone=False option.
Likewise, ExtensibleRate objects that don't implement the set_parameters / get_parameters pair will require this alternative approach, as will any other user-defined model that doesn't fully implement serialization.

It should be pretty straight-forward to come up with Func1 serialization so things work out of the box. For ExtensibleRate, it's obviously up to the users. This clearly goes beyond this PR.

The "next steps" I mentioned before are now completed, but I'm currently struggling with how to get this to play nice with the generated CLib. I added a new method, shared_ptr<Solution> ReactorBase::solution() for accessing the Solution object used by a Reactor. Depending on whether a clone was created or not, this is either the new cloned object or just the same object that was passed into the constructor. In the first case, I understand why CLib is struggling -- that cloned Solution isn't already in the cabinet, but I'm not sure what the right way to update the code generation for that is. In the second case, this accessor also doesn't seem to be working, and I can't figure out why.

Hmm. This makes sense - it wasn't straightforward to get the initial accessors to work properly (most relevant example: sol_adjacent); I managed to address this with the uses field. While the Python code should be relatively easy to follow (I did go back and revised), the conditionals in Jinja are quite difficult to make readable (this may be improved by Cantera/enhancements#232).

At the same time, the existing logic may be sufficient to handle this situation using your new shared_ptr<Solution> ReactorBase::solution() method. The cabinet entries should be created during constructor calls for Reactor and Domain1D objects. Fortunately, the Jinja logic for constructors is among the more straightforward/better commented ones:

clib-constructor: |-
  ## CLib constructor template: instantiates new object
  // constructor: {{ cxx_wraps }}
  try {
      {% for line in lines %}
      ## add lines used for CLib/C++ variable crosswalk
      {{ line }}
      {% endfor %}
      {% if shared %}
      ## instantiated object manages associated objects (see: sol3_newSolution)
      ## storage of associated objects requires id (handle) of new object
      auto obj = {{ cxx_name }}({{ ', '.join(cxx_args) }});
      int id = {{ base }}Cabinet::add(obj);
      {% for typ, getter in shared %}
      ## add associated objects (see: sol3_newSolution, sol3_adjacent)
      if (obj->{{ getter }}()) {
          {{ typ }}Cabinet::add(obj->{{ getter }}(), id);
      }
      {% endfor %}
      return id;
      {% else %}{# not shared #}
      ## instantiated object has no associated objects; no id is needed
      return {{ base }}Cabinet::add({{ cxx_name }}({{ ', '.join(cxx_args) }}));
      {% endif %}{# shared #}
  } catch (...) {
      return handleAllExceptions({{ error[0] }}, {{ error[1] }});
  }

Specifically, you'd need to get the Solution object clones into the following portion via the shared argument:

      {% for typ, getter in shared %}
      ## add associated objects (see: sol3_newSolution, sol3_adjacent)
      if (obj->{{ getter }}()) {
          {{ typ }}Cabinet::add(obj->{{ getter }}(), id);
      }
      {% endfor %}

As the cloned Solution is a managed object, the CLib destructors need to use the uses field as well.

As an aside, I noticed that there are some stray sol3_* references left in the ## Jinja comment blocks 😢 (should be sol_*).

PS: In a nutshell, you may need to update ctreactor_auto.yaml as follows:

- name: new
  wraps: newReactorBase
  uses: [solution]
  [...]
- name: del
  uses: [solution]

I haven't tested it, though.

[ischoegl]

In other contexts (specifically Python), we use contents. I don't have strong opinions, though.

Fwiw, we're not consistent in Python, either:

    property thermo:
        """
        The `ThermoPhase` object representing the reactor's contents.
        """
        def __get__(self):
            self.rbase.restoreState()
            return self._contents

The docstring here isn't correct any longer as it has become the Solution object (it refers to the same object as the new C++ method). The situation is the also same for an existing kinetics property. It still all works due to inheritance, but is overall misleading.

Obviously, this is a pre-existing condition. Introducing the C++ method does, however, provide an opportunity to make things consistent across all APIs.