Merge release/v1.1.0 to develop
#5178
Conversation
* add slice filter to qp sidebar * cleanup, add slice tests to lightning * rm log
WalkthroughThe changes in this pull request involve enhancements to the Changes
Possibly related PRs
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
fiftyone/server/lightning.py (2)
302-302: LGTM: Consistent filter parameter propagationThe filter parameter is consistently added across all relevant methods. Consider caching the filter construction if it's used frequently in large datasets.
async def lightning_resolver( input: LightningInput, info: Info ) -> t.List[LightningResults]: dataset: fo.Dataset = fo.load_dataset(input.dataset) + filter = ( + {f"{dataset.group_field}.name": input.slice} + if dataset.group_field and input.slice + else None + ) collections, queries, resolvers = zip( *[ _resolve_lightning_path_queries(path, dataset, info) for path in input.paths ] ) counts = [len(a) for a in queries] flattened = [ (collection, item) for collection, sublist in zip(collections, queries) for item in sublist ] - filter = ( - {f"{dataset.group_field}.name": input.slice} - if dataset.group_field and input.slice - else None - ) result = await _do_async_pooled_queries(dataset, flattened, filter)Also applies to: 316-316, 331-333, 366-366
339-342: Enhance error handling for distinct queriesThe current try-except block catches all exceptions. Consider catching specific exceptions and logging the error for debugging.
try: result = await collection.distinct(query.path, filter) - except: + except Exception as e: + import logging + logging.error(f"Failed to execute distinct query: {e}") # too many results return None🧰 Tools
🪛 Ruff (0.7.0)
341-341: Do not use bare
except(E722)
app/schema.graphql (1)
445-445: LGTM! Addition of filters field with architectural considerationThe new
filtersfield using JSON type provides good flexibility for complex filtering options while maintaining backward compatibility.Consider implementing runtime validation for the JSON filters structure to ensure:
- Early detection of malformed filter objects
- Consistent filter schema across different queries
- Protection against injection attacks
This could be achieved through:
- JSON schema validation
- Type-safe filter builders
- Input sanitization middleware
tests/unittests/lightning_tests.py (3)
1056-1139: Consider adding more test cases for edge scenariosWhile the current test cases cover the basic functionality of group dataset queries, consider adding tests for:
- Empty slice names
- Non-existent slice names
- Multiple samples in the same slice
- Samples without group elements
Line range hint
1155-1170: Add docstring to explain theframesandsliceparametersConsider adding a docstring to explain:
- Purpose and impact of the
framesparameter- Format and usage of the
sliceparameter- Default behavior when parameters are omitted
async def _execute( query: str, dataset: fo.Dataset, field: fo.Field, keys: t.Set[str], frames=True, slice: t.Optional[str] = None, ): + """Execute a lightning query on the dataset. + + Args: + query: The GraphQL query string + dataset: The FiftyOne dataset + field: The field type to query + keys: Set of field keys to include + frames: Whether to include frame fields in the query + slice: Optional slice name to filter the dataset + + Returns: + The query execution result + """
1178-1194: Add docstring to explain theframesparameter behaviorThe
_get_pathsfunction would benefit from documentation explaining how theframesparameter affects path generation.def _get_paths( dataset: fo.Dataset, field_type: t.Type[fo.Field], keys: t.Set[str], frames=True, ): + """Get all field paths matching the given type and keys. + + Args: + dataset: The FiftyOne dataset + field_type: The field type to match + keys: Set of field keys to include + frames: Whether to include frame fields in the paths + + Returns: + List of LightningPathInput objects + """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/packages/relay/src/queries/__generated__/lightningQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (5)
app/packages/state/src/recoil/queryPerformance.ts(2 hunks)app/schema.graphql(1 hunks)docs/source/user_guide/app.rst(0 hunks)fiftyone/server/lightning.py(5 hunks)tests/unittests/lightning_tests.py(3 hunks)
💤 Files with no reviewable changes (1)
- docs/source/user_guide/app.rst
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/state/src/recoil/queryPerformance.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (5)
app/packages/state/src/recoil/queryPerformance.ts (2)
14-14: LGTM: Import statement is properly placed and utilized
The import of groupSlice follows TypeScript conventions and is effectively used in the lightningQuery selector.
38-38: LGTM: Verify GraphQL schema compatibility
The addition of slice to the lightningQuery input is clean and follows Recoil best practices.
Let's verify the GraphQL schema compatibility:
✅ Verification successful
✅ GraphQL schema compatibility verified
The slice field is properly defined in the GraphQL schema (app/schema.graphql) as an optional String field with a default value of null, which aligns perfectly with the Recoil selector modification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the GraphQL schema defines the slice field in LightningInput
# and that it's being used consistently across the codebase
# Check the GraphQL schema definition
rg -A 5 "input LightningInput"
# Check for other usages of LightningInput to ensure consistency
ast-grep --pattern 'interface LightningInput {
$$$
slice?: $_
$$$
}'
Length of output: 323
fiftyone/server/lightning.py (2)
48-48: LGTM: Clean addition of slice field
The new optional slice field is well-typed and follows the existing pattern in the class.
141-147: Verify group field access safety
While the filter construction logic is correct, we should verify that accessing dataset.group_field is safe in all scenarios.
Let's verify the group field access pattern:
app/schema.graphql (1)
447-447: LGTM! Addition of nullable slice field
The new slice field is properly defined as nullable with an appropriate type and default value, maintaining backward compatibility.
Let's verify the usage of this new field in the codebase:
✅ Verification successful
Verified: The slice field is properly integrated
The slice field is correctly defined in both the GraphQL schema and the server implementation (fiftyone/server/lightning.py). The field is:
- Properly typed as an optional string in the server code (
t.Optional[str] = None) - Consistently defined as nullable with default value in the GraphQL schema (
String = null) - Used in the
LightningInputtype which maintains backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new slice field in lightning queries
# Look for GraphQL queries using LightningInput to ensure they handle the new field correctly
# Search for GraphQL queries using LightningInput
rg -l "LightningInput.*{" | xargs -I {} rg -A 10 "LightningInput.*{" {}
Length of output: 175
Script:
#!/bin/bash
# Search for GraphQL queries using LightningInput with fixed regex
rg "LightningInput" -A 10
# Also search for any references to 'slice' field in GraphQL operations
rg "slice.*:" -A 5
Length of output: 116140
Script:
#!/bin/bash
# Search for lightning query implementations and usages
ast-grep --pattern 'async def lightning_resolver($_, $_) {
$$$
}'
# Also search for any direct references to the slice field in lightning queries
rg -A 5 'slice.*=.*input\.'
Length of output: 99
Script:
#!/bin/bash
# Search for lightning resolver implementation with Python syntax
ast-grep --pattern 'async def lightning_resolver(input: LightningInput, $_):
$$$'
# Search for files that use the slice field
rg "slice.*=" fiftyone/server/lightning.py
Length of output: 164
* don't inherit dynamic groups setting from grid * fix e2e * dynamic groups paginationd default e2e * fix key conflict
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/state/src/recoil/options.ts (1)
69-93: Consider improving readability of the fallback logicThe implementation is solid, but the nested ternary operator could be made more readable.
Consider restructuring the getter logic like this:
get: (modal: boolean) => ({ get }) => { const value = get(dynamicGroupsViewModeStore(modal)); if (!value) { - return modal - ? get(dynamicGroupsViewModeStore(false)) ?? "pagination" - : "pagination"; + if (modal) { + return get(dynamicGroupsViewModeStore(false)) ?? "pagination"; + } + return "pagination"; } return value; },Also, consider adding a comment explaining the fallback behavior for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/state/src/hooks/useSetModalState.ts(0 hunks)app/packages/state/src/recoil/options.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/state/src/hooks/useSetModalState.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/state/src/recoil/options.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/state/src/recoil/options.ts (2)
7-7: LGTM: Import addition is appropriate
The addition of DefaultValue import is necessary for implementing the reset functionality in the new selector.
58-67: LGTM: Improved naming and type safety
The rename to dynamicGroupsViewModeStore better reflects its role as a storage atom. The addition of null to the type union improves type safety by explicitly handling the default state.
minor fixes embeddings and model evaluation panels CTA
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/packages/operators/src/SplitButton.tsx (2)
132-134: Consider adding type safety to the onClick checkThe cursor styling based on onClick presence is a good UX improvement. However, consider making the check more explicit for better type safety.
sx={{ - cursor: option.onClick ? "pointer" : "default", + cursor: typeof option.onClick === "function" ? "pointer" : "default", }}
151-160: Consider memoizing the Box style objectThe Box implementation for description text is good, but we can optimize it further.
+ const descriptionBoxStyles = { + fontSize: "11px", + "& *": { fontSize: "inherit" }, + }; // Then in the component: secondary={ <Box - sx={{ - fontSize: "11px", - "& *": { fontSize: "inherit" }, - }} + sx={descriptionBoxStyles} > {option.description} </Box> }fiftyone/core/config.py (1)
123-128: LGTM! Consider adding docstring documentation.The implementation of
allow_legacy_orchestratorsfollows the established pattern and is correctly integrated. However, adding documentation would help users understand its purpose and impact.Add a docstring comment above the class definition or in the configuration documentation:
class FiftyOneConfig(EnvConfig): """FiftyOne configuration settings. + Args: + allow_legacy_orchestrators (bool): Whether to allow usage of legacy orchestrators. + Can be set via FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS environment variable. + Defaults to False. """docs/source/plugins/using_plugins.rst (1)
894-901: LGTM! Consider adding a note about persistence.The documentation clearly explains the new requirement for setting the
allow_legacy_orchestratorsflag. However, it might be helpful to mention that users may want to add this to their shell profile (e.g.,.bashrcor.zshrc) for persistence across sessions.Consider adding this note:
export FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS=true + +.. note:: + + Consider adding this environment variable to your shell profile (e.g., `.bashrc` or `.zshrc`) + to persist the setting across terminal sessions.app/packages/operators/src/state.ts (2)
241-242: Good type improvements for better flexibilityThe changes enhance type safety and flexibility by:
- Supporting rich content (React nodes) in descriptions
- Making onClick optional, which is appropriate for disabled states
Consider documenting these type changes in the component's documentation to help other developers understand when to use React nodes vs strings for descriptions.
331-349: Consider extracting string literals to constantsThe UI/UX improvements for handling unavailable orchestrators are good. However, consider extracting the documentation URL and warning messages to named constants at the top of the file. This would make maintenance easier and ensure consistency across the codebase.
+const DELEGATED_EXECUTION_DOC_URL = "https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations"; +const NO_ORCHESTRATOR_WARNING = `This operation requires [delegated execution](${DELEGATED_EXECUTION_DOC_URL})`; +const LEARN_MORE_TEXT = `[Learn how](${DELEGATED_EXECUTION_DOC_URL}) to run this operation in the background`; // Later in the code - "[Learn how](https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations) to run this operation in the background" + LEARN_MORE_TEXT // And - "This operation requires [delegated execution](https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations)"; + NO_ORCHESTRATOR_WARNING;Also applies to: 395-399
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/source/images/plugins/operators/operator-user-delegation.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (7)
app/packages/operators/src/SplitButton.tsx(4 hunks)app/packages/operators/src/state.ts(8 hunks)docs/source/plugins/developing_plugins.rst(1 hunks)docs/source/plugins/using_plugins.rst(1 hunks)docs/source/user_guide/config.rst(3 hunks)fiftyone/core/config.py(1 hunks)fiftyone/operators/executor.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/operators/src/SplitButton.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (11)
app/packages/operators/src/SplitButton.tsx (1)
14-14: LGTM: Box import is correctly added
The Box import is properly placed with other MUI imports and is effectively utilized in the component.
fiftyone/core/config.py (1)
123-128: Verify the impact on existing orchestrator functionality.
The introduction of this feature flag might affect existing orchestrator implementations.
✅ Verification successful
Feature flag safely controls orchestrator registration
The allow_legacy_orchestrators flag is properly implemented as a safety control for orchestrator registration. When set to False (default), it enables orchestrator registration through orchestrator_registration_enabled. This is a controlled transition that:
- Only affects the registration of new orchestrators
- Is documented in the configuration guide
- Has a clear environment variable override (
FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS) - Is referenced appropriately in plugin documentation for users who need legacy behavior
The implementation in executor.py shows this is a clean feature flag that doesn't break existing functionality but rather controls the availability of orchestrator registration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for orchestrator-related code that might be affected
echo "Searching for orchestrator implementations..."
rg -l "orchestrator" --type python
echo "Searching for specific usage of the new configuration..."
rg "allow_legacy_orchestrators" --type python
echo "Searching for environment variable usage..."
rg "FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS" --type python
Length of output: 549
Script:
#!/bin/bash
# Search for orchestrator implementations
echo "Searching for orchestrator implementations..."
rg -l "orchestrator"
echo -e "\nSearching for specific usage of the new configuration..."
rg "allow_legacy_orchestrators"
echo -e "\nSearching for environment variable usage..."
rg "FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS"
Length of output: 1947
Script:
#!/bin/bash
# Let's examine the executor.py implementation to understand the impact
echo "Checking executor.py implementation..."
rg -A 5 -B 5 "allow_legacy_orchestrators" fiftyone/operators/executor.py
echo -e "\nChecking for orchestrator-related code in executor.py..."
rg "orchestrator" fiftyone/operators/executor.py
echo -e "\nChecking delegated.py for orchestrator usage..."
rg "orchestrator" fiftyone/operators/delegated.py
Length of output: 1881
app/packages/operators/src/state.ts (2)
4-10: LGTM! Import organization improvements
The changes improve code organization by explicitly declaring React hook dependencies and adding the Markdown component import.
Also applies to: 41-41
261-261: LGTM! Fixed typo and improved orchestrator availability checks
The changes improve code quality by:
- Fixing the typo in the variable name
- Enhancing orchestrator availability validation
Also applies to: 310-310
fiftyone/operators/executor.py (3)
261-261: LGTM: Proper tracking of delegated operations
The addition of the delegated flag in request_params ensures proper tracking of operation delegation state.
698-703: LGTM: Clear and accurate documentation
The updated property documentation clearly describes the distinction between an operation being delegated and delegation being requested.
1281-1281: Verify the orchestrator registration logic
The inverted logic (not fo.config.allow_legacy_orchestrators) needs verification to ensure it correctly enables/disables orchestrator registration as intended.
Run the following script to verify the configuration usage:
✅ Verification successful
Let me gather more context about the orchestrator registration logic.
Let me do one final check to understand the orchestrator implementation.
Based on the gathered evidence, I can now provide the final response:
The orchestrator registration logic is correctly implemented
The inverted logic not fo.config.allow_legacy_orchestrators is correct because:
- The config option
allow_legacy_orchestratorsdefaults toFalsein core config - Documentation confirms this is intended to control whether operations can be scheduled locally
- When
allow_legacy_orchestratorsis False (default), orchestrator registration is enabled (True) - When
allow_legacy_orchestratorsis True, orchestrator registration is disabled (False)
This aligns with the migration from legacy local orchestration to the new orchestration system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of allow_legacy_orchestrators config
# Test: Search for all usages of allow_legacy_orchestrators
rg -A 3 "allow_legacy_orchestrators"
# Test: Search for orchestrator registration related code
ast-grep --pattern 'orchestrator_registration_enabled'
Length of output: 2803
Script:
#!/bin/bash
# Search for the class containing orchestrator_registration_enabled
ast-grep --pattern 'class $_ {
$$$
orchestrator_registration_enabled($_) {
$$$
}
$$$
}'
# Search for references to orchestrator registration in docs
rg -A 5 "delegated-orchestrator"
Length of output: 6011
Script:
#!/bin/bash
# Search for the implementation of the orchestrator class
rg -B 5 -A 10 "class.*Orchestrator"
# Search for any tests related to orchestrator registration
rg -A 3 "test.*orchestrator.*registration"
Length of output: 1153
docs/source/user_guide/config.rst (2)
113-115: LGTM! Well-documented configuration entry.
The new configuration field is properly documented with all necessary details and follows the established format.
183-183: LGTM! Configuration examples are consistent.
The new field is properly added to both Python and CLI configuration examples, maintaining consistency with the documentation table.
Also applies to: 233-233
docs/source/plugins/developing_plugins.rst (2)
Line range hint 1-995: LGTM! Comprehensive plugin development documentation.
The documentation provides a thorough and well-structured guide for developing FiftyOne plugins, including clear examples and best practices for both Python and JavaScript implementations.
995-996: LGTM! Clear explanation of panel state and data.
The new content about panel state and data management is well-written and provides important information for plugin developers about state persistence and data handling.
| function PrimaryWithTag({ label, tag, disabled }) { | ||
| const theme = useTheme(); | ||
| const tagEl = tag ? ( | ||
| <span | ||
| style={{ | ||
| fontSize: "11px", | ||
| color: theme.custom.primarySoft, | ||
| color: disabled ? theme.text.secondary : theme.custom.primarySoft, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type definitions for PrimaryWithTag props
While the implementation is correct, adding proper TypeScript types would improve maintainability and type safety.
+ interface PrimaryWithTagProps {
+ label: string;
+ tag?: string;
+ disabled?: boolean;
+ }
- function PrimaryWithTag({ label, tag, disabled }) {
+ function PrimaryWithTag({ label, tag, disabled }: PrimaryWithTagProps) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function PrimaryWithTag({ label, tag, disabled }) { | |
| const theme = useTheme(); | |
| const tagEl = tag ? ( | |
| <span | |
| style={{ | |
| fontSize: "11px", | |
| color: theme.custom.primarySoft, | |
| color: disabled ? theme.text.secondary : theme.custom.primarySoft, | |
| interface PrimaryWithTagProps { | |
| label: string; | |
| tag?: string; | |
| disabled?: boolean; | |
| } | |
| function PrimaryWithTag({ label, tag, disabled }: PrimaryWithTagProps) { | |
| const theme = useTheme(); | |
| const tagEl = tag ? ( | |
| <span | |
| style={{ | |
| fontSize: "11px", | |
| color: disabled ? theme.text.secondary : theme.custom.primarySoft, |
improve overlay rendering performance
Fix execute_operator() with delegation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (16)
app/packages/looker/src/worker/decorated-fetch.ts (6)
42-45: Preserve original error using the 'cause' propertyWhen throwing a new error after max retries are exhausted, use the
causeproperty to include the original error. This enhances error tracing and debugging.Apply this diff to include the original error as the cause:
- throw new Error( - "Max retries for fetch reached (linear backoff), error: " + e - ); + throw new Error("Max retries for fetch reached (linear backoff)", { + cause: e, + });
34-34: Type the caught error as 'unknown' for better type safetyIn the
catchblock, typing the error parametereasunknownenhances type safety and aligns with TypeScript best practices.Apply this diff to update the error type:
- } catch (e) { + } catch (e: unknown) {
34-37: Ensure proper handling of unknown error typesAfter typing the error as
unknown, safely check its instance and type before using it to prevent runtime errors.Apply this diff to refine error handling:
} catch (e: unknown) { - if (e instanceof NonRetryableError) { + if (e instanceof NonRetryableError) { // immediately throw throw e; + } else if (e instanceof Error) { + // handle generic errors + if (i < retries - 1) { + await new Promise((resolve) => + setTimeout(resolve, delay * (i + 1)) + ); + } else { + // max retries reached + throw new Error( + "Max retries for fetch reached (linear backoff)", + { cause: e } + ); + } + } else { + // handle non-Error exceptions + throw new Error("An unknown error occurred"); } - if (i < retries - 1) { - await new Promise((resolve) => setTimeout(resolve, delay * (i + 1))); - } else { - // max retries reached - throw new Error( - "Max retries for fetch reached (linear backoff), error: " + e - ); - } }
20-31: Consider handling non-retryable HTTP status codes more thoroughlyThe function currently throws a
NonRetryableErrorfor specific client errors but might benefit from returning the response or including response details in the error message for better debugging and handling at the call site.
39-39: Adjust delay calculation to prevent excessive wait timesThe current delay increases linearly and might result in long wait times on subsequent retries. Consider capping the delay or using exponential backoff for better performance.
1-4: Export constants for reusability and testingExporting
DEFAULT_MAX_RETRIES,DEFAULT_BASE_DELAY, andNON_RETRYABLE_STATUS_CODEScan enhance reusability and facilitate testing.Apply this diff to export the constants:
-export const DEFAULT_MAX_RETRIES = 10; -export const DEFAULT_BASE_DELAY = 200; -const NON_RETRYABLE_STATUS_CODES = [400, 401, 403, 404, 405, 422]; +export const DEFAULT_MAX_RETRIES = 10; +export const DEFAULT_BASE_DELAY = 200; +export const NON_RETRYABLE_STATUS_CODES = [400, 401, 403, 404, 405, 422];app/packages/looker/src/worker/index.ts (1)
164-164: Consider adding more context to error logsCurrently, the error is logged using
console.error(e);. Including additional context, such as the URL being fetched or a descriptive message, can aid in debugging fetch failures more effectively.fiftyone/operators/delegated.py (2)
Line range hint
521-527: Syntax Error: Missingtryblock beforeexceptclauseThe
exceptclause at line 525 is not associated with anytryblock, which will result in a syntax error. In Python,exceptclauses must be preceded by atryblock.Apply this diff to fix the issue:
ctx.request_params["results"] = result + try: outputs = await resolve_type_with_context(operator, ctx) if outputs is not None: outputs_schema = outputs.to_json() + except (AttributeError, Exception): logger.warning( "Failed to resolve output schema for the operation." + traceback.format_exc(), )
Line range hint
525-529: Enhancement: Useexc_infoin logger to capture tracebackConsider using the
exc_info=Trueparameter in the logging call to automatically include the traceback information. This provides better formatting and ensures all exception details are captured.Apply this diff:
except (AttributeError, Exception): - logger.warning( - "Failed to resolve output schema for the operation." - + traceback.format_exc(), - ) + logger.warning( + "Failed to resolve output schema for the operation.", + exc_info=True, + )app/packages/looker/src/worker/decorated-fetch.test.ts (5)
10-19: Consider using more descriptive mock response data.While the test is well-structured, using a more descriptive mock response payload would make debugging easier if the test fails.
- const mockResponse = new Response("Success", { status: 200 }); + const mockResponse = new Response(JSON.stringify({ status: "success", message: "First try succeeded" }), { + status: 200, + headers: { 'Content-Type': 'application/json' } + });
21-32: Consider verifying retry timing.The test could be enhanced by verifying the timing between retries to ensure the linear backoff is working as expected.
+ vi.useFakeTimers(); const response = await fetchWithLinearBackoff("http://fiftyone.ai"); + expect(vi.getTimerCount()).toBe(0); // Verify all timers were executed + vi.useRealTimers();
44-64: Consider adding edge case tests for error responses.The error handling tests are good, but consider adding tests for:
- Edge case status codes (e.g., 418, 429)
- Response with error messages in the body
- Network errors with specific error types
Would you like me to provide example test cases for these scenarios?
66-90: Fix typo and enhance timing assertions.There's a typo in the comment "scond" and the timing assertions could be more explicit.
- // after scond delay + // after second delay await vi.advanceTimersByTimeAsync(200); + // Verify exact timing of retries + expect(vi.getTimerCount()).toBe(0);
1-91: Consider adding JSDoc documentation for the test suite.While the test cases are comprehensive, adding JSDoc documentation would improve maintainability by:
- Describing the purpose of the test suite
- Explaining the retry strategy being tested
- Documenting the expected behavior for different error scenarios
+/** + * Test suite for fetchWithLinearBackoff function. + * Verifies: + * - Successful fetch scenarios + * - Retry behavior with linear backoff + * - Error handling for different HTTP status codes + * - Maximum retry limit enforcement + */ describe("fetchWithLinearBackoff", () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/packages/utilities/src/fetch.ts (2)
21-21: LGTM! Consider adding JSDoc comments.The removal of the
"arrayBuffer"type fromretryCodesimproves type safety as retry codes should only be HTTP status codes.Consider adding JSDoc comments to document the expected HTTP status codes:
+/** + * @param retryCodes - HTTP status codes that should trigger a retry (e.g., [502, 503, 504]) + */ retryCodes?: number[]
113-113: LGTM! Consider making retry delay configurable.The 500ms retry delay is a good default that prevents overwhelming the server and allows transient issues to resolve.
Consider making the retry delay configurable:
- retryDelay: 500, + retryDelay: options?.retryDelay ?? 500,This would allow adjusting the delay based on different scenarios while maintaining the 500ms default.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/packages/looker/src/elements/common/error.ts(1 hunks)app/packages/looker/src/elements/image.ts(2 hunks)app/packages/looker/src/worker/decorated-fetch.test.ts(1 hunks)app/packages/looker/src/worker/decorated-fetch.ts(1 hunks)app/packages/looker/src/worker/index.ts(8 hunks)app/packages/utilities/src/fetch.ts(2 hunks)fiftyone/operators/delegated.py(1 hunks)fiftyone/operators/executor.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/looker/src/elements/common/error.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/elements/image.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/decorated-fetch.test.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/decorated-fetch.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/index.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/utilities/src/fetch.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/decorated-fetch.test.ts
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🪛 Ruff (0.7.0)
fiftyone/operators/executor.py
402-402: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (15)
app/packages/looker/src/worker/index.ts (7)
33-33: Usage of fetchWithLinearBackoff enhances error handling
The introduction of fetchWithLinearBackoff improves the robustness of fetch operations by implementing retries with linear backoff, which enhances error handling during image loading.
111-112: Updated function signature for imputeOverlayFromPath
Adding cls: string and maskPathDecodingPromises: Promise<void>[] = [] as parameters allows passing the class type and accumulating mask path decoding promises for concurrent processing, improving efficiency.
Line range hint 116-131: Efficient accumulation of promises for concurrent execution
By collecting promises in the promises array and then spreading them into maskPathDecodingPromises, the code enables concurrent processing of overlay images, which can improve performance.
159-167: Improved error handling with try-catch and linear backoff
Wrapping the fetch operation in a try-catch block and using fetchWithLinearBackoff ensures that fetch failures are gracefully handled, preventing unhandled exceptions and allowing the function to exit cleanly if an error occurs.
195-197: Initialization of maskPathDecodingPromises for promise management
Introducing maskPathDecodingPromises to collect all decoding promises allows for effective management and ensures that all overlay processing is completed before proceeding.
259-259: Review the use of Promise.allSettled for handling promises
Using Promise.allSettled(maskPathDecodingPromises) waits for all promises to settle, regardless of fulfillment or rejection. Consider checking the results to handle any rejected promises appropriately, ensuring data integrity and proper error handling during mask decoding.
289-289: Consistent return of buffers after processing labels
Returning the buffers after all painter promises have been resolved ensures that label processing is complete, maintaining consistency in the data flow and preventing potential race conditions.
app/packages/looker/src/elements/image.ts (3)
21-26: LGTM! Proper cleanup of retry mechanism
The load event handler correctly cleans up the retry mechanism by clearing the timeout and resetting the retry count.
65-69: LGTM! Proper cleanup on source change
The render method correctly resets the retry mechanism when the source URL changes, preventing any lingering retries from affecting the new image load.
13-13: Consider the implications of exposing imageSource
Changing imageSource from private to protected exposes internal state to subclasses. While this enables inheritance, it might break encapsulation. Please document why this change was necessary and consider if there's a more encapsulated way to achieve the same goal.
✅ Verification successful
Protected access modifier for imageSource is justified
The change from private to protected for imageSource is warranted as the codebase shows a clear inheritance pattern where multiple element types (ThreeDElement, VideoElement, FrameElement, ImaVidElement) need access to this property. Each of these elements handles different types of media sources and requires access to imageSource for rendering purposes. The property is used consistently across these implementations for managing their respective image sources (canvas, video, or image elements).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any subclasses using this protected member
ast-grep --pattern 'class $_ extends ImageElement {
$$$
imageSource
$$$
}'
Length of output: 82
Script:
#!/bin/bash
# First, let's check if there are any files extending ImageElement
rg "extends\s+ImageElement" -l
# Then, let's check for any usage of imageSource property
rg "imageSource" -A 2 -B 2
# Also check the full ImageElement class definition to understand the context
ast-grep --pattern 'class ImageElement {
$$$
}'
Length of output: 5633
app/packages/looker/src/worker/decorated-fetch.test.ts (1)
1-8: LGTM! Clean test setup with proper mock handling.
The test setup follows best practices by clearing mocks and resetting timers before each test run.
app/packages/looker/src/elements/common/error.ts (1)
106-109: LGTM! Good cleanup implementation.
The added cleanup logic properly handles the removal of error elements when there's no error, following best practices by both removing the DOM element and nullifying the reference. This prevents memory leaks and ensures proper UI state management.
fiftyone/operators/executor.py (3)
261-261: LGTM: Delegated execution tracking enhancement
The addition of delegated=True to request params improves operation tracking by explicitly marking delegated executions.
386-401: LGTM: Improved type resolution implementation
The updated function signature and implementation provide better clarity and error handling for resolving operator schemas.
1278-1278: LGTM: Configuration-driven orchestrator registration
The property now correctly uses the configuration setting allow_legacy_orchestrators to determine if orchestrator registration is enabled.
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unreachable code at the end of the function
The return null; statement at line 48 is unreachable because all code paths either return a response or throw an error. Removing it will clean up the code.
Apply this diff to remove the unreachable code:
- return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return null; |
| // sometimes image loading fails because of insufficient resources | ||
| // we'll want to try again in those cases | ||
| if (this.retryCount < MAX_IMAGE_LOAD_RETRIES) { | ||
| // schedule a retry after a delay | ||
| if (this.timeoutId !== null) { | ||
| window.clearTimeout(this.timeoutId); | ||
| } | ||
| this.timeoutId = window.setTimeout(() => { | ||
| this.retryCount += 1; | ||
| const retrySrc = `${this.src}`; | ||
| this.element.setAttribute("src", retrySrc); | ||
| // linear backoff | ||
| }, 1000 * this.retryCount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance retry mechanism implementation
Several improvements could be made to the retry logic:
- The
retrySrcvariable is redundant - Consider exponential backoff instead of linear for better handling of resource issues
- The
timeoutIdshould be cleared when max retries are reached
Consider this implementation:
if (this.retryCount < MAX_IMAGE_LOAD_RETRIES) {
if (this.timeoutId !== null) {
window.clearTimeout(this.timeoutId);
}
this.timeoutId = window.setTimeout(() => {
this.retryCount += 1;
- const retrySrc = `${this.src}`;
- this.element.setAttribute("src", retrySrc);
+ this.element.setAttribute("src", this.src);
// linear backoff
- }, 1000 * this.retryCount);
+ }, 1000 * Math.pow(2, this.retryCount - 1)); // exponential backoff
+} else {
+ this.timeoutId = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // sometimes image loading fails because of insufficient resources | |
| // we'll want to try again in those cases | |
| if (this.retryCount < MAX_IMAGE_LOAD_RETRIES) { | |
| // schedule a retry after a delay | |
| if (this.timeoutId !== null) { | |
| window.clearTimeout(this.timeoutId); | |
| } | |
| this.timeoutId = window.setTimeout(() => { | |
| this.retryCount += 1; | |
| const retrySrc = `${this.src}`; | |
| this.element.setAttribute("src", retrySrc); | |
| // linear backoff | |
| }, 1000 * this.retryCount); | |
| } | |
| // sometimes image loading fails because of insufficient resources | |
| // we'll want to try again in those cases | |
| if (this.retryCount < MAX_IMAGE_LOAD_RETRIES) { | |
| // schedule a retry after a delay | |
| if (this.timeoutId !== null) { | |
| window.clearTimeout(this.timeoutId); | |
| } | |
| this.timeoutId = window.setTimeout(() => { | |
| this.retryCount += 1; | |
| this.element.setAttribute("src", this.src); | |
| // linear backoff | |
| }, 1000 * Math.pow(2, this.retryCount - 1)); // exponential backoff | |
| } else { | |
| this.timeoutId = null; | |
| } |
| it("should throw an error after max retries when fetch fails every time", async () => { | ||
| global.fetch = vi.fn().mockRejectedValue(new Error("Network Error")); | ||
|
|
||
| await expect( | ||
| fetchWithLinearBackoff("http://fiftyone.ai", 3, 10) | ||
| ).rejects.toThrowError(new RegExp("Max retries for fetch reached")); | ||
|
|
||
| expect(global.fetch).toHaveBeenCalledTimes(3); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use regex literal instead of RegExp constructor.
The RegExp constructor is unnecessary here and can be simplified.
- ).rejects.toThrowError(new RegExp("Max retries for fetch reached"));
+ ).rejects.toThrowError(/Max retries for fetch reached/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should throw an error after max retries when fetch fails every time", async () => { | |
| global.fetch = vi.fn().mockRejectedValue(new Error("Network Error")); | |
| await expect( | |
| fetchWithLinearBackoff("http://fiftyone.ai", 3, 10) | |
| ).rejects.toThrowError(new RegExp("Max retries for fetch reached")); | |
| expect(global.fetch).toHaveBeenCalledTimes(3); | |
| }); | |
| it("should throw an error after max retries when fetch fails every time", async () => { | |
| global.fetch = vi.fn().mockRejectedValue(new Error("Network Error")); | |
| await expect( | |
| fetchWithLinearBackoff("http://fiftyone.ai", 3, 10) | |
| ).rejects.toThrowError(/Max retries for fetch reached/); | |
| expect(global.fetch).toHaveBeenCalledTimes(3); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
| except Exception as e: | ||
| return ExecutionResult(error=traceback.format_exc()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused exception variable
The exception variable e is assigned but never used. Consider using _ for unused variables or include the error information in the returned ExecutionResult.
- except Exception as e:
+ except Exception:
return ExecutionResult(error=traceback.format_exc())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| return ExecutionResult(error=traceback.format_exc()) | |
| except Exception: | |
| return ExecutionResult(error=traceback.format_exc()) |
🧰 Tools
🪛 Ruff (0.7.0)
402-402: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
Don't require pipeline updates when setting output schema
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/factory/repos/delegated_operation.py (1)
187-188: Simplify metadata initialization using direct assignmentThe metadata initialization is correct, but can be simplified by using direct property assignment instead of
setattr.- else: - setattr(op, "metadata", {}) + else: + op.metadata = {}🧰 Tools
🪛 Ruff (0.7.0)
188-188: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/factory/repos/delegated_operation.py(1 hunks)tests/unittests/operators/delegated_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
fiftyone/factory/repos/delegated_operation.py
188-188: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (2)
fiftyone/factory/repos/delegated_operation.py (1)
187-188: Verify the impact of metadata initialization
The change to always initialize metadata as an empty dictionary is a good improvement for consistency. Let's verify that all consumers handle this properly.
✅ Verification successful
The previous search showed metadata usage but we need to verify how metadata is initialized and accessed across the codebase to ensure the change is safe. Let's check for initialization patterns and property access patterns.
Metadata initialization change is safe to proceed
The verification shows that the metadata initialization in delegated_operation.py is consistent with the codebase's patterns:
- The only other direct
setattrfor metadata is in the same file with the same pattern - Direct metadata assignments always use either an explicit empty dict
{},None, or a valid metadata object - No code patterns were found that assume metadata could be
None
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct metadata access that might assume None
# Look for patterns that might access metadata without checking existence
rg -g '*.py' 'metadata\.' fiftyone/
# Look for patterns that might check if metadata is None
rg -g '*.py' 'metadata(\s+)?[=!]=(\s+)?None' fiftyone/
Length of output: 20335
Script:
#!/bin/bash
# Check for metadata initialization patterns in the codebase
ast-grep --pattern 'setattr($_, "metadata", $_)'
# Check for direct metadata assignments
ast-grep --pattern '$_.metadata = $_'
# Check for metadata property access patterns
ast-grep --pattern 'if $_.metadata'
Length of output: 1709
🧰 Tools
🪛 Ruff (0.7.0)
188-188: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
tests/unittests/operators/delegated_tests.py (1)
215-215: LGTM: Improved metadata handling consistency
The change from checking None to checking for an empty dictionary {} as the default metadata value is a good improvement. This makes the behavior more predictable and follows Python best practices for handling default dictionary values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/operators/src/built-in-operators.ts (1)
1364-1391: LGTM! Consider adding input validation.The
OpenSampleoperator implementation is clean and well-structured. It properly extends the baseOperatorclass and correctly handles both sample and group IDs.Consider adding validation in the execute method to ensure required parameters are present:
async execute({ hooks, params }: ExecutionContext) { + if (!params.id) { + throw new Error("Sample ID is required"); + } hooks.setExpanded({ id: params.id, group_id: params.group_id, }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/operators/src/built-in-operators.ts(2 hunks)fiftyone/operators/operations.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/built-in-operators.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
fiftyone/operators/operations.py (2)
668-677: LGTM! Well-implemented sample modal opener.
The implementation follows the class's established patterns and is well-documented.
679-681: LGTM! Verify frontend action handlers.
The implementation is clean and follows the class's patterns. Let's verify the corresponding frontend handlers are properly implemented.
✅ Verification successful
Frontend handlers for close_sample and open_sample actions are properly implemented
Based on the search results, the frontend has proper handlers for both actions:
handleSampleId()inapp/packages/app/src/useWriters/onSetSample.tsandapp/packages/app/src/useEvents/useSetSample.tshandle sample modal state- The modal-related code is well integrated across the codebase with proper types (
ModalSample), constants (SAMPLE_MODAL_PLUGIN_NAME), and consistent usage patterns - The implementation follows the application's event handling architecture with dedicated hooks and writers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the frontend has proper handlers for open_sample and close_sample actions
# Search for open_sample and close_sample action handlers in frontend code
echo "Searching for open_sample action handler..."
rg -p "open_sample.*action" --type ts
echo "Searching for close_sample action handler..."
rg -p "close_sample.*action" --type ts
Length of output: 276
Script:
#!/bin/bash
# Let's try a broader search pattern to find any references to these actions in the frontend
echo "Searching for open_sample references..."
rg -p "open_sample" --type ts --type js --type jsx --type tsx -A 3
echo "Searching for close_sample references..."
rg -p "close_sample" --type ts --type js --type jsx --type tsx -A 3
# Also search for sample modal related components
echo "Searching for SampleModal component..."
rg -p "SampleModal" --type ts --type js --type jsx --type tsx -A 3
Length of output: 552
Script:
#!/bin/bash
# Let's try searching without specific file type filters and use fd to understand the frontend structure
echo "Finding frontend files..."
fd -e js -e ts -e tsx
echo "Searching for sample-related code in all files..."
rg -p "sample.*modal" -i
# Look for any event handling patterns related to samples
echo "Searching for sample-related event handlers..."
rg -p "handleSample|onSample|sample.*handler" -i
Length of output: 92020
app/packages/operators/src/built-in-operators.ts (2)
1393-1410: LGTM! Clean and focused implementation.
The CloseSample operator is well-implemented with a clear single responsibility of closing the sample modal.
1463-1464: LGTM! Proper registration of new operators.
The new operators are correctly registered as built-in operators following the established pattern.
* intialize from no sample selected state to optimize performance * cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (1)
95-95: Consider adding loading state for large datasetsWhile the toggle implementation is correct, consider adding a loading state for better UX when handling large datasets that might cause noticeable re-render delays.
+ const [isLoading, setIsLoading] = React.useState(false); const handleExpandCollapseAll = () => { + setIsLoading(true); setCollapsedState((prevState) => { const newState = { ...prevState }; Object.keys(newState).forEach((key) => { newState[key] = allCollapsed; }); return newState; }); setAllCollapsed(!allCollapsed); + setIsLoading(false); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (3)
app/packages/core/src/plugins/SchemaIO/components/TreeSelectionView.tsx (3)
42-42: LGTM: Good defensive programming practice
Early handling of undefined data with an empty array is a clean approach.
85-85: LGTM: Consistent state initialization
The initial state of allCollapsed correctly corresponds with the individual collapsed states.
76-76: Verify if collapsed initial state matches UX requirements
Starting with all nodes collapsed might affect user experience. Please confirm if this aligns with the intended default behavior.
✅ Verification successful
Initial collapsed state appears to be a deliberate design choice
The component is part of a schema-driven tree selection view, and the collapsed initial state is a reasonable default for hierarchical data structures, especially when dealing with potentially large datasets. This pattern:
- Prevents overwhelming users with too much information at once
- Allows users to progressively explore the tree structure
- Is a common UX pattern in tree views across many applications
Since there are no explicit requirements or tests contradicting this behavior, and it follows established UX patterns for tree components, the current implementation appears appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any UX specifications or related tests
rg -g '!node_modules' -g '*.{tsx,ts,spec,test}*' -B2 -A2 'TreeSelectionView.*initial.*state|TreeSelectionView.*collapse'
Length of output: 122
Script:
#!/bin/bash
# Let's try a broader search for the component and its state management
rg -g '!node_modules' -g '*.{tsx,ts}' -B5 -A5 'class TreeSelectionView|function TreeSelectionView'
# Also search for any tests or stories related to this component
fd "TreeSelectionView" -e test.tsx -e spec.tsx -e stories.tsx
Length of output: 1951
Script:
#!/bin/bash
# Let's look for the component's usage and any state initialization patterns
rg -g '!node_modules' -g '*.{tsx,ts}' -B3 -A3 '<TreeSelectionView'
# Also search for any documentation about the component's behavior
fd "TreeSelectionView" -e md -e mdx
# Look for the collapsedState type definition and usage
rg -g '!node_modules' -g '*.{tsx,ts}' 'type.*CollapsedState|interface.*CollapsedState'
Length of output: 292
| @@ -183,7 +178,7 @@ export default function TreeSelectionView(props: ViewPropsType) { | |||
| const isSample = | |||
| !structure.some(([parentId]) => parentId === key) && | |||
| key !== "selectAll"; | |||
| return isSample && updatedState[key].checked; // Only checked samples | |||
| return isSample && updatedState[key].checked; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize sample filtering performance
The current implementation repeatedly checks the structure array for each key. Consider pre-computing parent IDs for better performance.
+ const parentIds = React.useMemo(() => new Set(structure.map(([parentId]) => parentId)), [structure]);
const selectedSampleIds = Object.keys(updatedState).filter((key) => {
- const isSample = !structure.some(([parentId]) => parentId === key) && key !== "selectAll";
+ const isSample = !parentIds.has(key) && key !== "selectAll";
return isSample && updatedState[key].checked;
});Committable suggestion skipped: line range outside the PR's diff.
More helpful error when awaiting embeddings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/server/routes/embeddings.py (1)
58-67: Consider consolidating error handling.The method has two similar error handling blocks for brain results. Consider consolidating them for better maintainability:
try: results = dataset.load_brain_results(brain_key) + if results is None: + raise ValueError("Results are not yet available") except Exception as e: + error_type = "not yet available" if isinstance(e, ValueError) else "failed to load" msg = ( - "Failed to load results for brain run with key '%s'. Try " - "regenerating the results" + "Results for brain run with key '%s' are %s" + % (brain_key, error_type) ) % brain_key + if not isinstance(e, ValueError): + msg += ". Try regenerating the results" return {"error": msg} - if results is None: - msg = ( - "Results for brain run with key '%s' are not yet available" - % brain_key - ) - return {"error": msg}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx(1 hunks)fiftyone/server/routes/embeddings.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx
🔇 Additional comments (2)
fiftyone/server/routes/embeddings.py (2)
61-67: LGTM! Improved error handling with clearer user feedback.
The change from assertion to explicit error handling is a good improvement because it:
- Provides a more user-friendly error message
- Maintains consistent error handling pattern with the try-except block above
- Prevents potential server crashes from assertion errors
58-67: Verify input sanitization for brain key.
While the brain key appears to be used safely, it's good practice to verify that it's properly sanitized before use in database queries and error messages.
✅ Verification successful
Let me gather more information about the input validation.
Input validation for brain key is handled securely
The brain key is handled securely through FiftyOne's core implementation:
- It's validated through
has_brain_run()checks before use - Access is mediated through the FiftyOne SDK's
BrainMethodclass - Keys are stored and retrieved through a proper database abstraction layer
- No direct SQL queries or command execution is involved
- Error messages use standard string formatting without exposing sensitive information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for input validation of brain key in the codebase
# Look for brain key validation patterns
rg -A 5 "load_brain_results|get_brain_info" --type py
# Check for any SQL injection prevention patterns
ast-grep --pattern 'load_brain_results($brain_key)'
Length of output: 6662
Script:
#!/bin/bash
# Check for brain key validation in the core implementation
rg -A 10 "def get_brain_info|def load_brain_results" fiftyone/core/collections.py
# Look for any input validation or sanitization patterns
ast-grep --pattern 'if not isinstance($brain_key, str)'
# Check for any key validation before use
rg -B 2 -A 3 "brain_key.*=|brain_key.*in" --type py
Length of output: 19887
| self.allow_legacy_orchestrators = self.parse_bool( | ||
| d, | ||
| "allow_legacy_orchestrators", | ||
| env_var="FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS", | ||
| default=False, | ||
| ) |
There was a problem hiding this comment.
Why in the world would False be the default?
|
In this release, I'm picking up a friendly warning about directional changes based on constant name alone.... 🤔 Looks like the teams product is expanding and starting to feature gate via cloud offering? Convinced myself i must have regressed something if all internal and public plugins just silently dismissed when scheduled, and unrelated to a minor version bump around the same time. |
Merge
release/v1.1.0todevelopSummary by CodeRabbit
Release Notes
New Features
sliceinLightningInputandfiltersinExtendedViewFormfor enhanced data querying.OpenSampleandCloseSampleoperators for improved sample modal control.fetchWithLinearBackofffor robust HTTP requests with retry logic.Documentation
Bug Fixes
Style