fix(runtime): reduce memory growth in long-running multi-simulation workloads#810
fix(runtime): reduce memory growth in long-running multi-simulation workloads#810isaac-gumbrell wants to merge 2 commits into
Conversation
During long simulations and multi-episode RL training, several data structures grew without bound, causing RAM to climb from ~5GB to 12GB+ per model before crashing. Fixes: - MarketRole.open_auctions: fix - to -= typo so cleared products are actually removed from the set instead of discarding the result - MarketRole.results: prune stale entries after storing market results, preventing the full clearing history from accumulating in memory - calculate_content_size: replace shallow sys.getsizeof() with deep recursive sizing that correctly measures DataFrames, numpy arrays, torch tensors, and nested dicts -- the output buffer 300MB flush threshold was effectively never triggered - UnitsOperator.valid_orders: only accumulate accepted orders for dispatch tracking; rejected orders were kept unnecessarily - WriteOutput.on_stop: dispose SQLAlchemy engine to release connection pool on shutdown - TensorBoardLogger: add explicit close() method that flushes the SummaryWriter and disposes the DB engine; call it before creating a new logger each episode instead of relying on __del__/GC - World.reset: clear all @lru_cache'd forecast algorithm caches that retained full DataFrames and stale unit/config references across RL training episodes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #810 +/- ##
==========================================
- Coverage 80.51% 80.46% -0.05%
==========================================
Files 56 56
Lines 9056 9109 +53
==========================================
+ Hits 7291 7330 +39
- Misses 1765 1779 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
isaac-gumbrell
changed the title
maurerle
left a comment
maurerle
left a comment
There was a problem hiding this comment.
Hi @isaac-gumbrell - thanks for this PR, which includes a lot of well intended improvements.
I added some comments and remarks to the optimizations.
In general, I don't think that there is a lot to gain from doing memory management on top of pythons own memory management cycle.
| try: | ||
| self.db.dispose() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
I would not fail silently here but still raise or log the error.
Besides, after on_stop, the __del__ function should be called, which the reference count of self.db to 0, which immediately should call the objects __del__ function, terminate the active db connections and not create memory leaks.
Did you actively encouter issues with this?
| marketconfig = self.registered_markets[content["market_id"]] | ||
| self.valid_orders[marketconfig.product_type].extend(orderbook) | ||
| # Only keep accepted orders for dispatch tracking — | ||
| # rejected orders are not needed and cause unbounded growth. |
There was a problem hiding this comment.
I remember that there was something about this, but it looks more right now.
The valid_orders are not needed for dispatch tracking though, but are kept so that the agent has information about all past orders.
So please adjust/remove the comment.
It is better to describe this in the commit message instead.
|
|
||
| Uses recursive traversal to account for nested structures, | ||
| DataFrames, numpy arrays, and torch tensors — unlike sys.getsizeof | ||
| which only measures the shallow container size. |
There was a problem hiding this comment.
Is the shallow size different? If so, when? Can we get a test for this?
In my short try-out there were no bigger differences between those two
| # Keep only results that data-request handlers may still need | ||
| # (those whose product_start has not yet passed). | ||
| now = timestamp2datetime(self.context.current_timestamp) | ||
| self.results = [r for r in self.results if r.get("product_start", now) >= now] |
There was a problem hiding this comment.
Currently, the self.results in the base_market are there to be available for future market mechanisms, and for inspection at runtime.
The results only contain the meta data of the market (cleared volume and so on).
I don't think that this has a large impact on simulation resources..?
Though this is a relevant point here.
| # Explicitly close the previous logger to release its DB engine | ||
| # and SummaryWriter file handles before creating a new one. | ||
| if ( | ||
| hasattr(self, "tensor_board_logger") |
There was a problem hiding this comment.
self.tensor_board_logger is initialized with None, so the hasattr can be removed and is just slop at this point
| try: | ||
| self.db.dispose() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
silently failing is unclean
I would still keep the close function content in __del__
| hasattr(self, "tensor_board_logger") | ||
| and self.tensor_board_logger is not None | ||
| ): | ||
| self.tensor_board_logger.close() |
There was a problem hiding this comment.
reassigning the tensor_board_logger, sets it reference count to 0, which immediately should call the objects __del__ function and not create memory leaks.
Did you actively encouter issues with this?
>>> class Tomate:
... def __init__(self):
... print('Tomate created')
...
... def __del__(self):
... print('Tomate destroyed')
...
...
... a = Tomate() # 1st instance
... a = Tomate() # 2nd instance, overwriting the name 'a'
...
Tomate created
Tomate created
Tomate destroyed| calculate_naive_residual_load.cache_clear() | ||
| calculate_naive_congestion_signal.cache_clear() | ||
| calculate_naive_renewable_utilisation.cache_clear() | ||
|
|
There was a problem hiding this comment.
The lru_cache only keeps up to maxsize=128 elements. This does not increase in size. I would spare the complexity of keeping track of the low-level LRU_Cache in the world ourselves..?
|
Potentially reloading the world makes it slower; double-check that. Discussion with @isaac-gumbrell . |
This removes the redundant `DsmCapacityHeuristicBalancingNegStrategy/DsmCapacityHeuristicBalancingPosStrategy` - it is fine to have both names resolving to the same class, but not good to have duplicated codebase without benefit. It looks similar, and actually was. Additionally a bug in the base_market for the list of currently open auctions is fixed. This did not have any known use yet. This was also found in #810 Also fixes #823
|
I'd like to close this as the valid findings were addressed in #827 and there is not much else. |
3 participants
Description
This PR addresses several likely contributors to memory growth observed when running many simulations concurrently over long periods.
During sustained multi-run workloads, memory usage increases significantly over time even after the replay buffer is fully populated. This PR patches a number of suspected retention points and cache / object-lifecycle issues to reduce unnecessary memory accumulation.
Changes in this PR:
Checklist
docfolder updates etc.)Additional Notes
These combinations of changes have entirely eliminated memory creep without altering results.