fix dataset group slice is not being used on launch_app #4423
Conversation
WalkthroughThe change involves enhancing the Changes
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 1
Outside diff range and nitpick comments (6)
fiftyone/core/session/session.py (6)
Line range hint
28-28: Replace bareexceptwithexcept Exceptionto avoid catching exceptions likeSystemExitorKeyboardInterrupt.- except: + except Exception:
Line range hint
528-528: Replace bareexceptwithexcept Exceptionto avoid catching exceptions likeSystemExitorKeyboardInterrupt.- except: + except Exception:
Line range hint
1150-1200: Refactor lambda expressions todeffunctions for better readability and debugging.- on_close_session: t.Callable[[CloseSession], None] = lambda _: setattr( - session, "_wait_closed", True - ) + def on_close_session(_: CloseSession) -> None: + setattr(session, "_wait_closed", True)
Line range hint
1235-1235: Consider usingimportlib.util.find_specto check for the availability offiftyone.desktopinstead of importing it directly.- import fiftyone.desktop + import importlib.util + if importlib.util.find_spec("fiftyone.desktop") is not None: + import fiftyone.desktop
Line range hint
1300-1300: Replace bareexceptwithexcept Exceptionto avoid catching exceptions likeSystemExitorKeyboardInterrupt.- except: + except Exception:
Line range hint
1310-1310: Replace bareexceptwithexcept Exceptionto avoid catching exceptions likeSystemExitorKeyboardInterrupt.- except: + except Exception:
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/core/session/session.py (2 hunks)
Additional Context Used
Ruff (15)
fiftyone/core/session/session.py (15)
10-10:
dataclasses.asdictimported but unused
28-28: Do not use bare
except
528-528: Do not use bare
except
1150-1152: Do not assign a
lambdaexpression, use adef
1155-1157: Do not assign a
lambdaexpression, use adef
1160-1162: Do not assign a
lambdaexpression, use adef
1165-1167: Do not assign a
lambdaexpression, use adef
1170-1172: Do not assign a
lambdaexpression, use adef
1175-1177: Do not assign a
lambdaexpression, use adef
1180-1182: Do not assign a
lambdaexpression, use adef
1187-1193: Do not assign a
lambdaexpression, use adef
1196-1200: Do not assign a
lambdaexpression, use adef
1235-1235:
fiftyone.desktopimported but unused; consider usingimportlib.util.find_specto test for availability
1300-1300: Do not use bare
except
1310-1310: Do not use bare
except
Additional comments not posted (1)
fiftyone/core/session/session.py (1)
421-421: Ensure thatgroup_sliceis properly integrated and tested across the application.Verification successful
The search results indicate that
group_sliceis extensively used and tested across various parts of the application, including unit tests and core functionalities. This suggests thatgroup_sliceis well-integrated and likely covered by tests.
- Files with
group_sliceusage:
tests/unittests/view_tests.pytests/unittests/utils3d_tests.pytests/unittests/server_group_tests.pytests/unittests/evaluation_tests.pytests/unittests/dataset_tests.pytests/unittests/group_tests.pyfiftyone/utils/utils3d.pyfiftyone/utils/cvat.pyfiftyone/utils/labelbox.pyfiftyone/utils/groups.pyfiftyone/utils/data/exporters.pyfiftyone/utils/annotations.pyfiftyone/server/aggregations.pyfiftyone/server/query.pyfiftyone/server/view.pyfiftyone/server/mutation.pyfiftyone/server/routes/frames.pyfiftyone/server/events/dispatch.pyfiftyone/migrations/revisions/v0_17_0.pyfiftyone/core/video.pyfiftyone/core/odm/mixins.pyfiftyone/core/stages.pyfiftyone/core/odm/dataset.pyfiftyone/core/view.pyfiftyone/core/state.pyfiftyone/core/session/session.pyfiftyone/core/session/client.pyfiftyone/core/media.pyfiftyone/core/metadata.pyfiftyone/core/dataset.pyfiftyone/core/expressions.pyfiftyone/core/aggregations.pyfiftyone/core/collections.pyGiven the extensive usage and presence in unit tests, it appears that
group_sliceis properly integrated and tested across the application.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `group_slice` is used consistently across the application. # Test: Search for `group_slice` usage in Python files. Expect: Consistent usage across the application. rg --type python $'group_slice'Length of output: 61
Script:
#!/bin/bash # Description: Verify that `group_slice` is used consistently across the application. # Test: Search for `group_slice` usage in Python files. Expect: Consistent usage across the application. rg --type py 'group_slice'Length of output: 44873
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
fiftyone/core/session/session.py (3)
Line range hint
27-27: Replace bareexceptwith specific exceptions.Using bare
exceptstatements can catch unexpected exceptions and obscure real errors. It's better to catch specific exceptions to avoid hiding bugs and to make the error handling clearer.- except: + except ImportError:Also applies to: 527-527, 1299-1299, 1309-1309
Line range hint
1149-1151: Replace lambda expressions with function definitions.Using
deffor function definitions instead of assigning lambda expressions can improve readability and debugging. It's also more conventional in Python to usedeffor complex functions.- on_close_session: t.Callable[[CloseSession], None] = lambda _: setattr( - session, "_wait_closed", True - ) + def on_close_session(_: CloseSession) -> None: + setattr(session, "_wait_closed", True)Also applies to: 1154-1156, 1159-1161, 1164-1166, 1169-1171, 1174-1176, 1179-1181, 1186-1192, 1195-1199
Line range hint
1234-1234: Consider checking for module availability instead of importing directly.Instead of importing
fiftyone.desktopdirectly, which could fail if the module is not installed, useimportlib.util.find_specto check for its availability. This approach prevents runtime errors in environments where the module is not available.- import fiftyone.desktop + import importlib.util + if importlib.util.find_spec("fiftyone.desktop") is not None: + import fiftyone.desktop + else: + raise ImportError("Module 'fiftyone.desktop' is not available")
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/core/session/session.py (2 hunks)
Additional Context Used
Ruff (14)
fiftyone/core/session/session.py (14)
27-27: Do not use bare
except
527-527: Do not use bare
except
1149-1151: Do not assign a
lambdaexpression, use adef
1154-1156: Do not assign a
lambdaexpression, use adef
1159-1161: Do not assign a
lambdaexpression, use adef
1164-1166: Do not assign a
lambdaexpression, use adef
1169-1171: Do not assign a
lambdaexpression, use adef
1174-1176: Do not assign a
lambdaexpression, use adef
1179-1181: Do not assign a
lambdaexpression, use adef
1186-1192: Do not assign a
lambdaexpression, use adef
1195-1199: Do not assign a
lambdaexpression, use adef
1234-1234:
fiftyone.desktopimported but unused; consider usingimportlib.util.find_specto test for availability
1299-1299: Do not use bare
except
1309-1309: Do not use bare
except
Additional comments not posted (1)
fiftyone/core/session/session.py (1)
420-420: Ensure the newgroup_sliceparameter is correctly integrated.The addition of the
group_sliceparameter in theSessionclass constructor correctly implements the functionality described in the PR. This change should ensure that thegroup_slicefrom the dataset is used when the session is initialized, aligning with the PR's objectives.
What changes are proposed in this pull request?
Fixes an issue where
dataset.group_sliceis not being used in the app onlaunch_appHow is this patch tested? If it is not, please explain why.
Using snippet below in py shell and ensuring app is opened with
pcdslice selectedRelease Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Fix launch_app not using
dataset.group_sliceWhat areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit