Fix cache deserialization for ops using OpBlockedArrayCache - #3119
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3119 +/- ##
==========================================
- Coverage 63.38% 63.37% -0.02%
==========================================
Files 534 534
Lines 64852 64835 -17
==========================================
- Hits 41105 41086 -19
- Misses 23747 23749 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ab5dfbe to
b2821af
Compare
direct invocation of setInSlot will not work for this OpBlockedArrayCache, but has to go via connected slots __setitem__ to reach setInSlot of the cache.
b2821af to
be94d58
Compare
k-dominik
changed the title
prevent accidental usage of direct setInSlot invocations which led to hard to locate bugs. _setInSlot is only intended to be invoked by Slot class.
There was a problem hiding this comment.
I tried to experience the speedup but didn't manage... just for documentation it would be nice if you can describe in what situation there is a speedup 😅 but lgtm
I did find a small problem (easy fix):
oc-from-segmentation: While trying to reproduce, I was putting mitocheck (txyc) into raw data, and an apparently misordered segmentation (zxyc) as secondary data.
Problem: Instead of the "fix this dataset" dialog to adjust axis order, on this PR I just get a "here's an error" dialog, with no option. It's because OpRegionFeatures.setupOutputs raises Exception, not DatasetConstraintError
opObjectExtraction.py", line 531, in setupOutputs
raise Exception(
Exception: shapes do not match. label volume shape: (1, 1344, 1024, 53, 1). raw data shape: (53, 1344, 1024, 1, 1)
I guess OpRegionFeatures is now higher up in the chain, or previously wasn't ready this early..? If this doesn't immediately make sense to you, maybe worth checking - otherwise it's a quick fix of just replacing the Exception.
| # "Invalid slot for _setInSlot(): {}".format( slot.name ) | ||
| # Nothing to do here. | ||
| # Our Input slots are directly fed into the cache, | ||
| # so all calls to __setitem__ are forwarded automatically |
There was a problem hiding this comment.
| # "Invalid slot for _setInSlot(): {}".format( slot.name ) | |
| # Nothing to do here. | |
| # Our Input slots are directly fed into the cache, | |
| # so all calls to __setitem__ are forwarded automatically |
seems obsolete now :)
There was a problem hiding this comment.
Any operator with a cache deep inside needs to implement _setInSlot(...): pass...
Currently, any op where SomeSlot[slicing] = value is called needs to implement _setInSlot(...): pass) in order for the forwarding to a (possibly) nested cache op can be done - which - idk, I find a bit too much noise (as well as these comments throughout the code base that you highlight). I was still debating though how to safely do so that one could not forget to implement it in the ops that actually need it (like caches)... So for this PR I'm gonna leave it in, but will open an issue.
The speedup is observed when opening any applet that would show the binary image (e.g. classification one). Saving the project with this applet enabled will give you the speedup on load. Also note that the data needs to be large for the speedup to be relevant. Mitocheck has very small images that take no time to do thresholding on. I was testing with a project that had 10k x 14k input images |
Nice find 😅 . I can reproduce this - but also on main. So it's not related to this PR. Will raise the |
Raising a blank exception does not give users a chance to correct, whereas this exception is treated differently, and the user gets the dialog for re- ordering. Co-authored-by: Benedikt Best <[email protected]>
2 participants
.setInSlotshould not be called from outside of slot. Deserialization should always go viaSlot.__setitem__which handlessetItemof the operators.I made
.setInSlotprivate now so that invoking it directly is clearly discouraged.Workflows that were affected by this (check mark indicates confirmed fix):
CarvingnoOpBlockedArrayCacheserialization here.Multicutthis one was actually workingSome timings to show benefit ;)
Checklist
Footnotes
timing measured from start loading to 0 requests - make sure to save in Object Classification or object feature selection applet (binary image needs to be shown) to measure this. ↩ ↩2