[Python] Set memory policy to "strict" by guitargeek · Pull Request #13593 · root-project/root

guitargeek · 2023-09-04T13:04:33Z

The ROOT Python interfaces have many memory leaks, which is a major pain point for people using it for long-running scripts in batch jobs.

One source of memory leaks was indentified to be the "heuristic memory policy" of cppyy. This means that cppyy assumes that every non-const pointer member function argument was interpreted as the object taking ownership if the argument.

For examle, take the non-owning RooLinkedList container. It has a RooLinkedList::Add(RooAbsArg *arg) method. ROOT wrongly assumes that this means the RooLinkedList takes ownership of arg, and it drops the ROOT ownership. Nobody feels responsible for deleting the object anymore, and there is a memory leak or arg.

That particular leak was reported in this forum post:
https://root-forum.cern.ch/t/memory-leak-in-fits/56249

Function parameters of type T * are very common in ROOT, and only rarely do they imply ownership transfer. So changing the memory policy to "strict" would surely fix also many other memory leaks that are not reported so far. In fact, upstream cppyy doesn't even enable this heuristic memory policy anymore by default! So moving ROOT also to the strict memory policy closes the gap between ROOT and cppyy.

The potential drawback of this change are crashes in usercode if memory is not properly managed. But these problems should either be fixed by:

the user
dedicated pythonizations for these methods to manage shared ownership via Python reference counters (i.e., setting the parameter as an attribute of the object that the member function was called on)

This follows up on PR #4294, in particular it reverts guitargeek@3a12063.

Note that owning TCollection, TSeqCollection, and TList instances are Pythonized to preserve the old behavior of dropping Python ownership of added elements, as that was the case where the memory heuristic was correct.

guitargeek · 2024-02-13T17:57:42Z

Hi @vepadulano, thanks a lot for your review! I have updated the PR and the associated roottest PR

dpiparo · 2025-01-02T09:29:40Z

restarted all builds due to the issues with the server serving files for the tests.

vepadulano

I'm coming back to this old PR, I still agree with my previous statement that this is the right direction, but I see more potential misunderstandings and issues creeping up. I'm thinking about approaches we could employ to mitigate them, e.g. by running also these changes in CMSSW CI, or by thinking about more Pythonizations to include (e.g. in TList), or by thinking about if we can add more warnings for users in situations that lead to dangling pointers.

Also, a question that becomes more important now, are all of these changes still required in light of the recent and future improvements to the cppyy we use?

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py

bindings/pyroot/pythonizations/test/tcollection_listmethods.py

bindings/pyroot/pythonizations/test/tseqcollection_listmethods.py

README/ReleaseNotes/v636/index.md

vepadulano · 2026-01-07T14:48:06Z

README/ReleaseNotes/v636/index.md

+**Note:** You can change back to the old policy by calling
+`ROOT.SetMemoryPolicy(ROOT.kMemoryHeuristics)` after importing ROOT, but this
+should be only used for debugging purposes and this function might be removed
+in the future!


We should make this perhaps explicit at usage by also raising a warning from the function when calling it.

We learned from the ROOT workshop that our users also prefer clear deprecation and removal schedules. Should we aim for removing ROOT.SetMemoryPolicy in 6.42? If people really want to change the policy, they can still use the Cppyy API after the removal of the ROOT wrapper though, so there is an escape hatch that makes the removal seem a bit less drastic. Of course we don't mention that in the release notes 🙂

I have added a clear deprecation and removal warning to the release notes and to ROOT.SetMemoryPolicy() in the latest iteration of this PR.

guitargeek · 2026-01-07T15:34:11Z

Hi @vepadulano, thanks for coming back to this! Let's try to make progress then and merge this soon 🙂

I'm coming back to this old PR, I still agree with my previous statement that this is the right direction, but I see more potential misunderstandings and issues creeping up. I'm thinking about approaches we could employ to mitigate them, e.g. by running also these changes in CMSSW CI, or by thinking about more Pythonizations to include (e.g. in TList), or by thinking about if we can add more warnings for users in situations that lead to dangling pointers.

Good points. I'll address them once we see where we stand with a fresh CI run.

Also, a question that becomes more important now, are all of these changes still required in light of the recent and future improvements to the cppyy we use?

The upcoming cppyy rebase on libinterop is more happening in the backend, and we have no frontend development lined up that change the behavior of these memory heuristics. But what we should consider is that it's better to not change both backend and frontend too much in one release, because it makes potential regressions harder to fix. If we assume that the backend upgrade will hit in 6.42, then this means the best time to update the memory policy is now for 6.40. Otherwise, it would be wiser to wait for 6.44, but this is quite far away.

guitargeek · 2026-01-09T16:45:29Z

Hi @vepadulano, I have suggested a TList Pythonization now that might address your concerns.

The proposal is that when adding to owning TCollections (checked with IsOwner()), the ownership is always relinquished on the Python side, just like the old memory heuristics implied.

So that means that the behavior change is only for non-owning TCollections, which now add borrowed references instead of letting the elements leak when added from Python.

Is that an acceptable compromise? (no need to rush an answer, I still have to fix test failures with Python 3.14 for Fedora Rawhide 🙂 )

vepadulano

Thanks! I like the direction, I left a few more comments/questions.

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py

README/ReleaseNotes/v640/index.md

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py

guitargeek · 2026-03-02T11:28:05Z

So from the latest changes I guess that the default behaviour of TList is that it owns its elements, do you agree?

That's true only for lists returned by this create_tseqcollection function in the test, where it called SetOwner(True) inside. So one has to call SetOwner(False) afterwards to get a non-owning collection.

I agree that this is confusing. I'll make some improvement suggestions.

guitargeek · 2026-03-02T12:10:11Z

Okay I have figured out a simpler way to manage the object ownership in these T(Seq)Collection tests, which is flexible enough so it can also be spun of into a separate PR:

[Python] Don't rely on possible memory leaks in T(Seq)Collection tests #21449

guitargeek · 2026-03-03T09:39:57Z

Hi @vepadulano! This PR is greatly simplified now, because I factored the technical changes beyond the one line that changes the memory policy out into separate PRs that are merged now.

Once our own CI is green, I'll request a CMSSW test run and then we can proceed here from my side.

Clear the TList at the end of the `test_delitem` unit test in tseqcollection_itemaccess.py to make sure the contained list elements are still alive when the list is cleared. Otherwise, the elements might be deleted before the containing list, depending on the order or garbage collection.

The ROOT Python interfaces have many memory leaks, which is a major pain point for people using it for long-running scripts in batch jobs. One source of memory leaks was indentified to be the "heuristic memory policy" of cppyy. This means that cppyy assumes that every non-const pointer member function argument was interpreted as the object taking ownership if the argument. For examle, take the non-owning RooLinkedList container. It has a `RooLinkedList::Add(RooAbsArg *arg)` method. ROOT wrongly assumes that this means the RooLinkedList takes ownership of arg, and it drops the ROOT overship. Nobody feels responsible for deleting the object anymore, and there is a memory leak or `arg`. That particular leak was reported in this forum post: https://root-forum.cern.ch/t/memory-leak-in-fits/56249 Function parameters of type `T *` are very common in ROOT, and only rarely do they imply ownership transfer. So changing the memory policy to "strict" would surely fix also many other memory leaks that are not reported so far. In fact, upstream cppyy doesn't even enable this heuristic memory policy anymore by default! So moving ROOT also to the strict memory policy closes the gap between ROOT and cppyy. The potential drawback of this change are crashes in usercode if memory is not properly managed. But these problems should either be fixed by: * the user * dedicated pythonizations for these methods to manage shared ownership via Python reference counters (i.e., setting the parameter as an attribute of the object that the member function was called on) This follows up on PR root-project#4294, in particular it reverts 3a12063. Note that owning **TCollection**, **TSeqCollection**, and **TList** instances are Pythonized to preserve the old behavior of dropping Python ownership of added elements, as that was the case where the memory heuristic was correct.

guitargeek · 2026-03-03T15:36:58Z

Hi @smuzaffar, could you please test this PR with CMSSW? It's a potentially fragile change of behavior in our Python interface. Thanks!

vepadulano

Thanks! Although the changes are much fewer now, I would still run the CI with a clean build and with the Python wheel workflow.

vepadulano · 2026-03-06T09:15:18Z

bindings/pyroot/pythonizations/python/ROOT/_facade.py

+        msg = """ROOT.SetHeuristicMemoryPolicy() is deprecated and will be removed in ROOT 6.42.
+        Since ROOT 6.40, the heuristic memory policy is disabled by default, and with
+        ROOT 6.42 it won't be possible to re-enable it with ROOT.SetHeuristicMemoryPolicy(),
+        which was only meant to be used during a transition period and will be removed.
+        """


I believe we should not force a too short deprecation period, following multiple suggestions from the ROOT workshop. 2 ROOT releases should probably be the minimum, i.e. 6.44

Also, we need to keep track of when this should be removed either internally or by a condition to turn the warning into an error based on the ROOT version here.

vepadulano · 2026-03-06T09:18:35Z

README/ReleaseNotes/v640/index.md

+```Python
+obj = ROOT.MyClass()
+my_instance.foo(obj)
+del obj  # C++ may still hold a pointer: dangling reference


I would be more explicit here, it's not clear what the problem is in practice.

Suggested change

del obj # C++ may still hold a pointer: dangling reference

del obj # C++ may still hold a pointer: dangling reference. When the Python object is deleted, the memory associated with the C++ object is also freed. But the C++ side may want to delete the object as well, for instance if the destructor of the class of `my_instance` also calls `delete obj`.

vepadulano · 2026-03-06T09:26:52Z

README/ReleaseNotes/v640/index.md

+
+ 1. **Keep the Python object alive**
+
+    Assign the object to a Python variable that lives at least as long as the C++ reference.


Maybe I would show an example of a Pythonization lifeline

vepadulano · 2026-03-06T09:28:26Z

README/ReleaseNotes/v640/index.md

+
+after importing ROOT.
+
+This option is intended **for debugging only and will be removed in ROOT 6.42**.


Again I would be more relaxed with the deprecation period.

guitargeek requested review from Axel-Naumann and vepadulano as code owners September 4, 2023 13:04

guitargeek self-assigned this Sep 4, 2023

guitargeek added the in:Python Interface label Sep 4, 2023

guitargeek mentioned this pull request Sep 4, 2023

[RF][PyROOT] Fix memory leaks with RooFit command arguments in PyROOT #13594

Closed

guitargeek changed the title ~~[PyROOT] Set memory policy to strict~~ [PyROOT] Set memory policy to "strict" Sep 4, 2023

guitargeek force-pushed the pyroot_memory_policy_1 branch from 7b2132c to a2fe3d5 Compare September 4, 2023 13:06

guitargeek mentioned this pull request Sep 4, 2023

[PyROOT] Adapt tests to the new default memory policy "strict" root-project/roottest#1011

Closed

guitargeek force-pushed the pyroot_memory_policy_1 branch from a2fe3d5 to f3aa24f Compare September 4, 2023 16:26

root-project deleted a comment from phsft-bot Sep 4, 2023

guitargeek force-pushed the pyroot_memory_policy_1 branch from f3aa24f to b147011 Compare September 4, 2023 22:48

guitargeek requested review from bellenot and couet as code owners September 4, 2023 22:48

guitargeek force-pushed the pyroot_memory_policy_1 branch 2 times, most recently from b7f0c8f to 4eadd14 Compare September 5, 2023 08:40

root-project deleted a comment from phsft-bot Sep 5, 2023

root-project deleted a comment from phsft-bot Feb 12, 2024

guitargeek force-pushed the pyroot_memory_policy_1 branch from 2c1dc5d to fe2b100 Compare February 12, 2024 17:26

root-project deleted a comment from phsft-bot Feb 12, 2024

guitargeek requested a review from vepadulano February 13, 2024 17:57

guitargeek force-pushed the pyroot_memory_policy_1 branch from fe2b100 to 819ccd4 Compare August 27, 2024 10:31

root-project deleted a comment from phsft-bot Aug 27, 2024

couet removed their request for review December 4, 2024 09:01

guitargeek force-pushed the pyroot_memory_policy_1 branch from 819ccd4 to bb5d158 Compare January 2, 2025 08:36

dpiparo closed this Jan 2, 2025

dpiparo reopened this Jan 2, 2025

vepadulano reviewed Jan 7, 2026

View reviewed changes

guitargeek mentioned this pull request Jan 9, 2026

[Python] Implement ruff linter suggestions #20837

Merged

vepadulano requested changes Jan 12, 2026

View reviewed changes

guitargeek mentioned this pull request Jan 13, 2026

[hist] Delete TMultiGraph::Add(TMultiGraph*, Option_t*) overload #20869

Merged

vepadulano reviewed Jan 13, 2026

View reviewed changes

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py Outdated Show resolved Hide resolved

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py Outdated Show resolved Hide resolved

This was referenced Mar 2, 2026

[Python] Don't rely on possible memory leaks in T(Seq)Collection tests #21449

Merged

[Python] Don't rely on heuristic memory policy when passing ownership #21451

Merged

guitargeek added 2 commits March 3, 2026 14:09

vepadulano requested changes Mar 6, 2026

View reviewed changes

smuzaffar mentioned this pull request Mar 6, 2026

[Python] Set memory policy to "strict" cms-sw/root#230

Open

	del obj # C++ may still hold a pointer: dangling reference
	del obj # C++ may still hold a pointer: dangling reference. When the Python object is deleted, the memory associated with the C++ object is also freed. But the C++ side may want to delete the object as well, for instance if the destructor of the class of `my_instance` also calls `delete obj`.


		1. Keep the Python object alive

		Assign the object to a Python variable that lives at least as long as the C++ reference.


		after importing ROOT.

		This option is intended for debugging only and will be removed in ROOT 6.42.

Conversation

guitargeek commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guitargeek commented Feb 13, 2024

Uh oh!

dpiparo commented Jan 2, 2025

Uh oh!

vepadulano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Jan 7, 2026

Uh oh!

guitargeek commented Jan 9, 2026

Uh oh!

vepadulano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitargeek commented Mar 2, 2026

Uh oh!

guitargeek commented Mar 2, 2026

Uh oh!

guitargeek commented Mar 3, 2026

Uh oh!

guitargeek commented Mar 3, 2026

Uh oh!

vepadulano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guitargeek commented Sep 4, 2023 •

edited

Loading