Repository navigation
[tmva][sofie] Don't copy the Session in the RDataFrame Python tutorial - #23624
Merged
Merged
Conversation
guitargeek
force-pushed
the
sofie-rdf-fixup
branch
from
October 6, 2026 09:14
f86d684 to
212f846
Compare
vepadulano
requested changes
Oct 6, 2026
Contributor
|
Shouldn't the Session be non-copyable if this footgun exists? |
Test Results 24 files 24 suites 3d 23h 45m 47s ⏱️ For more details on these failures, see this check. Results for commit f22aa69. ♻️ This comment has been updated with latest results. |
guitargeek
force-pushed
the
sofie-rdf-fixup
branch
from
October 6, 2026 22:14
212f846 to
f22aa69
Compare
The RooONNXFunc stored the SOFIE Session in a std::any, which requires copy-constructible types. Since the copy constructor and copy assignment of the SOFIE-emitted Session classes should be deleted, we can't use it anymore. Use a std::shared_ptr<void> instead, which also type-erases the deleter but doesn't require the stored type to be copyable.
Follows up on 0878949. The TMVA_SOFIE_RDataFrame.py tutorial initialized its vector of Sessions with an initializer list, which copies the Session. A generated Session holds raw pointers into its own weight vectors and intermediate memory pool, so the copy still pointed to the buffers of the temporary Session, which were freed right after. The inference then read the weights from freed memory and wrote the intermediate tensors into it. This silently gave wrong results, and on macOS 27 it crashed with SIGTRAP because the system allocator detects the heap corruption. Construct the Session in place with emplace_back instead.
Some data members of the session classes are pointers to memory owned by the session class, so the session can't be trivially copied. Instead of implementing a proper copy constructor and copy assignment, just delete them because they are also not needed.
guitargeek
force-pushed
the
sofie-rdf-fixup
branch
from
October 7, 2026 09:14
f22aa69 to
318b85b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows up on 0878949.
The TMVA_SOFIE_RDataFrame.py tutorial initialized its vector of Sessions with an initializer list, which copies the Session. A generated Session holds raw pointers into its own weight vectors and intermediate memory pool, so the copy still pointed to the buffers of the temporary Session, which were freed right after. The inference then read the weights from freed memory and wrote the intermediate tensors into it.
This silently gave wrong results, and on macOS 27 it crashed with SIGTRAP because the system allocator detects the heap corruption.
Construct the Session in place with emplace_back instead.