[FIX] Only deepcopy the .attributes for the outermost Table transformation#6849
Merged
JakaKokosar merged 5 commits intobiolab:masterfrom Jul 26, 2024
Merged
[FIX] Only deepcopy the .attributes for the outermost Table transformation#6849JakaKokosar merged 5 commits intobiolab:masterfrom
JakaKokosar merged 5 commits intobiolab:masterfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6849 +/- ##
=======================================
Coverage 88.26% 88.26%
=======================================
Files 326 326
Lines 71146 71151 +5
=======================================
+ Hits 62796 62801 +5
Misses 8350 8350 |
ada371e to
4460fbb
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
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.
Issue
#6189 introduced deep-copying of the dictionary
Table.attributesat table transformations. Because table transformations often happen withing table transformation, this can lead to wasting computation and memory. An example of the bug that can happen is Quasars/orange-spectroscopy/pull/726.For illustration, here is a small example:
The memory use/time graphs below (first master, then this branch) show big differences (~30 seconds vs 1 second) that all stem from the wasteful use of deepcopy:
Description of changes
The code now only copies
.attributesonly for the outermost transformations.And the thing that should be discussed.This PRThe previous version of the PR would introduce problems if deeper internal transformations wanted to readtable.attributes. I intentionally left it so just to see if there are any problems. I could add support for them by just referencing that dict for non internal transformations, but if any were naughty and actually modifiedtable.attributes, we'd have problems.I discussed above with myself and noticed that there are no mechanisms for compute_values to set output
table.attributesand that even before this PR, if any compute_value was particularly naughty, there were problems. The following test fails on master.The last commit therefore ensures that
.attributesare always set, but yes, as before, they should not be modified within thecompute_value.Includes