Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove transaction #2848

Merged
merged 2 commits into from Apr 22, 2024
Merged

Remove transaction #2848

merged 2 commits into from Apr 22, 2024

Conversation

btbest
Copy link
Contributor

@btbest btbest commented Apr 19, 2024

This removes a Transaction mechanism that was implemented in 2019 and then never used.

I am guessing that this was intended to allow us to remove a TransactionSlot that is used in a bunch of ops as a workaround, and judging by #2207, some "disconnect slot X first before we do this other thing so that we don't end up inconsistent in the meantime". Possibly also as an alternative solution to the current SetupDepthContext / call_when_setup_finished queuing mechanism that buffers setupOutputs calls correction: the SetupDepthContext is only used to prevent LayerViewerGui.updateAllLayers from running while the graph is being changed through Slot.connect, disconnect, setValue or setDirty. (That, plus a few notifyDirty with defer=True in countingGui and edgeTrainingGui.)

In the meantime, the additional piping through functions to check if a transaction is going on has been bloating stack traces unnecessarily since then.

We could keep some sort of archive branch with a revert of this commit in case we ever want to seriously implement it...

Current:
Screenshot 2024-04-19 154008

With this PR:
Screenshot 2024-04-19 154046

The lazyflow imports are unused but left here because some scripts seem to import them from `lazyflow.graph` instead of their actual source.
This is unused, but the nesting of calls adds a lot of unnecessary bloat to stack traces.
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.73%. Comparing base (60b582f) to head (4580a98).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2848      +/-   ##
==========================================
- Coverage   55.77%   55.73%   -0.05%     
==========================================
  Files         533      533              
  Lines       62253    62207      -46     
  Branches     8516     8511       -5     
==========================================
- Hits        34721    34668      -53     
- Misses      25777    25785       +8     
+ Partials     1755     1754       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya,

part of me is saddened that this never took off, but implementing transactions on that level made the stack traces indeed more complicated than they should be - something noone considered back then, and I think we just got used to it in the end. Your example is striking. I almost look forward to future exceptions during setup ;).

We need some sort of more natural transaction mechanism, than the once you can currently find in the code, so +1 on your suggestions to "archive" this transaction approach in a branch (maybe with an accompanying issue (could also be a comment in #2207), so it's a bit more findable).

@btbest btbest merged commit 723a458 into ilastik:main Apr 22, 2024
15 of 16 checks passed
@btbest btbest deleted the remove-transaction branch April 22, 2024 12:29
@btbest
Copy link
Contributor Author

btbest commented Apr 22, 2024

The branch with the revert is here: https://github.com/ilastik/ilastik/tree/restore-transaction

Will crosspost on the other issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants