Fix stuck saving when more than ~1500 buckets were labeled at once #8409
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new changelog entry documenting a bug fix for an unresponsive saving process. It corrects a spelling error in a comment and updates various generator functions’ return types from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/model/actions/save_actions.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/save_reducer.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/annotation_saga.tsx(1 hunks)frontend/javascripts/oxalis/model/sagas/annotation_tool_saga.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/mapping_saga.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/quick_select_ml_saga.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/root_saga.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/save_saga.ts(6 hunks)frontend/javascripts/oxalis/throttled_store.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/oxalis/model/actions/save_actions.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (16)
CHANGELOG.unreleased.md (1)
21-21: Good addition to document the bug fix.The changelog entry accurately describes the issue that was fixed in this PR, providing users with information about the resolved problem.
frontend/javascripts/oxalis/throttled_store.ts (1)
21-21: Improved type safety with explicit Promise return type.This change correctly annotates the
gofunction withPromise<never>return type, which is appropriate for a function containing an infinite loop. The function is indeed designed to run forever without resolving.frontend/javascripts/oxalis/model/reducers/save_reducer.ts (1)
160-160: Improved state update pattern.Changing from
updateKey2toupdateKeyis a good refactoring that maintains the same functionality but uses a more consistent state update pattern. This aligns with the PR's goal of optimizing state updates related to saving.frontend/javascripts/oxalis/model/sagas/mapping_saga.ts (1)
228-228: Improved type safety with explicit Saga return type.This change correctly annotates the
watchChangedBucketsForLayerfunction withSaga<never>return type, which is appropriate for a saga containing an infinite loop. This is consistent with similar changes in other saga files mentioned in the PR summary.frontend/javascripts/oxalis/model/sagas/annotation_saga.tsx (1)
153-153: Improved type safety withSaga<never>return type.The return type change from
Saga<void>toSaga<never>accurately reflects that this function runs in an infinite loop and never completes normally. This is a more precise type annotation that aligns with the actual behavior of the saga.frontend/javascripts/oxalis/model/sagas/root_saga.ts (1)
26-26: Correct return type for infinite loop saga.Changing the return type to
Saga<never>properly indicates that this saga runs indefinitely in itswhile(true)loop and is not meant to complete normally. This is a good type safety improvement.frontend/javascripts/oxalis/model/sagas/quick_select_ml_saga.ts (1)
133-136: Accurate return type for continuous progress indicator.Adding the
Saga<never>return type correctly reflects that this progress indicator runs in an infinite loop and requires explicit cancellation (which happens in thefinallyblock of the caller at line 304). This type annotation is more precise than the previousSaga<void>.frontend/javascripts/oxalis/model/sagas/annotation_tool_saga.ts (2)
16-16: Return type accurately reflects infinite saga.The
Saga<never>return type correctly indicates that this function runs in a continuous loop and won't complete normally, which matches its implementation.
41-41: Appropriate return type for infinite tool reset watcher.The
Saga<never>return type properly reflects that this function continuously watches for tool reset events in an infinite loop, which is consistent with its implementation.frontend/javascripts/oxalis/model/sagas/mesh_saga.ts (2)
1202-1202: Return type updated from Saga to SagaThis is a good change. Since this saga function runs in an infinite loop with
while(true), it will never actually return. UsingSaga<never>instead ofSaga<void>properly reflects this behavior and is more type-safe.
1215-1215: Fixed error handling to continue instead of throwingThis change improves the saga's resilience. Rather than throwing an error when an unexpected action type is encountered (even though the saga already filtered for ["SET_ADDITIONAL_COORDINATES"]), the saga will now continue listening for the next action.
frontend/javascripts/oxalis/model/sagas/save_saga.ts (5)
57-57: Added memoizeOne import for performance optimizationGood addition for performance optimization, especially since this will be used to optimize the bucket processing function.
61-61: Return type updated from Saga to SagaAppropriate type change. Since this saga runs in an infinite loop with
while(true), it will never actually return. UsingSaga<never>instead ofSaga<void>properly reflects this behavior and is more type-safe.
292-292: Using the memoized functionProperly using the memoized function to get layer and magInfo for the tracing ID, replacing the previous direct calculation with cached results when possible.
383-383: Return type updated from Saga to SagaCorrect type change. This saga also runs in an infinite loop and will never return normally.
456-456: Return type updated from Saga to SagaAppropriate type change for this infinite loop saga, consistent with similar changes in other functions.
| const meshRecords = segmentMeshController.meshesGroupsPerSegmentId; | ||
|
|
||
| if (action.values == null || action.values.length === 0) break; | ||
| if (action.values == null || action.values.length === 0) continue; |
There was a problem hiding this comment.
Changed break to continue when values are empty
This is a crucial fix that likely addresses the core issue. Previously, the saga would exit with break when action.values was null or empty, which would terminate the saga permanently. Changing to continue ensures the saga keeps listening for subsequent actions, maintaining its long-running nature.
| function* markBucketsAsNotDirty(saveQueue: Array<SaveQueueEntry>) { | ||
| const getLayerAndMagInfoForTracingId = memoizeOne((tracingId: string) => { | ||
| const segmentationLayer = Model.getSegmentationTracingLayer(tracingId); | ||
| const segmentationMagInfo = getMagInfo(segmentationLayer.mags); | ||
| return [segmentationLayer, segmentationMagInfo] as const; | ||
| }); |
There was a problem hiding this comment.
Added memoization to optimize bucket processing
Great performance optimization! This directly addresses the PR objective by reducing redundant calculations in the markBucketsAsNotDirty function, which was causing the application to become unresponsive when processing a large number of buckets at once.
The memoization ensures that for a given tracingId, the layer and magInfo are only computed once, which can significantly improve performance when processing thousands of buckets.
| continue; | ||
| } | ||
| const meshRecords = segmentMeshController.meshesGroupsPerSegmentId; | ||
|
|
||
| if (action.values == null || action.values.length === 0) break; | ||
| if (action.values == null || action.values.length === 0) continue; |
There was a problem hiding this comment.
Is this ok to change? The previously thrown error is now swallowed
There was a problem hiding this comment.
yes, the handling is only done for TS, as far as I understand. I can output a warning to console, though.
| const segmentationLayer = Model.getSegmentationTracingLayer(tracingId); | ||
| const segmentationMagInfo = yield* call(getMagInfo, segmentationLayer.mags); | ||
| const [segmentationLayer, segmentationMagInfo] = getLayerAndMagInfoForTracingId(tracingId); |
There was a problem hiding this comment.
This is the fix, correct?
Strange that non-nested yield calls can lead to a stack overflow :/
There was a problem hiding this comment.
yes, apparently, the effects are processed tail recursively by redux which leads to this problem.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/model/sagas/save_saga.ts (1)
104-104: Fixed comment typoSimple spelling fix in the comments for better readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/model/sagas/mesh_saga.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/save_saga.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/sagas/mesh_saga.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (6)
frontend/javascripts/oxalis/model/sagas/save_saga.ts (6)
10-10: Adding memoization dependency for performance optimizationGood addition. This new dependency will be used to optimize the bucket processing function, which is key to fixing the performance issue mentioned in the PR objectives.
61-61: Improved type safety withSaga<never>return typeThis return type more accurately reflects that this saga runs infinitely and doesn't normally return. This is a good practice for long-running sagas that helps prevent accidental early termination.
282-287: Added memoization to optimize bucket processingGreat performance optimization! This directly addresses the PR objective by reducing redundant calculations in the
markBucketsAsNotDirtyfunction, which was causing the application to become unresponsive when processing a large number of buckets at once.The memoization ensures that for a given
tracingId, the layer and magInfo are only computed once, which can significantly improve performance when processing thousands of buckets.
292-292: Using memoized function to retrieve layer and magInfoGood implementation. This line applies the memoized function created above, ensuring that repeated calls with the same
tracingIdreuse the cached result rather than recalculating.
383-383: Updated return type for consistency and accuracySimilar to the change for
pushSaveQueueAsync, this provides more accurate typing for a saga that runs continuously.
456-456: Added missing return type annotationThe consistent application of
Saga<never>here maintains type safety across all infinite-running sagas.
…8409) * tmp: try to debug stuck saving * tmp: attempts to better guard against silent stack overflow * clean up * fix stuck saving * further clean up * clean up * update changelog * format * add console.error in case of ts error
* Create Release 25.02.0 * Fix stuck saving when more than ~1500 buckets were labeled at once (#8409) * tmp: try to debug stuck saving * tmp: attempts to better guard against silent stack overflow * clean up * fix stuck saving * further clean up * clean up * update changelog * format * add console.error in case of ts error * Release 25.02.1 --------- Co-authored-by: Florian M <florian@scm.io>
2 participants
See redux-saga/redux-saga#1592 for the underlying issue. Avoiding high amounts of
yieldwithinmarkBucketsAsNotDirtyshould fix the stuck saving issue. Ideally, we would also add measures so that the same problem cannot happen silently again, but I haven't found a way for this (yet).URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)