-
Notifications
You must be signed in to change notification settings - Fork 29
Do not restrict bbox size for inferral jobs for super users #8200
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
Do not restrict bbox size for inferral jobs for super users #8200
Conversation
WalkthroughThe changes in this pull request involve updates to the WEBKNOSSOS project, focusing on asynchronous image file handling and modifications to bounding box configurations for super users. The changelog highlights performance improvements and bug fixes, including resolving issues with tree deletions and NML file imports. Additionally, user permissions related to bounding box selection have been refined in the frontend code, allowing super users to bypass certain validation checks. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
@normanrz I just assigned you in case you have time. If you do not, feel free to re-assign the review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.unreleased.md (2)
17-17
: Consider spelling and clarity improvementsThe changelog entry could be improved for clarity and consistent spelling:
-Removed bounding box size restriction for inferral jobs for super users. [#8200](https://github.com/scalableminds/webknossos/pull/8200) +Removed bounding box size restriction for inference jobs for superusers. [#8200](https://github.com/scalableminds/webknossos/pull/8200)🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: This is normally spelled as one word.
Context: ... size restriction for inferral jobs for super users. [#8200](https://github.com/scalablemin...(SUPER_COMPOUNDS)
17-17
: Add more context about the change's impactConsider expanding the changelog entry to provide more context about:
- The previous limitation that was removed
- The impact and benefits for superusers
- Any related configuration changes needed
This would help users better understand the significance of this change.
Example:
-Removed bounding box size restriction for inferral jobs for super users. [#8200](https://github.com/scalableminds/webknossos/pull/8200) +Removed bounding box size restriction for inference jobs for superusers, allowing them to process larger datasets without size limitations. Previously enforced size restrictions no longer apply to superuser accounts. [#8200](https://github.com/scalableminds/webknossos/pull/8200)🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: This is normally spelled as one word.
Context: ... size restriction for inferral jobs for super users. [#8200](https://github.com/scalablemin...(SUPER_COMPOUNDS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md
(1 hunks)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
(5 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~17-~17: This is normally spelled as one word.
Context: ... size restriction for inferral jobs for super users. [#8200](https://github.com/scalablemin...
(SUPER_COMPOUNDS)
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (3)
114-114
: LGTM: Type definition updated correctly
The isSuperUser
property has been properly added to the BoundingBoxSelectionProps
type definition.
Line range hint 223-241
: LGTM: Validation bypass for super users implemented correctly
The validation logic has been updated to bypass bounding box size restrictions for super users while maintaining the restrictions for regular users. The implementation:
- Early returns for non-configurable boxes or super users
- Maintains existing validation for regular users
- Provides clear error messages
538-538
: Verify super user state handling
The super user state is correctly retrieved from the Redux store and properly passed down to the BoundingBoxSelectionFormItem
. However, let's verify that the super user state is consistently handled across the application.
Also applies to: 649-649
✅ Verification successful
Let me search for more specific patterns to verify the super user state handling.
Let me search for the actual usage of bounding box validation in the codebase.
Based on the code analysis, I can now provide a final response about the super user state handling in relation to bounding box validation.
Super user state handling is correctly implemented
The super user state handling for bounding box validation is properly implemented with appropriate checks:
- The
isSuperUser
flag is correctly retrieved from the Redux store and passed down to components - Super users are exempted from bounding box size restrictions as seen in the validation logic:
if (!isBoundingBoxConfigurable || isSuperUser) return Promise.resolve();
- The
isBoundingBoxExportable
function is consistently used across the application for validating bounding box sizes - The validation is properly bypassed for super users while still being enforced for regular users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of super user state
# Look for other instances where super user checks might be needed for bounding box validation
# Check for other bounding box validation logic
rg -A 5 "isBoundingBoxExportable|boundingBox.*validation"
# Check for other super user checks that might need similar updates
rg -A 5 "isSuperUser.*boundingBox"
Length of output: 2994
Script:
#!/bin/bash
# Search for all occurrences of isSuperUser to understand its usage patterns
rg -A 5 "isSuperUser"
# Search for bounding box related components and forms
ast-grep --pattern 'const $_ = $$$boundingBox$$$'
Length of output: 143834
Script:
#!/bin/bash
# Search for bounding box validation logic and its relationship with super user checks
rg -A 5 "boundingBox.*validation|isBoundingBoxExportable" frontend/
# Search for bounding box related components and their props
ast-grep --pattern 'function $name($props) { $$$boundingBox$$$ }'
Length of output: 3035
@dieknolle3333 Do you maybe have some time to look at this PR? It is pretty small 🐌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this. Testing went well and code LGTM! 🎉 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
19-19
: Consider enhancing clarity and fixing spelling.Two minor suggestions:
- "super users" should be "superusers" as it's commonly written as one word
- Consider adding more context about the implications, e.g.: "Removed bounding box size restriction for inferral jobs when initiated by superusers, allowing them to process larger datasets."
-Removed bounding box size restriction for inferral jobs for super users. [#8200](https://github.com/scalableminds/webknossos/pull/8200) +Removed bounding box size restriction for inferral jobs when initiated by superusers, allowing them to process larger datasets. [#8200](https://github.com/scalableminds/webknossos/pull/8200)🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: This is normally spelled as one word.
Context: ... size restriction for inferral jobs for super users. [#8200](https://github.com/scalablemin...(SUPER_COMPOUNDS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md
(1 hunks)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~19-~19: This is normally spelled as one word.
Context: ... size restriction for inferral jobs for super users. [#8200](https://github.com/scalablemin...
(SUPER_COMPOUNDS)
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
19-19
: LGTM! The changelog entry accurately reflects the changes.
The entry is appropriately placed in the "Changed" section and includes the correct PR reference.
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: This is normally spelled as one word.
Context: ... size restriction for inferral jobs for super users. [#8200](https://github.com/scalablemin...
(SUPER_COMPOUNDS)
URL of deployed dev instance (used for testing):
Steps to test:
yarn enable-jobs
, inapplication.conf
setexportTiffMaxVolumeMVx = 100
andexportTiffMaxEdgeLengthVx = 100
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
New Features
precomputed://
when pasting remote URIs from Neuroglancer.Bug Fixes
Enhancements