Fix memory leak in appsi LegacySolverInterface wrapper

[Marl0nL]

Fixes # .

Prevents a memory leak observed when iteratively solving via the LegacySolverInterface wrapper.

Summary/Motivation:

Observed an issue with significant increase in memory use when iteratively solving a model (that fundamentally wasn't changing size) via SolverFactory('appsi_cbc'), which uses the the APPSI LegacySolverInterface.
Found that the growing memory use was the symbol maps.

The memory leak can be avoided by not using the LegacySolverInterface wrapper in the first place, but it seems worth fixing the legacy wrapper to avoid unsuspecting users swapping "cbc" for "appsi_cbc" when calling SolverFactory(), and then having memory issues.

While I can't think of why someone would intensionally leverage the current behaviour, it's possible I'm naive to other usage paradigms which may conflict with this fix. If that is the case, it may be preferable to add a new option which handles symbol map deletion or retention.

Changes proposed in this PR:

• LegacySolverInterface.solve() now ensures the clean-up of symbol maps after solving, which prevents an effective memory-leak when iteratively re-solving a model via the interface.
• Added test_legacy_leak.py, which can be used to demonstrate the memory leak issue, and the resolution thereof.
  I recommend checking out commit 65402fa992afeae269decc9e69b1e4bac9c47d37, which adds only the test script, to verify the existence of the issue prior to the application of the fix in subsequent commits.

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.94%. Comparing base (34a3877) to head (090fc81).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3915   +/-   ##
=======================================
  Coverage   89.94%   89.94%           
=======================================
  Files         902      902           
  Lines      106457   106465    +8     
=======================================
+ Hits        95748    95762   +14     
+ Misses      10709    10703    -6     

Flag	Coverage Δ
builders	29.18% <0.00%> (+<0.01%)	⬆️
default	86.24% <100.00%> (?)
expensive	35.64% <0.00%> (?)
linux	87.39% <100.00%> (-2.04%)	⬇️
linux_other	87.39% <100.00%> (+<0.01%)	⬆️
oldsolvers	28.11% <0.00%> (+<0.01%)	⬆️
osx	82.72% <100.00%> (+<0.01%)	⬆️
win	85.83% <100.00%> (+<0.01%)	⬆️
win_other	85.83% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

[jsiirola]

First, this is great. It also points to a fundamental design flaw in the old model model.solutions cache - this is inherently a source of memory "leaks" (not a leak, per se, but an implicit, monotonically increasing cache of questionable value).

I just looked, and the only thing that was documented in The Book was model.solutions.load_from() ... we may be able to start reworking that entire infrastructure in advance of Pyomo 7.

[jsiirola]

Instead of deleting the symbol map, if we follow this suggestion of storing the SymbolMap on the legacy_results as _smap (which I think I like), can we just never call model.solutions.add_symbol_map() in the first place?

[jsiirola]

Following up here: I spoke with @michaelbynum: we think:

• both the solutions.delete_symbol_map() calls and the solutions.add_symbol_map() call above can be deleted.
• We can probably get rid of the delete_legacy_soln logic and just move:

  legacy_results.solution.insert(legacy_soln)
  legacy_results._smap_id = id(symbol_map)
  legacy_results._smap = symbol_map

  into the elif results.best_feasible_objective is not None: block above.
• finally, have the _smap_id default to None:

  --- a/pyomo/contrib/appsi/base.py
  +++ b/pyomo/contrib/appsi/base.py
  @@ -1601,8 +1601,7 @@ class LegacySolverInterface:
           symbol_map.bySymbol = dict(self.symbol_map.bySymbol)
           symbol_map.aliases = dict(self.symbol_map.aliases)
           symbol_map.default_labeler = self.symbol_map.default_labeler
  -        model.solutions.add_symbol_map(symbol_map)
  -        legacy_results._smap_id = id(symbol_map)
  +        legacy_results._smap_id = None
  
           delete_legacy_soln = True
           if load_solutions:

[Marl0nL]

I've implemented this, and also removed the block below from contrib/solver/common/base.py, as I believe it should no longer be needed, as the only reference to model.solutions was the call to model.solutions.add_symbol_map().

        if not hasattr(model, 'solutions'):
            # This logic gets around Issue #2130 in which
            # solutions is not an attribute on Blocks
            from pyomo.core.base.PyomoModel import ModelSolutions

            setattr(model, 'solutions', ModelSolutions(model))

[jsiirola]

When is tracemalloc not available?

[Marl0nL]

@jsiirola on the first CI run, I saw that the run with PyPy 3.11 failed with the following:

pyomo/contrib/appsi/tests/test_legacy_leak.py:3: in <module>
    import tracemalloc
/opt/hostedtoolcache/PyPy/3.11.13/x64/lib/pypy3.11/tracemalloc.py:9: in <module>
    from _tracemalloc import *
E   ModuleNotFoundError: No module named '_tracemalloc'

I'm not familiar with pypy, so I assumed perhaps it excludes some built-in packages like tracemalloc. A quick google now indicates that tracemalloc should be available for pypy 3.11, so I'm unsure what actually went wrong there.

[jsiirola]

That makes sense. I can confirm that it doesn't import for me with the current pypy release. As a matter of style, we generally use the following:

from pyomo.common.dependencies import attempt_import

# Note: tracemalloc is not always available, e.g., under PyPy
tracemalloc, tracemalloc_available = attempt_import('tracemalloc')