Release/v1.9.0 #6436
Release/v1.9.0 #6436
Conversation
- Change "Apache-2.0" to "Apache 2.0" for consistency - Affected models: rtdetr-v2-s-coco-torch, rtdetr-v2-m-coco-torch - Aligns with format used by other Apache-licensed models in the dataset
Update TensorFlow model descriptions: complete missing entries and standardize format * Add descriptions for all TensorFlow models in model zoo (manifest-tf.json) * Ensure all TF1 and TF2 models have clear, user-friendly descriptions
Changed AI-designed -> to NAS-designed to avoid fluff
cleaned up efficientdent descriptions as well as mentioned task in missing cases
-detector -> object detector -misc
Move the search bar to the top navbar
Fix Algolia search on mobile
Added integration with dinov3, embeddings, classification and pca
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [form-data](https://github.com/form-data/form-data) from 4.0.0 to 4.0.4. - [Release notes](https://github.com/form-data/form-data/releases) - [Changelog](https://github.com/form-data/form-data/blob/master/CHANGELOG.md) - [Commits](form-data/form-data@v4.0.0...v4.0.4) --- updated-dependencies: - dependency-name: form-data dependency-version: 4.0.4 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Optimize and harden `merge_samples()` on video collections
…ngs sub-functions, and remove unused utility function (#6391)
FOEPD-2119: Revert #6288 - improve consistency of apply_model and compute_embeddings sub-functions, and remove unused utility function
check that audience matches [FOEPD-2266]
force brace-expansion version resolution to address CVE-2025-5889
WalkthroughUpdated CI action versions and PyPI publish flow; added extensive docs and notebooks; UI/SchemaIO enhancements (dashboard PNG export, clipboard/multi-select); added operators pipeline types, browser download operator, and monitored multiprocessing delegated execution; core refactors (dataset merge, metadata, indexes); new server sample PATCH endpoint and 3D utilities; installer script replaced. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as CLI / Caller
participant Svc as DelegatedOperationService
participant Repo as DelegatedOperationRepo
participant Child as Child Process
participant LogQ as Log Queue
participant Monitor as Monitor Loop
Caller->>Svc: execute_operation(op, monitor=True, check_interval_seconds)
Svc->>Repo: queue_operation(..., pipeline=opt)
Svc->>LogQ: create log queue
Svc->>Child: spawn child process (mp_ctx)
Child->>Child: _configure_child_logging(LogQ)
Child->>Svc: _execute_operator_in_child_process(op_id)
Child->>LogQ: emit logs
Svc->>Monitor: start _monitor_operation(op_id)
Monitor->>Repo: ping(op_id)
Monitor->>Child: check process alive/status
alt completes normally
Child->>Svc: write completion result
Monitor->>Svc: report success ExecutionResult
else failure or timeout
Monitor->>Child: terminate (psutil kill descendants)
Monitor->>Svc: report failure ExecutionResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/packages/looker/src/overlays/detection.ts (1)
309-309: Add defensive check for emptyconvexHullarray.Line 309 accesses
convexHull[0][0]without verifying that the array is non-empty. While the conditional at line 136 checks thatconvexHullis defined, an empty array is truthy and would pass that check, causing a runtime error here.Apply this diff to add a defensive check:
private fillRectFor3d( ctx: CanvasRenderingContext2D, state: Readonly<State>, color: string ) { const convexHull = this.label.convexHull; + + // defensive check: ensure convexHull has at least one point + if (!convexHull || convexHull.length === 0) { + return; + } const previousAlpha = ctx.globalAlpha; // use double stoke width to make the box more visible ctx.lineWidth = state.strokeWidth * 2;Alternatively, update the conditional at line 136 to be more defensive:
- if (this.label.convexHull) { + if (this.label.convexHull && this.label.convexHull.length > 0) { // only fill 3d when 'convexHull' is defined this.fillRectFor3d(ctx, state, strokeColor);fiftyone/core/metadata.py (1)
370-372: Critical bug: AttributeError when metadata computation fails.When
skip_failures=Trueand metadata computation fails for an asset,_compute_sample_metadatareturnsNone(line 673). At line 372, attemptingmetadata.size_byteswill raise anAttributeErrorbecausemetadataisNone.Apply this diff to handle None metadata:
for task, result in zip(tasks, results): metadata = result[1] - asset_size += metadata.size_bytes - - if cache is not None: - scene_path = task[1] - cache[scene_path] = metadata + if metadata is not None: + asset_size += metadata.size_bytes + + if cache is not None: + scene_path = task[1] + cache[scene_path] = metadataapp/packages/app/src/routing/RouterContext.ts (1)
83-127: Critical: Resource cleanup bug with attach mechanism.When
load()creates a new resource and passes it asattachtoupdate(), a resource lifecycle bug occurs:
- Line 155 (in load): New resource R is created and assigned to
currentEntryResource- Line 162:
update()is called with R asattach- Line 88 (in update):
currentEntryResource.load()loads R, capturing its cleanup callback- Line 90:
nextCurrentEntryResource = attachassigns R (same resource)- Line 113:
loadingResource.load()loads R again (returns cached entry)- Line 122:
currentEntryResource = loadingResource(no-op since both are R)- Line 123:
cleanup()is called, unsubscribing R's active subscriptionImpact: The newly created resource has its subscription immediately unsubscribed, breaking the resource's data flow and potentially causing missing updates, stale data, or errors.
Apply this diff to fix the cleanup logic:
const update = ( location: FiftyOneLocation, action?: Action, attach?: Resource<Entry<T>> ) => { currentEntryResource.load().then(({ cleanup }) => { try { nextCurrentEntryResource = attach ?? getEntryResource({ environment, routes, location: location as FiftyOneLocation, hard: location.state.event !== "modal", handleError, }); } catch (e) { if (e instanceof Resource) { // skip the page change if a resource is thrown return; } throw e; } requestAnimationFrame(() => { for (const [_, [__, onPending]] of subscribers) onPending?.(); }); const loadingResource = nextCurrentEntryResource; loadingResource.load().then((entry) => { nextCurrentEntryResource === loadingResource && requestAnimationFrame(() => { let current: Entry<T> | undefined = undefined; try { current = currentEntryResource.read(); } catch {} for (const [_, [cb]] of subscribers) cb(entry, action, current); // update currentEntryResource after calling subscribers + const shouldCleanup = !attach || currentEntryResource !== attach; currentEntryResource = loadingResource; - cleanup(); + if (shouldCleanup) { + cleanup(); + } }); }); }); };README.md (1)
145-169: Update remaining source-install instructions toinstall.shThe new flow replaces
install.bash, but the upgrade/developer sections still tell users to runbash install.bash*. That’s now wrong and will fail. Please swap those references to./install.sh …(and adjust flags as needed) so the doc matches the new script.Example fix:
-# Mac or Linux -bash install.bash +# Mac or Linux +./install.sh @@ -# Mac or Linux -bash install.bash -d +# Mac or Linux +./install.sh -dfiftyone/factory/repos/delegated_operation_doc.py (1)
98-135: Restorepipeline_indexwhen hydrating documentsWe now persist
pipeline_index, butfrom_pymongonever reads it back, so any non-zero value stored in MongoDB is discarded and every hydrated document falls back to the default0. This breaks any workflow that relies on resuming a staged pipeline from its persisted index.Please load the value from the document, e.g.:
# grouped fields self.parent_id = doc.get("parent_id", None) + self.pipeline_index = doc.get("pipeline_index", self.pipeline_index)
🧹 Nitpick comments (27)
app/packages/looker/src/elements/common/tags.ts (1)
275-292: Consider a more user-friendly default or align with TEMPORAL_DETECTION_RENDERER's approach.The change defaults missing classification labels to the string "null", which may not provide the best user experience. Additionally, there's an inconsistency in this file:
TEMPORAL_DETECTION_RENDERER(lines 294-320) returnsnull(skips rendering) when the label is missing, whileCLASSIFICATION_RENDERERnow displays "null" as text.Consider one of these approaches:
- Use a more descriptive default like
"Unlabeled"or"(no label)"instead of"null"- Align with
TEMPORAL_DETECTION_RENDERERby returningnullto skip rendering whenparam.labelis falsy- If showing missing labels is intentional, document this behavior
Example for approach 1:
- const label = param.label ?? "null"; + const label = param.label ?? "Unlabeled";Example for approach 2 (align with TEMPORAL_DETECTION_RENDERER):
const CLASSIFICATION_RENDERER = (path, param: Classification) => { - const label = param.label ?? "null"; + if (!param.label) { + return null; + } return { path, - value: label, - title: `${path}: ${label}`, + value: param.label, + title: `${path}: ${param.label}`, color: getAssignedColor({ coloring, path, param, isTagged: shouldShowLabelTag(selectedLabelTags, param.tags), labelTagColors, customizeColorSetting, isValidColor, }), }; };app/packages/looker/src/overlays/classifications.ts (1)
397-403: Align the default value with the resolution in tags.ts.The change to default missing labels to
"null"is consistent with the similar change intags.ts. However, as noted in the review of that file, displaying "null" as a literal string may not provide the best user experience.Ensure that whatever approach is chosen for
tags.ts(whether using "Unlabeled", skipping rendering, or keeping "null") is also applied consistently here.If choosing a more descriptive default:
- return label.label ?? "null"; + return label.label ?? "Unlabeled";app/packages/looker-3d/src/hooks/use-fo-loaders.ts (1)
23-24: Extract magic string to a named constant.The sessionStorage key
"customCredentialsAudience"is hardcoded as a string literal. For better maintainability and to prevent typos, extract it as a module-level constant.For example:
const CUSTOM_CREDENTIALS_AUDIENCE_KEY = "customCredentialsAudience";Then reference it in sessionStorage:
- const customCredentialsAudience think = sessionStorage.getItem( - "customCredentialsAudience" - ); + const customCredentialsAudience = sessionStorage.getItem( + CUSTOM_CREDENTIALS_AUDIENCE_KEY + );As per coding guidelines.
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx (2)
61-64: Consider stronger typing for GraphQL error objects.The
e: anyparameter weakens type safety. If the GraphQL error structure is known, consider defining an interface or using a more specific type.- messages = error.errors.map((e: any) => ({ + messages = error.errors.map((e: { message: string; extensions: { stack: string[] } }) => ({ message: e.message, content: "\n\n" + e.extensions.stack.join("\n"), }));Note: Only apply if the GraphQL error structure is stable and well-defined.
132-155: Consider simplifying the component factory pattern.The pattern of returning a function that returns a component (
ErrorsDisplayWithSideEffects(onReset, disableReset)) is less idiomatic. A straightforward component with props would be clearer and more maintainable.-const ErrorsDisplayWithSideEffects = ( - onReset?: () => void, - disableReset?: boolean -) => { - const FallbackComponent = <T extends AppError>({ +const ErrorsDisplayWithSideEffects = <T extends AppError>({ + error, + resetErrorBoundary, + onReset, + disableReset, +}: Props<T> & { onReset?: () => void; disableReset?: boolean }) => { + const clearModal = useClearModal(); + useLayoutEffect(() => { + clearModal(); + }, []); + + return ( + <ErrorDisplayMarkup - error, - resetErrorBoundary, - }: Props<T>) => { - const clearModal = useClearModal(); - useLayoutEffect(() => { - clearModal(); - }, []); - - return ( - <ErrorDisplayMarkup - error={error} - onReset={onReset} - disableReset={disableReset} - resetErrorBoundary={resetErrorBoundary} - /> - ); - }; - return FallbackComponent; + error={error} + onReset={onReset} + disableReset={disableReset} + resetErrorBoundary={resetErrorBoundary} + /> + ); };Then update the usage at line 165:
const ActualFallback = - Fallback || ErrorsDisplayWithSideEffects(onReset, disableReset); + Fallback || ((props: Props<any>) => <ErrorsDisplayWithSideEffects {...props} onReset={onReset} disableReset={disableReset} />);app/packages/operators/src/types-internal.ts (1)
21-21: Consider usingunknowninstead ofanyfor better type safety.The
ParamsTypeis defined asRecord<string, any>, which disables TypeScript's type checking. Consider usingRecord<string, unknown>to maintain type safety while allowing flexibility. Consumers would then need to perform type narrowing before using the values, which catches errors at compile time.Apply this diff:
-export type ParamsType = Record<string, any>; +export type ParamsType = Record<string, unknown>;app/packages/operators/src/usePanelEvent.ts (1)
177-200: Consider removing redundantpanelIdin options object.The
useTriggerPanelEventhook passespanelIdboth as a parameter tohandleEvent(line 188) and as a field in the options object (line 193). SincehandleEventreceivespanelIdas its first parameter through theusePanelStateByIdCallbackwrapper, including it in the options object appears redundant.Apply this diff if the
panelIdfield in options is not used elsewhere:const triggerEvent = useCallback( ( event: string, params?: ParamsType, prompt?: boolean, callback?: ExecutionCallback ) => { handleEvent(panelId, { operator: event, params, prompt, callback, - panelId, }); }, [handleEvent, panelId] );app/packages/operators/src/built-in-operators.ts (1)
1529-1536: Consider adding URL validation and TypeScript typing.The current implementation only checks if the URL exists but doesn't validate its format or protocol. Additionally, the
paramsobject lacks TypeScript typing, which could lead to runtime errors.Consider applying these improvements:
- async execute({ params }: ExecutionContext) { - const url = params.url; - const filename = params.filename; - - if (!url) { - throw new Error("URL is required"); - } + async execute({ params }: ExecutionContext) { + const url = params.url as string; + const filename = params.filename as string | undefined; + + if (!url) { + throw new Error("URL is required"); + } + + // Validate URL format and protocol + try { + const urlObj = new URL(url); + if (!['http:', 'https:'].includes(urlObj.protocol)) { + throw new Error("Only HTTP and HTTPS URLs are supported"); + } + } catch (e) { + throw new Error(`Invalid URL: ${url}`); + }fiftyone/core/odm/database.py (1)
241-254: Consider the fragility of accessing private attributes.The function relies on the private
_closedattribute from pymongo and motor internals, which could change in future versions. Whilegetattrprovides safe fallback behavior, consider documenting this dependency or monitoring for breaking changes in pymongo/motor updates.fiftyone/core/metadata.py (2)
300-310: Parameters are unused in this code path.The
skip_failuresandwarn_failuresparameters are added to the signature but never used since the method immediately raisesValueError. While not technically incorrect, this is inconsistent with the other methods.Consider either:
- Removing these parameters if URLs will never be supported, or
- Adding a docstring note that these parameters are reserved for future URL support
381-398: Consider adding warn_failures parameter for consistency.The
compute_sample_metadatafunction hasskip_failuresbut notwarn_failures, unlikecompute_metadata(line 419) which has both. This inconsistency means callers ofcompute_sample_metadatacannot control warning behavior.For API consistency, consider adding
warn_failures=Trueparameter:-def compute_sample_metadata(sample, overwrite=False, skip_failures=True): +def compute_sample_metadata(sample, overwrite=False, skip_failures=True, warn_failures=True): """Populates the ``metadata`` field of the sample. Args: sample: a :class:`fiftyone.core.sample.Sample` overwrite (False): whether to overwrite existing metadata skip_failures (True): whether to gracefully continue without raising an error if metadata cannot be computed + warn_failures (True): whether to log a warning if metadata cannot + be computed """ if not overwrite and sample.metadata is not None: return sample.metadata = _compute_sample_metadata( - sample.filepath, sample.media_type, skip_failures=skip_failures + sample.filepath, sample.media_type, skip_failures=skip_failures, warn_failures=warn_failures )fiftyone/utils/caltech.py (1)
80-80: Consider adding try-finally for scratch directory cleanup.If an error occurs during archive extraction or file moving, the scratch directory won't be cleaned up, potentially leaving temporary files on disk. Consider wrapping the main logic in a try-finally block to ensure cleanup.
Example pattern:
try: # Download, extract, move logic ... finally: # Ensure cleanup even on failure if os.path.exists(scratch_dir): etau.delete_dir(scratch_dir)Also applies to: 145-145
fiftyone/utils/sam2.py (1)
212-212: Specify device for consistency with other output tensors.The boxes tensor is created without specifying a device, defaulting to CPU, while other outputs (masks, scores) explicitly use
sam2_predictor.device. This inconsistency could cause device mismatch issues if downstream code assumes all tensors are on the same device.Apply this diff to specify the device:
- "boxes": torch.tensor(boxes_xyxy), + "boxes": torch.tensor(boxes_xyxy, device=sam2_predictor.device),fiftyone/utils/sam.py (1)
396-409: Consider simplifying chunking logic.The
chunk_size=1e6parameter suggests handling up to 1 million boxes per image, which is far beyond typical computer vision workloads (usually hundreds to thousands). Unless there's a documented use case requiring this scale, the chunking adds unnecessary complexity.Consider simplifying to:
-def _to_abs_boxes(boxes, img_width, img_height, chunk_size=1e6): +def _to_abs_boxes(boxes, img_width, img_height): boxes_xyxy = np.copy(boxes) - num_boxes = len(boxes) - - for start in range(0, num_boxes, int(chunk_size)): - end = min(start + int(chunk_size), num_boxes) - boxes_xyxy[start:end, 2] += boxes_xyxy[start:end, 0] - boxes_xyxy[start:end, 3] += boxes_xyxy[start:end, 1] - boxes_xyxy[start:end, 0] *= img_width - boxes_xyxy[start:end, 2] *= img_width - boxes_xyxy[start:end, 1] *= img_height - boxes_xyxy[start:end, 3] *= img_height - + boxes_xyxy[:, 2] += boxes_xyxy[:, 0] + boxes_xyxy[:, 3] += boxes_xyxy[:, 1] + boxes_xyxy[:, 0] *= img_width + boxes_xyxy[:, 2] *= img_width + boxes_xyxy[:, 1] *= img_height + boxes_xyxy[:, 3] *= img_height return boxes_xyxyIf large-scale processing is a documented requirement, consider adding a comment explaining the rationale for chunking.
app/packages/core/src/plugins/SchemaIO/components/DashboardView.tsx (4)
602-609: Remove redundantpanelIdparameter.The
panelIdis passed both at the top level of the event object and within theparamsobject. This redundancy could lead to confusion.Apply this diff to remove the redundant parameter:
triggerPanelEvent(panelId, { - panelId, operator: schema.view.on_edit_item, params: { id, path }, });Note: Apply similar changes to all other
triggerPanelEventcalls in the file (lines 617, 629, 761, 885, 967, 1111, 1143).Also applies to: 759-767
728-741: Timestamp-based ID generation could have collisions under rapid operations.When pasting multiple plots, IDs are generated using
Date.now()with an index suffix. If users paste multiple items in rapid succession (within the same millisecond), IDs could collide.Consider using a more robust ID generation approach:
- const plotConfigs = clipboardData.plots.map((plot, index) => { - const newId = `${plot.name || "plot"}_${Date.now()}_${index}`; + // Generate a unique timestamp with microsecond precision + const baseTimestamp = Date.now(); + const plotConfigs = clipboardData.plots.map((plot, index) => { + // Use performance.now() for sub-millisecond precision if available + const uniqueSuffix = performance?.now ? + Math.floor(performance.now() * 1000) : + baseTimestamp; + const newId = `${plot.name || "plot"}_${uniqueSuffix}_${index}`;
1333-1333: Hardcoded delay for PNG export may be insufficient.The 1000ms delay (Line 1333) assumes all plots will load within 1 second, which may not be true for complex dashboards or slow network conditions. This could result in incomplete PNG exports.
Consider implementing a more robust loading detection:
// Wait for plots to finish rendering await new Promise((resolve) => { // Check if all plots are loaded by looking for loading indicators const checkLoaded = () => { const loadingIndicators = exportContainer.querySelectorAll('.loading, .spinner'); if (loadingIndicators.length === 0) { resolve(true); } else { setTimeout(checkLoaded, 100); } }; // Start checking after a brief initial delay setTimeout(checkLoaded, 500); // Timeout after 5 seconds to prevent infinite waiting setTimeout(() => resolve(true), 5000); });
1233-1248: Large dependency array in useEffect may cause performance issues.The keyboard event handler's dependency array contains 13 dependencies (lines 1233-1248), which could cause the effect to re-run frequently and potentially lead to memory leaks from adding/removing event listeners repeatedly.
Consider stabilizing the dependencies using
useCallbackfor handler functions or moving some logic outside the effect:// Memoize handlers that don't need to change frequently const stableHandlePaste = useCallback(handlePaste, [/* minimal deps */]); const stableDeleteSelected = useCallback(deleteSelectedItems, [/* minimal deps */]); const stableCopyItems = useCallback(copyItemsToClipboard, [/* minimal deps */]); useEffect(() => { const handleKeyDown = (event) => { // Use stable handlers instead of closure over all dependencies // ... }; document.addEventListener("keydown", handleKeyDown); return () => { document.removeEventListener("keydown", handleKeyDown); }; }, [/* reduced dependency array */]);docs/source/getting_started_guides/manufacturing/09_vad_dataset.ipynb (2)
62-62: Consider pinning package versions.Installing packages without version constraints can lead to reproducibility issues. Consider pinning specific versions for
fiftyone,huggingface_hub, andgdown.Apply this diff to pin versions:
-!pip install fiftyone huggingface_hub gdown +!pip install fiftyone==1.9.0 huggingface_hub>=0.20.0 gdown>=4.7.0
164-174: Consider batch updates for better performance.Iterating and saving samples individually can be slow for large datasets. Consider using batch operations or the
iter_samples()method withautosave=True.Apply this diff for better performance:
-for sample in dataset: - sample["ground_truth"] = fo.Classification(label=sample["label"]) - sample.save() +dataset.set_values( + "ground_truth", + [fo.Classification(label=s["label"]) for s in dataset], +)install.sh (4)
60-67: Consider providing clearer guidance on missing Python.When Python is not found, consider suggesting installation methods for common platforms.
else - echo "ERROR: Neither python3 nor python found in PATH." + echo "ERROR: Neither python3 nor python found in PATH." + echo "Please install Python 3.9-3.12 before running this script." + echo "Visit https://www.python.org/downloads/ for installation instructions." exit 1 fi
102-103: Add error handling for nvm install.The
nvm installandnvm usecommands can fail if the version is invalid or download fails. Consider adding error checks.- nvm install "$NODE_VERSION" - nvm use "$NODE_VERSION" + if ! nvm install "$NODE_VERSION"; then + echo "ERROR: Failed to install Node.js $NODE_VERSION" + exit 1 + fi + if ! nvm use "$NODE_VERSION"; then + echo "ERROR: Failed to switch to Node.js $NODE_VERSION" + exit 1 + fi
114-114: Consider showing yarn install output.The yarn install output is redirected to
/dev/null, which may hide important errors or warnings. Consider showing output or at least errors.- COREPACK_ENABLE_DOWNLOAD_PROMPT=0 yarn install >/dev/null 2>&1 + COREPACK_ENABLE_DOWNLOAD_PROMPT=0 yarn install || { + echo "ERROR: yarn install failed" + exit 1 + }
226-226: Export command will not persist across sessions.The suggested
export PYTHONPATHcommand only works in the current shell session. Consider adding instructions to update.bashrcor.zshrcfor persistence.echo "NOTE: You probably want to run:" echo " export PYTHONPATH=\$PYTHONPATH:$(pwd)" +echo "To make this permanent, add the above line to your ~/.bashrc or ~/.zshrc" echo "***** INSTALLATION COMPLETE *****"fiftyone/core/collections.py (3)
10526-10531: Index matching logic is brittle; prefer spec-based comparisonUsing string normalization
name.replace("-1", "1")to find existing indexes ignores direction but may misidentify names and doesn’t consider other options (e.g., partial filters, collation). Compare specs instead:
- Compare field sequences ignoring direction via
[(f, 1) for f, _ in spec].- Consider checking option deltas (unique, partialFilterExpression, collation) before reusing an index.
This will reduce false positives/negatives when deciding reuse vs replace. I can propose a concrete patch if desired.
Also applies to: 10549-10580
5798-5798: API default change:create_index=Falsein view stages
geo_near,geo_within,group_by, andsort_bynow default to not auto-create indexes. Good for safety, but may regress performance where callers relied on implicit indexing.
- Verify docs and changelog clearly call this out.
- Consider emitting a one-time info log hint when a slow path is detected with
create_index=False.Also applies to: 5912-5912, 5983-5983, 7532-7532
10582-10611: Index conversion/replace flow: edge-case checksOverall solid. Two suggestions:
- When deciding “satisfactory index already exists”, also compare relevant options (e.g.,
sparse,partialFilterExpression,collation) to avoid reusing a mismatched index.- Allow modifying frame-level
frames.filepathconsistently or clarify the exception; current guard only allows sample-level"filepath".If helpful, I can draft a patch to include option checks in the existence test.
Also applies to: 10627-10655
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
app/package.jsonis excluded by!**/*.jsonapp/packages/aggregations/package.jsonis excluded by!**/*.jsonapp/packages/app/package.jsonis excluded by!**/*.jsonapp/packages/components/package.jsonis excluded by!**/*.jsonapp/packages/core/package.jsonis excluded by!**/*.jsonapp/packages/embeddings/package.jsonis excluded by!**/*.jsonapp/packages/flashlight/package.jsonis excluded by!**/*.jsonapp/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/packages/looker/package.jsonis excluded by!**/*.jsonapp/packages/map/package.jsonis excluded by!**/*.jsonapp/packages/operators/package.jsonis excluded by!**/*.jsonapp/packages/playback/package.jsonis excluded by!**/*.jsonapp/packages/plugins/package.jsonis excluded by!**/*.jsonapp/packages/relay/package.jsonis excluded by!**/*.jsonapp/packages/spaces/package.jsonis excluded by!**/*.jsonapp/packages/spotlight/package.jsonis excluded by!**/*.jsonapp/packages/state/package.jsonis excluded by!**/*.jsonapp/packages/utilities/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.locke2e-pw/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lockfiftyone/zoo/models/manifest-tf.jsonis excluded by!**/*.jsonfiftyone/zoo/models/manifest-torch.jsonis excluded by!**/*.json
📒 Files selected for processing (124)
.github/CODEOWNERS(1 hunks).github/workflows/build-db.yml(3 hunks).github/workflows/build-docker-internal.yml(1 hunks).github/workflows/build-docs.yml(4 hunks).github/workflows/build-graphql.yml(2 hunks).github/workflows/build.yml(2 hunks).github/workflows/e2e.yml(2 hunks).github/workflows/lint-app.yml(2 hunks).github/workflows/pr.yml(1 hunks).github/workflows/publish.yml(2 hunks).github/workflows/push-release.yml(1 hunks).github/workflows/test.yml(2 hunks).github/workflows/windows-test.yml(1 hunks)README.md(1 hunks)app/packages/app/src/Sync.tsx(2 hunks)app/packages/app/src/components/Analytics.tsx(1 hunks)app/packages/app/src/components/AnalyticsConsent.tsx(1 hunks)app/packages/app/src/components/Nav.tsx(1 hunks)app/packages/app/src/components/Setup.tsx(1 hunks)app/packages/app/src/components/Teams.tsx(1 hunks)app/packages/app/src/pages/datasets/DatasetPage.tsx(1 hunks)app/packages/app/src/routing/RouterContext.ts(2 hunks)app/packages/app/src/useSetters/onSetFieldVisibilityStage.ts(1 hunks)app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx(2 hunks)app/packages/components/src/components/ErrorBoundary/index.ts(1 hunks)app/packages/components/src/components/index.ts(1 hunks)app/packages/core/src/components/Grid/Grid.tsx(1 hunks)app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DashboardPNGExport.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DashboardResizeHandles.css(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DashboardView.tsx(13 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/evaluation/overview/index.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx(1 hunks)app/packages/embeddings/src/usePlotSelection.tsx(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(3 hunks)app/packages/looker-3d/src/hooks/use-fo-loaders.ts(2 hunks)app/packages/looker/src/elements/common/tags.ts(1 hunks)app/packages/looker/src/overlays/classifications.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/operators/src/built-in-operators.ts(2 hunks)app/packages/operators/src/index.ts(1 hunks)app/packages/operators/src/types-internal.ts(1 hunks)app/packages/operators/src/usePanelEvent.ts(4 hunks)docs/README.md(2 hunks)docs/source/cli/index.rst(5 hunks)docs/source/dataset_zoo/datasets.rst(1 hunks)docs/source/dataset_zoo/remote.rst(3 hunks)docs/source/deprecation.rst(3 hunks)docs/source/getting_started_guides/manufacturing/01_intro.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/02_embeddings.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/03_clustering.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/04_custom_embeddings.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/05_evaluation.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/06_augmentation.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/07_3d_visualization.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/08_extended_exploration.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/09_vad_dataset.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/10_ppe_detection.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/11_video_analytics.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/index.rst(1 hunks)docs/source/getting_started_guides/manufacturing/summary.rst(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/01_intro.ipynb(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/02_explore.ipynb(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/index.rst(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/summary.rst(1 hunks)docs/source/getting_started_guides/segmentation/01_intro.ipynb(1 hunks)docs/source/getting_started_guides/segmentation/02_explore.ipynb(1 hunks)docs/source/getting_started_guides/segmentation/03_sam2.ipynb(1 hunks)docs/source/getting_started_guides/segmentation/index.rst(1 hunks)docs/source/getting_started_guides/segmentation/summary.rst(1 hunks)docs/source/integrations/activitynet.rst(1 hunks)docs/source/integrations/coco.rst(1 hunks)docs/source/integrations/open_images.rst(1 hunks)docs/source/plugins/developing_plugins.rst(1 hunks)docs/source/release-notes.rst(1 hunks)docs/source/user_guide/config.rst(1 hunks)docs/source/user_guide/evaluation.rst(3 hunks)docs/source/user_guide/using_datasets.rst(2 hunks)e2e-pw/src/oss/poms/field-visibility/field-visibility.ts(1 hunks)e2e-pw/src/oss/poms/sidebar.ts(1 hunks)e2e-pw/src/oss/specs/smoke-tests/field-visibility.spec.ts(4 hunks)e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts(2 hunks)fiftyone/core/cli.py(12 hunks)fiftyone/core/collections.py(18 hunks)fiftyone/core/config.py(1 hunks)fiftyone/core/dataset.py(7 hunks)fiftyone/core/metadata.py(13 hunks)fiftyone/core/models.py(22 hunks)fiftyone/core/odm/__init__.py(1 hunks)fiftyone/core/odm/database.py(3 hunks)fiftyone/core/sample.py(1 hunks)fiftyone/core/stages.py(15 hunks)fiftyone/core/utils.py(1 hunks)fiftyone/core/validation.py(1 hunks)fiftyone/factory/repos/delegated_operation.py(5 hunks)fiftyone/factory/repos/delegated_operation_doc.py(5 hunks)fiftyone/operators/__init__.py(1 hunks)fiftyone/operators/_types/__init__.py(1 hunks)fiftyone/operators/_types/pipeline.py(1 hunks)fiftyone/operators/delegated.py(11 hunks)fiftyone/operators/executor.py(9 hunks)fiftyone/operators/operations.py(1 hunks)fiftyone/operators/operator.py(2 hunks)fiftyone/operators/panel.py(1 hunks)fiftyone/server/app.py(1 hunks)fiftyone/server/decorators.py(3 hunks)fiftyone/server/routes/__init__.py(2 hunks)fiftyone/server/routes/sample.py(1 hunks)fiftyone/utils/caltech.py(1 hunks)fiftyone/utils/eval/base.py(2 hunks)fiftyone/utils/eval/classification.py(2 hunks)fiftyone/utils/labels.py(12 hunks)fiftyone/utils/sam.py(4 hunks)fiftyone/utils/sam2.py(3 hunks)fiftyone/utils/torch.py(2 hunks)fiftyone/utils/ultralytics.py(1 hunks)fiftyone/utils/utils3d.py(3 hunks)fiftyone/zoo/datasets/base.py(4 hunks)fiftyone/zoo/models/__init__.py(1 hunks)install.bash(0 hunks)install.sh(1 hunks)
💤 Files with no reviewable changes (1)
- install.bash
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/ErrorBoundary/index.tsapp/packages/app/src/components/Teams.tsxapp/packages/operators/src/types-internal.tsapp/packages/looker-3d/src/hooks/use-fo-loaders.tsapp/packages/looker/src/overlays/classifications.tsapp/packages/app/src/components/AnalyticsConsent.tsxapp/packages/core/src/components/Grid/Grid.tsxapp/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsxapp/packages/app/src/pages/datasets/DatasetPage.tsxapp/packages/components/src/components/index.tsapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsxapp/packages/operators/src/usePanelEvent.tsapp/packages/looker/src/elements/common/tags.tsapp/packages/app/src/useSetters/onSetFieldVisibilityStage.tsapp/packages/operators/src/index.tsapp/packages/embeddings/src/usePlotSelection.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.tse2e-pw/src/oss/poms/field-visibility/field-visibility.tsapp/packages/app/src/components/Nav.tsxapp/packages/app/src/routing/RouterContext.tsapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/evaluation/overview/index.tsxe2e-pw/src/oss/specs/smoke-tests/field-visibility.spec.tse2e-pw/src/oss/poms/sidebar.tse2e-pw/src/oss/specs/smoke-tests/quickstart.spec.tsapp/packages/core/src/plugins/SchemaIO/components/DashboardView.tsxapp/packages/looker/src/overlays/detection.tsapp/packages/app/src/Sync.tsxapp/packages/operators/src/built-in-operators.tsapp/packages/app/src/components/Setup.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsxapp/packages/app/src/components/Analytics.tsxapp/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsxapp/packages/core/src/plugins/SchemaIO/components/DashboardPNGExport.tsx
🧠 Learnings (9)
📚 Learning: 2025-09-10T03:01:27.501Z
Learnt from: swheaton
PR: voxel51/fiftyone#6334
File: fiftyone/core/expressions.py:2727-2741
Timestamp: 2025-09-10T03:01:27.501Z
Learning: FiftyOne requires MongoDB >= 6.0 as documented at https://docs.voxel51.com/user_guide/config.html#using-a-different-mongodb-version
Applied to files:
docs/source/deprecation.rst
📚 Learning: 2025-01-30T16:40:26.003Z
Learnt from: swheaton
PR: voxel51/fiftyone#5448
File: fiftyone/factory/repos/delegated_operation_doc.py:0-0
Timestamp: 2025-01-30T16:40:26.003Z
Learning: In FiftyOne's DelegatedOperationDocument class, all instance attributes are automatically serialized through `__dict__` in the `to_pymongo()` method, eliminating the need for explicit field inclusion in serialization.
Applied to files:
fiftyone/factory/repos/delegated_operation_doc.py
📚 Learning: 2025-06-16T21:08:14.104Z
Learnt from: swheaton
PR: voxel51/fiftyone#6023
File: tests/unittests/factory/delegated_operation_doc_tests.py:55-57
Timestamp: 2025-06-16T21:08:14.104Z
Learning: The DelegatedOperationDocument class in fiftyone/factory/repos/delegated_operation_doc.py inherits from object (plain Python class), not from Pydantic BaseModel. Using setattr to modify its attributes in tests is appropriate and does not bypass any validation.
Applied to files:
fiftyone/factory/repos/delegated_operation_doc.py
📚 Learning: 2025-09-12T15:01:34.588Z
Learnt from: afoley587
PR: voxel51/fiftyone#6339
File: .github/workflows/build-db.yml:112-116
Timestamp: 2025-09-12T15:01:34.588Z
Learning: For GitHub Actions workflows using pypa/gh-action-pypi-publish with trusted publishing: only `contents: read` and `id-token: write` permissions are required. `actions: read` is only needed when downloading artifacts from other workflow runs/repositories (not the same workflow run). `attestations: write` is only needed if uploading attestations to GitHub's attestation store, not for standard PyPI publishing which uploads attestations to PyPI automatically.
Applied to files:
.github/workflows/build-db.yml.github/workflows/publish.yml
📚 Learning: 2024-09-27T23:14:11.193Z
Learnt from: sashankaryal
PR: voxel51/fiftyone#4863
File: app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx:39-48
Timestamp: 2024-09-27T23:14:11.193Z
Learning: In `DynamicGroupCarousel`, using `useRecoilValue(fos.groupByFieldValue)` causes the flashlight to behave unexpectedly—specifically, it resets to the front when scrolling the carousel and selecting samples. Therefore, `setInterval` is currently used as a workaround to update `groupByValue`.
Applied to files:
app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx
📚 Learning: 2024-09-27T23:14:46.454Z
Learnt from: sashankaryal
PR: voxel51/fiftyone#4863
File: app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx:74-74
Timestamp: 2024-09-27T23:14:46.454Z
Learning: In `DynamicGroupCarousel`, setting the `key` prop on `DynamicGroupsFlashlightWrapper` to `groupByValue` is intentional to force a remount when `groupByValue` changes.
Applied to files:
app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx
📚 Learning: 2025-10-16T17:24:11.562Z
Learnt from: brimoor
PR: voxel51/fiftyone#6373
File: docs/source/dataset_zoo/datasets/ucf101.rst:32-0
Timestamp: 2025-10-16T17:24:11.562Z
Learning: In FiftyOne documentation, dataset zoo classes follow the pattern `fiftyone.zoo.datasets.base.{DatasetName}Dataset` and should be referenced using `:class:` directives (e.g., `:class:`UCF101Dataset <fiftyone.zoo.datasets.base.UCF101Dataset>``). This is a valid and established documentation pattern.
Applied to files:
docs/source/getting_started_guides/model_dataset_zoo/index.rst
📚 Learning: 2025-09-10T19:53:08.432Z
Learnt from: AdonaiVera
PR: voxel51/fiftyone#6337
File: docs/source/tutorials/dinov3.ipynb:121-124
Timestamp: 2025-09-10T19:53:08.432Z
Learning: The `foz.load_zoo_dataset()` function supports loading datasets from external sources including GitHub URLs (e.g., "https://github.com/voxel51/coco-2017"), which is different from loading built-in zoo datasets by name (e.g., "coco-2017"). GitHub URL loading allows access to custom datasets not available in the standard FiftyOne Dataset Zoo.
Applied to files:
docs/source/getting_started_guides/model_dataset_zoo/01_intro.ipynb
📚 Learning: 2025-06-16T21:07:49.946Z
Learnt from: swheaton
PR: voxel51/fiftyone#6023
File: fiftyone/operators/executor.py:1326-1332
Timestamp: 2025-06-16T21:07:49.946Z
Learning: In fiftyone/operators/executor.py, the ExecutionOptions.__init__ method intentionally ignores the allow_distributed_execution parameter by hardcoding _allow_distributed_execution to False. This is deliberate scaffolding behavior to maintain API compatibility while reserving distributed execution functionality for FiftyOne Enterprise only.
Applied to files:
fiftyone/operators/executor.py
| steps: | ||
| - name: Download docs | ||
| uses: actions/download-artifact@v4 | ||
| uses: actions/download-artifact@v5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the actions/download-artifact@v5 upgrade.
Confirm v5 compatibility and check for any breaking changes.
🌐 Web query:
What is the latest stable version of actions/download-artifact and are there any breaking changes in v5?
💡 Result:
Latest stable: v5.0.0 (release). [1][2]
Breaking changes in v5:
- Fixes an inconsistency in extraction path for single-artifact downloads by ID — previously downloading by ID placed files in path//, now a single-artifact download by ID extracts directly into the provided path (matching downloads by name). This can change the output location for workflows that relied on the old nested directory; migration guidance recommends explicitly including the artifact name in path if you need the old behaviour. [1][2]
Sources: GitHub Releases for actions/download-artifact (v5.0.0) and changelog. [1][2]
Update download-artifact step to include artifact name in path for v5
actions/[email protected] removes the default artifact-name subfolder for single-ID downloads and extracts directly into the given path. If your workflow expects a nested directory, amend the path in .github/workflows/build-docs.yml:160 to include the artifact name explicitly.
🤖 Prompt for AI Agents
.github/workflows/build-docs.yml around line 160: the
actions/download-artifact@v5 step now extracts directly into the specified path
(no implicit artifact-name subfolder), so update the step's "path" to include
the artifact name explicitly (e.g., append the artifact directory you expect, or
use the artifact name variable) so subsequent steps that rely on a nested
artifact-name directory continue to find files; ensure the new path matches the
artifact name used when uploading.
| path: docs-download/ | ||
| - name: Authorize gcloud | ||
| uses: google-github-actions/auth@v2 | ||
| uses: google-github-actions/auth@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the google-github-actions/auth@v3 upgrade.
Ensure v3 is stable and check for breaking changes in authentication setup.
🌐 Web query:
What is the latest stable version of google-github-actions/auth and are there any breaking changes in v3?
💡 Result:
Latest stable: v3 (published as v3.0.0). [1][2]
Breaking changes in v3: bump to Node 24 (runner must support Node 24+) and removal of some older parameters/flags (and removal of a legacy "hacky" script). These are the primary breaking items called out in the v3 release notes. [1][2]
Sources:
[1] google-github-actions/auth Releases (v3 / v3.0.0).
[2] Release: v3.0.0 — changelog (Bump to Node 24; remove old parameters; remove hacky script).
Ensure runner supports Node 24+ and remove deprecated flags for google-github-actions/auth@v3: v3.0.0 is the latest stable release; it bumps to Node 24 and removes older parameters/flags and a legacy script—update your runner environment to Node 24+ and remove or update any deprecated flags in .github/workflows/build-docs.yml.
🤖 Prompt for AI Agents
.github/workflows/build-docs.yml around line 165: the workflow uses
google-github-actions/auth@v3 which requires Node 24+ and no longer accepts
deprecated flags/legacy script options; update the workflow's runner to a Node
24+ environment (e.g., ubuntu-latest with actions/setup-node@v4 configuring
node-version: 24) and remove or replace any deprecated parameters or legacy
script invocations passed to google-github-actions/auth (update to the
v3-compatible inputs as documented), ensuring the auth step only uses supported
inputs for v3.
| credentials_json: "${{ secrets.DOCS_GCP_CREDENTIALS }}" | ||
| - name: Set up gcloud | ||
| uses: google-github-actions/setup-gcloud@v2 | ||
| uses: google-github-actions/setup-gcloud@v3 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the google-github-actions/setup-gcloud@v3 upgrade.
Confirm v3 compatibility and check for any breaking changes.
🌐 Web query:
What is the latest stable version of google-github-actions/setup-gcloud and are there any breaking changes in v3?
💡 Result:
Latest stable release: v3.0.1. [1]
Breaking changes in v3:
- Requires Node 24+ on the runner. [1][2]
- The skip_tool_cache option was removed; the action no longer uses the tool-cache by default (to use the old behavior enable caching with cache: true). [1][2]
Sources: GitHub releases and repo README. [1][2]
Ensure Node 24+ runner and update caching for setup-gcloud v3
- v3.0.1 requires Node 24+ (update your runner image or add
actions/setup-node@v3) skip_tool_cachewas removed (caching disabled by default; usecache: trueto re-enable)
File: .github/workflows/build-docs.yml:169
🤖 Prompt for AI Agents
In .github/workflows/build-docs.yml around line 169, update the setup for
google-github-actions/setup-gcloud@v3 to meet v3 requirements by ensuring the
job runs on a Node 24+ environment (either set the runner image that includes
Node 24 or add an actions/setup-node@v3 step before this action to install Node
24+), remove any use of the removed skip_tool_cache option, and if you want
caching re-enabled set the action input with with: cache: true (or leave omitted
to keep caching disabled).
| setTimeout(() => { | ||
| if (window.Plotly) { | ||
| const plotlyDivs = document.querySelectorAll(".js-plotly-plot"); | ||
| plotlyDivs.forEach((div) => { | ||
| if (div && window.Plotly) { | ||
| window.Plotly.relayout(div as HTMLElement, { | ||
| width: div.clientWidth, | ||
| height: div.clientHeight, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| }, 100); // Small delay to ensure DOM has updated |
There was a problem hiding this comment.
Verify Plotly availability before accessing global object.
The code assumes window.Plotly exists without checking if Plotly is loaded. This could throw errors if Plotly hasn't been initialized yet.
Apply this diff to add a safety check:
// Force Plotly plots to resize after layout changes
setTimeout(() => {
- if (window.Plotly) {
+ if (typeof window !== 'undefined' && window.Plotly) {
const plotlyDivs = document.querySelectorAll(".js-plotly-plot");
plotlyDivs.forEach((div) => {
- if (div && window.Plotly) {
+ if (div) {
window.Plotly.relayout(div as HTMLElement, {
width: div.clientWidth,
height: div.clientHeight,
});📝 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.
| setTimeout(() => { | |
| if (window.Plotly) { | |
| const plotlyDivs = document.querySelectorAll(".js-plotly-plot"); | |
| plotlyDivs.forEach((div) => { | |
| if (div && window.Plotly) { | |
| window.Plotly.relayout(div as HTMLElement, { | |
| width: div.clientWidth, | |
| height: div.clientHeight, | |
| }); | |
| } | |
| }); | |
| } | |
| }, 100); // Small delay to ensure DOM has updated | |
| // Force Plotly plots to resize after layout changes | |
| setTimeout(() => { | |
| if (typeof window !== 'undefined' && window.Plotly) { | |
| const plotlyDivs = document.querySelectorAll(".js-plotly-plot"); | |
| plotlyDivs.forEach((div) => { | |
| if (div) { | |
| window.Plotly.relayout(div as HTMLElement, { | |
| width: div.clientWidth, | |
| height: div.clientHeight, | |
| }); | |
| } | |
| }); | |
| } | |
| }, 100); // Small delay to ensure DOM has updated |
🤖 Prompt for AI Agents
In app/packages/core/src/plugins/SchemaIO/components/DashboardView.tsx around
lines 1428 to 1440, the code accesses window.Plotly without fully verifying
Plotly is available; update the block to first confirm the global exists (e.g.
check typeof window !== "undefined" && window.Plotly), store it in a local const
(const Plotly = window.Plotly) and use that for relayout calls, and if Plotly is
not present simply skip the relayout (or schedule a retry) to avoid runtime
errors when Plotly hasn't loaded.
| function ensureArray(value) { | ||
| if (Array.isArray(value)) { | ||
| return value; | ||
| } else { | ||
| return [value]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add TypeScript type annotations to ensureArray.
The helper function lacks type annotations for both its parameter and return type, resulting in implicit any types that bypass TypeScript's type safety.
Apply this diff to add proper type annotations:
-function ensureArray(value) {
+function ensureArray<T>(value: T | T[]): T[] {
if (Array.isArray(value)) {
return value;
} else {
return [value];
}
}📝 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 ensureArray(value) { | |
| if (Array.isArray(value)) { | |
| return value; | |
| } else { | |
| return [value]; | |
| } | |
| } | |
| function ensureArray<T>(value: T | T[]): T[] { | |
| if (Array.isArray(value)) { | |
| return value; | |
| } else { | |
| return [value]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/packages/looker-3d/src/hooks/use-fo-loaders.ts around lines 3 to 9, the
helper ensureArray currently has no TypeScript annotations; change it to a
generic typed signature like ensureArray<T>(value: T | T[]): T[] so the
parameter accepts either a single T or an array of T and the return type is
always T[]; keep the implementation logic the same but add the generic type
parameter and the union parameter/array return types to restore type safety.
| if camera_intrinsics.shape[0] > 4 or camera_intrinsics.shape[1] > 4: | ||
| raise ValueError("intrinsics must be 3x3 or 4x4") |
There was a problem hiding this comment.
Verify the intrinsics shape validation logic.
The condition checks if either dimension is greater than 4, but this allows invalid shapes like 5x3 or 3x5. The validation should ensure the matrix is exactly 3x3 or 4x4.
Apply this diff to fix the validation:
- if camera_intrinsics.shape[0] > 4 or camera_intrinsics.shape[1] > 4:
- raise ValueError("intrinsics must be 3x3 or 4x4")
+ if camera_intrinsics.shape not in [(3, 3), (4, 4)]:
+ raise ValueError("intrinsics must be 3x3 or 4x4")📝 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.
| if camera_intrinsics.shape[0] > 4 or camera_intrinsics.shape[1] > 4: | |
| raise ValueError("intrinsics must be 3x3 or 4x4") | |
| if camera_intrinsics.shape not in [(3, 3), (4, 4)]: | |
| raise ValueError("intrinsics must be 3x3 or 4x4") |
🤖 Prompt for AI Agents
In fiftyone/utils/utils3d.py around lines 594-595, the intrinsics shape check is
incorrect because it permits shapes like 5x3 or 3x5; replace the current
condition with one that explicitly requires the matrix to be exactly 3x3 or
exactly 4x4 (e.g., if camera_intrinsics.shape not in ((3, 3), (4, 4)): raise
ValueError("intrinsics must be 3x3 or 4x4")), ensuring you access
camera_intrinsics.shape safely (it must be a 2-tuple) before comparing.
| @@ -0,0 +1,227 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use bash for better compatibility.
The script uses several bash-specific features (like [[ conditional and command -v), but declares #!/bin/sh. Consider using #!/bin/bash for better compatibility and consistency.
Apply this diff:
-#!/bin/sh
+#!/bin/bash📝 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.
| #!/bin/sh | |
| #!/bin/bash |
🤖 Prompt for AI Agents
In install.sh around line 1, the script declares a /bin/sh shebang but uses
bash-specific features; update the shebang to #!/bin/bash so the script runs
with bash (ensuring compatibility with [[ and other bash builtins) and keep
existing script logic unchanged.
| echo "***** INSTALLING FIFTYONE-APP *****" | ||
| if ! command -v nvm >/dev/null 2>&1; then | ||
| echo "Installing nvm..." | ||
| curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | sh |
There was a problem hiding this comment.
Security: Piping curl to shell is risky.
Downloading and executing a script directly from the internet without verification is a security risk. Consider adding verification steps or checksum validation.
Consider this safer approach:
- curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | sh
+ NVM_INSTALL_SCRIPT=$(mktemp)
+ curl -o "$NVM_INSTALL_SCRIPT" https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh
+ # Optionally verify checksum here
+ sh "$NVM_INSTALL_SCRIPT"
+ rm "$NVM_INSTALL_SCRIPT"📝 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.
| curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | sh | |
| NVM_INSTALL_SCRIPT=$(mktemp) | |
| curl -o "$NVM_INSTALL_SCRIPT" https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | |
| # Optionally verify checksum here | |
| sh "$NVM_INSTALL_SCRIPT" | |
| rm "$NVM_INSTALL_SCRIPT" |
🤖 Prompt for AI Agents
In install.sh around line 89, the script pipes a remote installer directly into
sh which is a security risk; instead, download the installer to a temporary file
(using a safe curl invocation), fetch and validate its integrity (compare
against a known SHA256 checksum or verify a GPG signature retrieved from a
trusted source), only execute the downloaded file if verification succeeds, and
fail the install otherwise; also ensure the version is pinned and use curl
options that fail on error before running the validated script.
| if [ "$OS" = "Darwin" ]; then | ||
| MONGODB_BUILD="mongodb-macos-x86_64-$MONGODB_VERSION" | ||
| curl "https://fastdl.mongodb.org/osx/${MONGODB_BUILD}.tgz" --output mongodb.tgz | ||
| tar -zxvf mongodb.tgz | ||
| mv "${MONGODB_BUILD}/bin/"* ./bin/ | ||
| rm -rf mongodb.tgz "${MONGODB_BUILD}" | ||
| elif [ "$OS" = "Linux" ]; then | ||
| MONGODB_BUILD="mongodb-linux-x86_64-ubuntu2204-$MONGODB_VERSION" | ||
| curl "https://fastdl.mongodb.org/linux/${MONGODB_BUILD}.tgz" --output mongodb.tgz | ||
| tar -zxvf mongodb.tgz | ||
| mv "${MONGODB_BUILD}/bin/"* ./bin/ | ||
| rm -rf mongodb.tgz "${MONGODB_BUILD}" | ||
| else | ||
| echo "WARNING: unsupported OS, skipping MongoDB installation" | ||
| fi |
There was a problem hiding this comment.
Add verification for downloaded MongoDB binaries.
Downloading binaries without checksum verification is a security risk. Consider adding SHA256 checksum validation.
Consider adding checksum verification:
# After downloading mongodb.tgz
EXPECTED_SHA256="<expected_checksum>"
ACTUAL_SHA256=$(shasum -a 256 mongodb.tgz | cut -d' ' -f1)
if [ "$ACTUAL_SHA256" != "$EXPECTED_SHA256" ]; then
echo "ERROR: MongoDB download checksum mismatch"
exit 1
fi🤖 Prompt for AI Agents
In install.sh around lines 140 to 154, the script downloads MongoDB archives but
never verifies integrity; add SHA256 checksum validation after each curl
download: fetch or define the expected checksum (for example by downloading the
vendor-provided checksum file or embedding the known checksum into a variable),
compute the actual checksum (using shasum -a 256 or sha256sum), compare them,
and if they differ print an error, remove the incomplete download, and exit with
non‑zero status; also ensure curl uses fail/redirect options (e.g. -fL) so
failed HTTP responses are detected before checksum validation.
| elif [ "$OS" = "Linux" ]; then | ||
| MONGODB_BUILD="mongodb-linux-x86_64-ubuntu2204-$MONGODB_VERSION" | ||
| curl "https://fastdl.mongodb.org/linux/${MONGODB_BUILD}.tgz" --output mongodb.tgz | ||
| tar -zxvf mongodb.tgz | ||
| mv "${MONGODB_BUILD}/bin/"* ./bin/ | ||
| rm -rf mongodb.tgz "${MONGODB_BUILD}" |
There was a problem hiding this comment.
Linux MongoDB version may not work on all distributions.
The script assumes Ubuntu 22.04 compatibility for all Linux systems. This may fail on other distributions like CentOS, Fedora, or older Ubuntu versions.
Consider detecting the distribution:
if [ -f /etc/os-release ]; then
. /etc/os-release
# Select MongoDB build based on $ID and $VERSION_ID
fi🤖 Prompt for AI Agents
In install.sh around lines 146 to 151, the script unconditionally downloads the
MongoDB build for Ubuntu 22.04 which will fail on other Linux distributions;
detect the distribution and version first (e.g., source /etc/os-release and use
$ID and $VERSION_ID), map common IDs/versions to the correct MongoDB build
string (handle ubuntu, debian, centos/rhel, fedora and fall back to a generic or
error), then construct the appropriate MONGODB_BUILD variable for that platform
or exit with a helpful message if unsupported; ensure the curl/tar/mv steps use
that computed build name.
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/packages/looker/src/overlays/detection.ts (1)
136-141: Validate convexHull is non-empty before 3D rendering.The simplified condition now triggers 3D rendering whenever
convexHullis defined, butfillRectFor3d(line 309) directly accessesconvexHull[0]without checking if the array contains elements. An emptyconvexHullarray would cause a runtime error when destructuringconvexHull[0][0]andconvexHull[0][1].Apply this diff to add a length check:
- if (this.label.convexHull) { - // only fill 3d when 'convexHull' is defined + if (this.label.convexHull && this.label.convexHull.length > 0) { + // only fill 3d when 'convexHull' is defined and non-empty this.fillRectFor3d(ctx, state, strokeColor); } else { this.strokeRect(ctx, state, strokeColor); }README.md (1)
145-145: Update the upgrade instructions to use the new script name.Line 145 still references
bash install.bashin the upgrade instructions, but the installation instructions at Line 119 were updated to./install.sh. This inconsistency should be corrected.Apply this diff to fix the inconsistency:
# Mac or Linux -bash install.bash +./install.shfiftyone/core/stages.py (1)
3092-3121: Reverting the auto-index default breaks existing GeoNear/GeoWithin usageBy switching
_GeoStageand the GeoNear/GeoWithin constructors tocreate_index=Falseby default, we now skip the automatic 2dsphere index creation that existing callers relied on. Many datasets ship without that index; previously those calls worked out of the box because we transparently added it. With this change the same code path will start throwing MongoDB runtime errors ($geoNear requires a 2dsphere index) unless every caller is updated to opt in explicitly. That’s a hard regression in behavior for deployed integrations. Either keep the default atTrueor add defensive validation that fails early with a clear message and ensure downstream consumers are updated before we ship.Also applies to: 3222-3233, 3382-3392
fiftyone/core/metadata.py (1)
370-377: Guard against skipped asset metadata before dereferencing it.When
skip_failuresisTrue,_compute_sample_metadata()returnsNoneon failure. This loop still callsmetadata.size_bytes, so any asset failure now raisesAttributeError, defeating the new skip logic for 3D scenes. Skip the size/caching work when metadata isNone.for task, result in zip(tasks, results): - metadata = result[1] - asset_size += metadata.size_bytes - - if cache is not None: - scene_path = task[1] - cache[scene_path] = metadata + metadata = result[1] + if metadata is None: + continue + + asset_size += metadata.size_bytes + + if cache is not None: + scene_path = task[1] + cache[scene_path] = metadata
♻️ Duplicate comments (31)
docs/source/deprecation.rst (1)
13-23: Fix misaligned section anchors (unresolved from past review).The anchor
.. _deprecation-kubernetes-1.30:on line 13 still precedes the new MongoDB 6.0 section (lines 15–23) rather than the Kubernetes 1.30 section (line 24). This breaks existing references to#deprecation-kubernetes-1.30, which will now resolve to MongoDB 6.0 content instead. Additionally, the new MongoDB 6.0 section lacks an anchor label.Relocate anchors to align with their corresponding sections, as demonstrated by the correctly positioned anchors for MongoDB 5.0 (line 33), Kubernetes 1.29 (line 44), etc.
Apply this diff to fix the anchor alignment:
-.. _deprecation-kubernetes-1.30: - +.. _deprecation-mongodb-6.0: + MongoDB 6.0 ----------- *Support ending December 1, 2025* @@ -21,6 +21,8 @@ MongoDB 6.0 releases after December 1, 2025 might no longer be compatible with MongoDB 6.0 and older. +.. _deprecation-kubernetes-1.30: + Kubernetes 1.30 --------------- *Support ended July 11, 2025*app/packages/looker-3d/src/hooks/use-fo-loaders.ts (2)
3-9: Add TypeScript type annotations toensureArray.This issue was previously flagged and remains unresolved. The helper function lacks type annotations, resulting in implicit
anytypes.Apply this diff to add proper type annotations:
-function ensureArray(value) { +function ensureArray<T>(value: T | T[]): T[] { if (Array.isArray(value)) { return value; } else { return [value]; } }As per coding guidelines.
27-32: Security risk: substring matching for credential forwarding.Two concerns:
The type mismatch (previously flagged):
urlstyped asstring | string[]but code handlesstring[][]at runtime. Consider updating the type definition.NEW CRITICAL ISSUE: Line 31 uses substring matching (
url.includes(customCredentialsAudience)) to determine whether to forward credentials. This is unsafe—a malicious URL likehttps://evil.com/path?q=customCredentialsAudiencewould match and receive credentials.Use proper URL parsing and origin comparison instead:
if (customCredentialsAudience) { const urlArray = ensureArray(urls).flat(); - if (urlArray.some((url) => url.includes(customCredentialsAudience))) { + if (urlArray.some((url) => { + try { + const urlObj = new URL(url); + return urlObj.origin === customCredentialsAudience; + } catch { + return false; + } + })) { loaderInstance.setWithCredentials(true); } }This ensures credentials are only sent to exact origin matches, not arbitrary URLs containing the audience string.
As per coding guidelines.
.github/workflows/build-docs.yml (1)
160-160: ** Verify Node 24+ support and download-artifact@v5 path extraction.**These action upgrades were previously flagged with breaking changes (see past review comments). Key concerns remain:
- download-artifact@v5 (lines 160, 181): Extracts directly to the specified path without artifact-name nesting. Verify the downstream rsync command at line 171 correctly finds files at
docs-download/rather thandocs-download/docs/.- auth@v3 and setup-gcloud@v3 (lines 165–169, 186–190): Require Node 24+. Confirm that
ubuntu-latest-mrunner includes Node 24 or add an explicitactions/setup-node@v4step before these actions.Also applies to: 165-165, 169-169
docs/source/getting_started_guides/model_dataset_zoo/02_explore.ipynb (1)
1120-1128: Misleading comment needs correction.The comment on line 1121 states "Now evaluate on the 'defect2' field", but the actual code evaluates the
"predictions"field against"ground_truth". This comment should be updated to accurately reflect what the code does.Apply this diff to correct the comment:
eval_key = "class_eval" - # Now evaluate on the "defect2" field + # Evaluate predictions against ground truth eval_classif_padim = dataset.evaluate_classifications( "predictions", gt_field="ground_truth",fiftyone/core/models.py (2)
673-679: Critical: Emptyselect_fields()call breaks non-SamplesMixin modelsThis issue was previously flagged and remains unresolved. When
modelis not aSamplesMixin(the common case), line 678 callssamples.select_fields()with no arguments, which raisesTypeErrorbecauseSampleCollection.select_fields()requires at least one field name. This breaksapply_model()for the majority of models before any inference occurs.Apply the previously suggested fix:
def _select_fields_for_inference(samples, model): - if isinstance(model, SamplesMixin): - fields = list(model.needs_fields.values()) - return samples.select_fields(fields) - else: - return samples.select_fields() + if isinstance(model, SamplesMixin): + fields = [f for f in model.needs_fields.values() if f] + if fields: + return samples.select_fields(fields) + return samples
1432-1438: Critical: Emptyselect_fields()call breaks embedding flowsThis issue was previously flagged and remains unresolved. When
embeddings_fieldisNone(the common case where embeddings are returned in-memory) or doesn't contain a dot, line 1437 callssamples.select_fields()with no arguments, raising the sameTypeError. This causescompute_embeddings()to crash immediately for most use cases.Apply the previously suggested fix:
def _select_fields_for_embeddings(samples, embeddings_field): - if embeddings_field is not None and "." in embeddings_field: - root_field = embeddings_field.split(".", 1)[0] - return samples.select_fields(root_field) - else: - return samples.select_fields() + if embeddings_field and "." in embeddings_field: + root_field = embeddings_field.split(".", 1)[0] + return samples.select_fields(root_field) + return samplesfiftyone/core/odm/database.py (1)
504-511: Add error handling for robust public API.The function lacks error handling for
client.server_info()and dictionary access, which could raise exceptions on connection failures or if the version key is missing. The similar internal function_validate_db_version()at lines 343-359 demonstrates the proper error handling pattern that should be applied here.docs/source/getting_started_guides/segmentation/02_explore.ipynb (2)
126-131: Field name mismatch will cause runtime error.This is a known issue from previous reviews. The
prompt_fieldparameter references"ground_truth_segmentations", but the dataset import at line 42 stores segmentations under"ground_truth".
207-248: Incorrect mask attribute usage.This is a known issue from previous reviews.
fo.Detectiondoes not support amask_pathattribute. The mask data should be passed directly via themaskparameter.fiftyone/utils/caltech.py (2)
73-76: Replace bareexceptwithexcept ExceptionThis block still swallows
SystemExit,KeyboardInterrupt, etc. CatchExceptioninstead and log the failure so we can debug cleanup issues.- except: - pass + except Exception as exc: + logger.debug( + "Failed to delete extraneous file %s: %s", + os.path.join(dataset_dir, "BACKGROUND_Google", "tmp"), + exc, + )
133-141: Replace bareexceptwithexcept ExceptionSame concern here: catch only
Exception(notBaseException) and emit a debug log so we know when cleanup fails.- except: - pass + except Exception as exc: + logger.debug( + "Failed to delete extraneous directory %s: %s", + os.path.join(dataset_dir, "056.dog", "greg"), + exc, + ) @@ - except: - pass + except Exception as exc: + logger.debug( + "Failed to delete extraneous file %s: %s", + os.path.join(dataset_dir, "198.spider", "RENAME2"), + exc, + )fiftyone/operators/delegated.py (1)
745-755: Mark operation as FAILED when monitoring encounters an exception.The exception handler logs the error and terminates the child process but does not update the operation's state in the database. This leaves the operation stuck in RUNNING state.
Apply this diff to mark the operation as FAILED:
except Exception as e: reason = f"Error in monitoring loop: {e}" logger.error( "Error in monitoring loop for operation %s: %s", operation_id, e, ) + err = ExecutionResult(error=reason) + try: + self.set_failed(doc_id=operation_id, result=err) + except Exception: + logger.exception( + "Failed to mark operation %s as FAILED after monitor error", + operation_id, + ) self._terminate_child_process( child_process, operation_id, reason ) - return ExecutionResult(error=reason) + return errfiftyone/core/cli.py (1)
3434-3461: Handle non-dict operation results.At line 3456,
_print_dict_as_table(result)assumes the result is a dictionary. However, operators can return scalars, lists, or other JSON-serializable types, which will cause anAttributeError.Apply this diff to handle different result types:
if op.run_state == fooe.ExecutionRunState.COMPLETED: result = op._doc.get("result", None) if result: result = result.get("result", None) - if result: - _print_dict_as_table(result) + if result is not None: + if isinstance(result, dict): + _print_dict_as_table(result) + else: + print(etas.json_to_str(result)) else: print( "Cannot get output for operation %s in state %s" % (args.id, op.run_state) )fiftyone/operators/panel.py (1)
167-176: Initializeresultbefore the conditionalWhen
hasattr(self, method_name)is false,resultis never defined and Line 176 raisesNameError. Please initialize it toNonebefore theifand leave the existing assignment inside the branch.- # trigger the event - if hasattr(self, method_name): + # trigger the event + result = None + if hasattr(self, method_name): method = getattr(self, method_name) ctx.event_args = event_args result = method(ctx) @@ - return result + return resultdocs/source/getting_started_guides/manufacturing/06_augmentation.ipynb (1)
66-80: Handle missingMVTec_ADdataset before cloningOn a clean environment this cell calls
fo.load_dataset("MVTec_AD")without ensuring the source dataset exists, so the notebook aborts on first run. Please add a fallback that downloads (for example viafiftyone.utils.huggingface.load_from_hub) when the source dataset is absent before cloning it.- print(f"Dataset '{dataset_name}' does not exist. Creating a new one...") - # Clone the dataset with a new name and make it persistent - dataset_ = fo.load_dataset("MVTec_AD") - dataset = dataset_.clone(dataset_name, persistent=True) + print(f"Dataset '{dataset_name}' does not exist. Creating a new one...") + source_name = "MVTec_AD" + if source_name not in fo.list_datasets(): + print(f"Source dataset '{source_name}' not found. Downloading from Hugging Face...") + dataset_ = fouh.load_from_hub( + "Voxel51/mvtec-ad", + dataset_name=source_name, + persistent=True, + overwrite=False, + ) + else: + dataset_ = fo.load_dataset(source_name) + dataset = dataset_.clone(dataset_name, persistent=True)docs/source/getting_started_guides/manufacturing/02_embeddings.ipynb (1)
104-104: Guard the embedding dataset loadThis still crashes on the first run because
MVTec_AD_embdoes not exist yet. Please restore the existence check/clone logic that was previously requested so the notebook is self-contained.- dataset_emb = fo.load_dataset("MVTec_AD_emb") +emb_dataset_name = "MVTec_AD_emb" +if fo.dataset_exists(emb_dataset_name): + dataset_emb = fo.load_dataset(emb_dataset_name) +else: + dataset_emb = dataset.clone(emb_dataset_name, persistent=True)app/packages/operators/src/built-in-operators.ts (1)
1591-1605: Replace the fragile cross-origin fallback with an explicit errorThe simulated
<a target="_blank" download>fallback still fails in most browsers for cross-origin URLs (download is ignored, pop-up blockers intervene, or users just get a blank tab). Please surface a clear error instead of relying on this unreliable path.- } catch (error) { - // If fetch fails (CORS issues), fall back to direct link approach - console.warn("Direct fetch failed, falling back to direct link:", error); - - const downloadFilename = filename || url.split("/").pop() || "download"; - - const link = document.createElement("a"); - link.href = url; - link.download = downloadFilename; - link.target = "_blank"; - link.style.display = "none"; - - document.body.appendChild(link); - link.click(); - document.body.removeChild(link); - } + } catch (error) { + console.error("Failed to download file:", error); + throw new Error( + `Unable to download file from ${url}. This is likely due to CORS restrictions on the remote host.` + ); + }docs/source/getting_started_guides/manufacturing/03_clustering.ipynb (1)
134-134: Replace theembeddings=...placeholderLeaving the literal ellipsis still crashes
compute_visualization. Please supply the actual embeddings field name (e.g."mvtec_emb_nc") so the cell runs.docs/source/getting_started_guides/manufacturing/04_custom_embeddings.ipynb (1)
274-279: Fix embeddings field mismatch before running Brain.
compute_visualizationis still queryingembeddings="padin_emb", but the loop above only ever writes vectors tosample["embedding"]. FiftyOne will raise aKeyError, so PCA never runs. Point the visualization step at the field you actually created.compute_visualization( mvtec_bottle, - embeddings="padin_emb", + embeddings="embedding", brain_key="embedding_pca", method="pca", )fiftyone/server/routes/sample.py (1)
20-97: Resolve dynamic label deserialization.The hard-coded
LABEL_CLASS_MAPonly covers a handful of label types. Any dataset using other supported labels (Keypoint(s), Segmentation, Heatmap, temporal labels, custom subclasses, etc.) will now 400 with “Unsupported label class”, so PATCH is broken for them. Resolve by deriving the class fromfolat runtime and validating it’s aLabelsubclass.-LABEL_CLASS_MAP = { - "Classification": fol.Classification, - "Classifications": fol.Classifications, - "Detection": fol.Detection, - "Detections": fol.Detections, - "Polyline": fol.Polyline, - "Polylines": fol.Polylines, -} +from fiftyone.core.labels import Label + +def _resolve_label_class(cls_name: str) -> type[Label] | None: + label_cls = getattr(fol, cls_name, None) + if isinstance(label_cls, type) and issubclass(label_cls, Label): + return label_cls + return None @@ - if isinstance(value, dict) and "_cls" in value: - cls_name = value.get("_cls") - if cls_name in LABEL_CLASS_MAP: - label_cls = LABEL_CLASS_MAP[cls_name] - try: - sample[field_name] = label_cls.from_dict(value) - except Exception as e: - errors[field_name] = str(e) - else: - errors[ - field_name - ] = f"Unsupported label class '{cls_name}'" + if isinstance(value, dict) and "_cls" in value: + cls_name = value["_cls"] + label_cls = _resolve_label_class(cls_name) + if label_cls is None: + errors[field_name] = f"Unsupported label class '{cls_name}'" + continue + try: + sample[field_name] = label_cls.from_dict(value) + except Exception as e: + errors[field_name] = str(e)docs/source/getting_started_guides/manufacturing/11_video_analytics.ipynb (1)
341-375: Fix clip extraction to reference the new files and assign embeddings per sample.
create_clip_datasetmutates each original clip, briefly swapsclip.filepath, appends the same object, then restores the original path. The resulting dataset still points at the full-length videos, so the extracted clips are never referenced. On top of that,set_field(..., clip_view.values(...))broadcasts the entire embedding list to every sample instead of assigning per-clip values. Instantiate a freshfo.Samplefor each extracted clip, set its filepath toout_path, copy over any metadata you need, and useset_values(or assign during construction) to persist the corresponding embedding.- clip.filepath = out_path - samples.append(clip) - clip.filepath = fpath + samples.append( + fo.Sample( + filepath=out_path, + tags=list(clip.tags), + metadata=clip.metadata, + ground_truth=clip["ground_truth"], + **{ + "Twelve Labs Marengo-retrieval-27 embedding": + clip["Twelve Labs Marengo-retrieval-27"]["embedding"], + }, + ) + ) @@ - clip_dataset.add_samples(samples) - clip_dataset.add_sample_field("Twelve Labs Marengo-retrieval-27 Embeddings", fo.VectorField) - clip_dataset.set_field("Twelve Labs Marengo-retrieval-27 Embeddings", clip_view.values("Twelve Labs Marengo-retrieval-27.embedding")) + clip_dataset.add_samples(samples)(Alternatively, collect embeddings in a list aligned with
samplesand callclip_dataset.set_values(...).)install.sh (2)
89-90: Stop piping unverified scripts intosh
curl ... | shexecutes unauthenticated code from the network. Download to a temp file, verify integrity (checksum/signature), then execute; otherwise this is a critical supply-chain risk.- curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | sh + NVM_INSTALL_SCRIPT="$(mktemp)" + curl -fL https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh -o "$NVM_INSTALL_SCRIPT" + # TODO: verify checksum/signature before executing + sh "$NVM_INSTALL_SCRIPT" + rm -f "$NVM_INSTALL_SCRIPT"
140-151: Verify MongoDB download integrityWe still fetch tarballs without validating SHA256 (or signatures). Any MITM corrupts the binary silently. Please add vendor-provided checksum verification before untarring.
- curl "https://fastdl.mongodb.org/linux/${MONGODB_BUILD}.tgz" --output mongodb.tgz - tar -zxvf mongodb.tgz + curl -fL "https://fastdl.mongodb.org/linux/${MONGODB_BUILD}.tgz" --output mongodb.tgz + curl -fL "https://fastdl.mongodb.org/linux/${MONGODB_BUILD}.tgz.sha256" --output mongodb.tgz.sha256 + if ! shasum -a 256 --check mongodb.tgz.sha256; then + echo "ERROR: MongoDB checksum mismatch" + rm -f mongodb.tgz mongodb.tgz.sha256 + exit 1 + fi + tar -zxvf mongodb.tgz + rm -f mongodb.tgz mongodb.tgz.sha256(Apply analogous logic on macOS.)
docs/source/getting_started_guides/manufacturing/09_vad_dataset.ipynb (1)
154-269: Drop the hardcoded App portEvery
fo.launch_app(..., port=5152, ...)risks collisions when the port is busy (and duplicates earlier feedback). Let FiftyOne choose the port or add retry logic.-session = fo.launch_app(dataset, port= 5152, auto=False) +session = fo.launch_app(dataset, auto=False) @@ -session = fo.launch_app(view, port=5152, auto=False) +session = fo.launch_app(view, auto=False) @@ -session = fo.launch_app(filter, port=5152, auto=False) +session = fo.launch_app(filter, auto=False)docs/source/getting_started_guides/manufacturing/07_3d_visualization.ipynb (1)
113-153: Use a PNG-capable loader formask_path
mask_pathpoints to PNG masks;tifffile.imread()rejects them (ValueError: not a TIFF file). Switch to PIL (or cv2) before converting to NumPy.-import tifffile +import tifffile +from PIL import Image @@ - mask = tifffile.imread(mask_path) + mask = np.array(Image.open(mask_path))docs/source/getting_started_guides/manufacturing/05_evaluation.ipynb (1)
428-457: Address theengine.modelattribute issue.This issue has been previously identified:
engine.predict(model=engine.model, ...)at line 457 relies onengine.modelwhich is never assigned. As per the earlier review, the function should accept amodelparameter.fiftyone/utils/labels.py (1)
1321-1323: Clear the target field when no polylines are producedIf the current run yields zero visible cuboids, we never write
camera_sample[out_field], so any previously storedfol.Polylinessurvives and consumers see stale geometry. Always overwrite the field—either with a newPolylinescontainer orNone—so each projection pass reflects the latest data.- if polylines: - camera_sample[out_field] = fol.Polylines(polylines=polylines) + if polylines: + camera_sample[out_field] = fol.Polylines(polylines=polylines) + else: + camera_sample[out_field] = Nonefiftyone/utils/utils3d.py (1)
593-596: Validate the intrinsics matrix shape exactlyThe current check only rejects matrices larger than 4×4, so shapes like 2×3 or 3×4 slip through and later math produces nonsense projections. Enforce the documented requirement of a 3×3 or 4×4 matrix before proceeding.
- camera_intrinsics = np.asarray(camera_intrinsics) - if camera_intrinsics.shape[0] > 4 or camera_intrinsics.shape[1] > 4: - raise ValueError("intrinsics must be 3x3 or 4x4") + camera_intrinsics = np.asarray(camera_intrinsics) + if camera_intrinsics.shape not in {(3, 3), (4, 4)}: + raise ValueError("intrinsics must be 3x3 or 4x4")fiftyone/core/dataset.py (1)
10185-10196: Guard unique-index creation for logicalidkeysWe still create a unique index on the logical
"id"field. In our collections that field does not exist (only the Mongo_iddoes), so Mongo immediately raisesE11000 duplicate key error dup key: { id: null }, breakingmerge_samples(..., key_field="id")before the merge starts. Please derive the actual DB field name (e.g._id) and skip index creation when it’s already uniquely indexed. Otherwise, ensure the field exists before creating a unique index.app/packages/core/src/plugins/SchemaIO/components/DashboardView.tsx (1)
1428-1439: Still need thewindowguard from the earlier review.
windowis referenced unconditionally here, so the original SSR/runtime crash that coderabbitai[bot] flagged remains. Please add thetypeof window !== "undefined"guard as previously suggested before dereferencingwindow.Plotly.
🧹 Nitpick comments (9)
app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx (1)
23-48: Approve async pattern for consistency; consider error handling.The change to use
await snapshot.getPromise(fos.groupByFieldValue)makes the code consistent with theviewfield retrieval pattern on line 36. This async approach within a selector callback is appropriate and avoids the flashlight reset issues mentioned in previous learnings (which were specific to usinguseRecoilValuein component scope).However, consider adding error handling for potential Promise rejections:
return getCallback( ({ snapshot }) => async (page: number, pageSize: number) => { + try { const params = { dataset, view: await snapshot.getPromise(fos.view), }; return { ...params, dynamicGroup: await snapshot.getPromise(fos.groupByFieldValue), filter: {}, after: page ? String(page * pageSize - 1) : null, count: pageSize, }; + } catch (error) { + console.error("Failed to fetch pagination parameters:", error); + throw error; + } } );Based on learnings: The async pattern used here differs from the problematic
useRecoilValueapproach that caused flashlight resets, so this should be safe.fiftyone/utils/ultralytics.py (1)
669-674: Good defensive coding with the hasattr check.The addition of
hasattr(self._model.predictor, "_legacy_transform_name")properly handles cases where the Ultralytics predictor may not have this attribute, preventing AttributeError exceptions.For consistency with this defensive pattern, consider also verifying that
self.transformsand itstransformsattribute exist before accessing them on line 673:is_legacy_transform = False if hasattr(self._model.predictor, "_legacy_transform_name"): + if hasattr(self, "transforms") and hasattr(self.transforms, "transforms"): is_legacy_transform = any( self._model.predictor._legacy_transform_name in str(transform) for transform in self.transforms.transforms )app/packages/looker/src/elements/common/tags.ts (1)
275-292: Consider a more user-friendly default for missing labels.The implementation correctly defaults missing labels to
"null", which ensures consistency. However, displaying the literal string "null" to users might not provide the best experience, especially in a data visualization context where missing labels could be common during the labeling workflow.Consider these alternatives:
"(unlabeled)"or"(no label)"- more descriptive for users""(empty string) - minimal but might cause layout issues- A configurable default via application settings
Additionally, be aware that this approach could create ambiguity if an actual label named "null" exists in the dataset.
- const label = param.label ?? "null"; + const label = param.label ?? "(unlabeled)";That said, if "null" is an intentional choice to clearly indicate missing data (common in data tooling), the current implementation is acceptable.
fiftyone/utils/sam.py (1)
396-409: Consider adding input validation to prevent confusing errors.The function assumes
boxesis a 2D numpy array with 4 columns. If the input doesn't match this shape, the function will fail with unclear error messages.Add a shape check at the beginning of the function:
def _to_abs_boxes(boxes, img_width, img_height, chunk_size=1e6): boxes_xyxy = np.copy(boxes) num_boxes = len(boxes) + + if boxes.ndim != 2 or boxes.shape[1] != 4: + raise ValueError( + f"Expected boxes to be a 2D array with 4 columns, got shape {boxes.shape}" + ) for start in range(0, num_boxes, int(chunk_size)):app/packages/operators/src/types-internal.ts (1)
21-21: Consider usingunknowninstead ofany.The
ParamsTypeusesanywhich disables type checking. Consider usingRecord<string, unknown>instead to maintain type safety while still allowing flexibility. Callers would need to validate or assert types before use, which is a safer pattern.Apply this diff:
-export type ParamsType = Record<string, any>; +export type ParamsType = Record<string, unknown>;app/packages/operators/src/usePanelEvent.ts (1)
177-200: Clarify redundantpanelIdin options object.The
useTriggerPanelEventhook includespanelIdin the options object (line 193), but this is redundant sincehandleEventreceivespanelIdas its first parameter (line 188) and uses that parameter value instead of the options field. While not a bug, this could be confusing.Consider removing the redundant field for clarity:
handleEvent(panelId, { operator: event, params, prompt, callback, - panelId, });Or add a comment explaining that the panelId in options is for reference only.
docs/source/getting_started_guides/manufacturing/08_extended_exploration.ipynb (1)
192-223: Clarify the dual mask processing logic.The function processes segmentation masks in two separate passes:
- Lines 192-197: For
badsamples intest_public, finds and adds masks- Lines 201-223: Separately walks
ground_truth/badfolder to process all masksWhile the second pass attempts to match and update existing samples (line 214), the relationship between these two approaches could be clearer. Consider adding comments explaining:
- Why masks are processed in two passes
- What scenarios each pass handles
- Whether the second pass is meant to catch masks missed by the first
This would improve code maintainability and help future contributors understand the intent.
docs/source/getting_started_guides/manufacturing/05_evaluation.ipynb (2)
250-274: Consider clarifying the dataset cloning logic.The code creates two datasets sequentially, where
mvtec-ad_5is used to createmvtec-bottle_2. While functionally correct, adding comments explaining this dependency chain would improve readability.For example:
# Define the new dataset name dataset_name = "mvtec-ad_5" +# Step 1: Ensure the full MVTec-AD dataset exists # Check if the dataset exists if dataset_name in fo.list_datasets(): print(f"Dataset '{dataset_name}' exists. Loading...") dataset = fo.load_dataset(dataset_name) else: print(f"Dataset '{dataset_name}' does not exist. Creating a new one...") # Clone the dataset with a new name and make it persistent dataset = dataset_.clone(dataset_name, persistent=True) +# Step 2: Create bottle-specific subset from the full dataset # Define the new dataset name for split set dataset_name_split = "mvtec-bottle_2"
483-508: Remove redundant save() call.The
sample.save()call at line 508 appears redundant since the sample was already saved at line 483, and no additional modifications tosampleoccur between these calls.Apply this diff:
sample.save() # ----- Heat Map Slice ----- heat = heat_map.copy() if heat.dtype != np.uint8: heat = (heat * 255).astype(np.uint8) heat_path = os.path.join(output_dir, f"{group_id}_heat.png") cv2.imwrite(heat_path, heat) sample_heat = fo.Sample(filepath=heat_path, group=group.element("heat_map")) - - sample.save()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
app/package.jsonis excluded by!**/*.jsonapp/packages/aggregations/package.jsonis excluded by!**/*.jsonapp/packages/app/package.jsonis excluded by!**/*.jsonapp/packages/components/package.jsonis excluded by!**/*.jsonapp/packages/core/package.jsonis excluded by!**/*.jsonapp/packages/embeddings/package.jsonis excluded by!**/*.jsonapp/packages/flashlight/package.jsonis excluded by!**/*.jsonapp/packages/looker-3d/package.jsonis excluded by!**/*.jsonapp/packages/looker/package.jsonis excluded by!**/*.jsonapp/packages/map/package.jsonis excluded by!**/*.jsonapp/packages/operators/package.jsonis excluded by!**/*.jsonapp/packages/playback/package.jsonis excluded by!**/*.jsonapp/packages/plugins/package.jsonis excluded by!**/*.jsonapp/packages/relay/package.jsonis excluded by!**/*.jsonapp/packages/spaces/package.jsonis excluded by!**/*.jsonapp/packages/spotlight/package.jsonis excluded by!**/*.jsonapp/packages/state/package.jsonis excluded by!**/*.jsonapp/packages/utilities/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.locke2e-pw/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lockfiftyone/zoo/models/manifest-tf.jsonis excluded by!**/*.jsonfiftyone/zoo/models/manifest-torch.jsonis excluded by!**/*.json
📒 Files selected for processing (124)
.github/CODEOWNERS(1 hunks).github/workflows/build-db.yml(3 hunks).github/workflows/build-docker-internal.yml(1 hunks).github/workflows/build-docs.yml(4 hunks).github/workflows/build-graphql.yml(2 hunks).github/workflows/build.yml(2 hunks).github/workflows/e2e.yml(2 hunks).github/workflows/lint-app.yml(2 hunks).github/workflows/pr.yml(1 hunks).github/workflows/publish.yml(2 hunks).github/workflows/push-release.yml(1 hunks).github/workflows/test.yml(2 hunks).github/workflows/windows-test.yml(1 hunks)README.md(1 hunks)app/packages/app/src/Sync.tsx(2 hunks)app/packages/app/src/components/Analytics.tsx(1 hunks)app/packages/app/src/components/AnalyticsConsent.tsx(1 hunks)app/packages/app/src/components/Nav.tsx(1 hunks)app/packages/app/src/components/Setup.tsx(1 hunks)app/packages/app/src/components/Teams.tsx(1 hunks)app/packages/app/src/pages/datasets/DatasetPage.tsx(1 hunks)app/packages/app/src/routing/RouterContext.ts(2 hunks)app/packages/app/src/useSetters/onSetFieldVisibilityStage.ts(1 hunks)app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx(2 hunks)app/packages/components/src/components/ErrorBoundary/index.ts(1 hunks)app/packages/components/src/components/index.ts(1 hunks)app/packages/core/src/components/Grid/Grid.tsx(1 hunks)app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DashboardPNGExport.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DashboardResizeHandles.css(1 hunks)app/packages/core/src/plugins/SchemaIO/components/DashboardView.tsx(13 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/evaluation/overview/index.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsx(3 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx(1 hunks)app/packages/embeddings/src/usePlotSelection.tsx(1 hunks)app/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsx(3 hunks)app/packages/looker-3d/src/hooks/use-fo-loaders.ts(2 hunks)app/packages/looker/src/elements/common/tags.ts(1 hunks)app/packages/looker/src/overlays/classifications.ts(1 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/operators/src/built-in-operators.ts(2 hunks)app/packages/operators/src/index.ts(1 hunks)app/packages/operators/src/types-internal.ts(1 hunks)app/packages/operators/src/usePanelEvent.ts(4 hunks)docs/README.md(2 hunks)docs/source/cli/index.rst(5 hunks)docs/source/dataset_zoo/datasets.rst(1 hunks)docs/source/dataset_zoo/remote.rst(3 hunks)docs/source/deprecation.rst(3 hunks)docs/source/getting_started_guides/manufacturing/01_intro.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/02_embeddings.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/03_clustering.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/04_custom_embeddings.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/05_evaluation.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/06_augmentation.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/07_3d_visualization.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/08_extended_exploration.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/09_vad_dataset.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/10_ppe_detection.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/11_video_analytics.ipynb(1 hunks)docs/source/getting_started_guides/manufacturing/index.rst(1 hunks)docs/source/getting_started_guides/manufacturing/summary.rst(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/01_intro.ipynb(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/02_explore.ipynb(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/index.rst(1 hunks)docs/source/getting_started_guides/model_dataset_zoo/summary.rst(1 hunks)docs/source/getting_started_guides/segmentation/01_intro.ipynb(1 hunks)docs/source/getting_started_guides/segmentation/02_explore.ipynb(1 hunks)docs/source/getting_started_guides/segmentation/03_sam2.ipynb(1 hunks)docs/source/getting_started_guides/segmentation/index.rst(1 hunks)docs/source/getting_started_guides/segmentation/summary.rst(1 hunks)docs/source/integrations/activitynet.rst(1 hunks)docs/source/integrations/coco.rst(1 hunks)docs/source/integrations/open_images.rst(1 hunks)docs/source/plugins/developing_plugins.rst(2 hunks)docs/source/release-notes.rst(1 hunks)docs/source/user_guide/config.rst(1 hunks)docs/source/user_guide/evaluation.rst(3 hunks)docs/source/user_guide/using_datasets.rst(2 hunks)e2e-pw/src/oss/poms/field-visibility/field-visibility.ts(1 hunks)e2e-pw/src/oss/poms/sidebar.ts(1 hunks)e2e-pw/src/oss/specs/smoke-tests/field-visibility.spec.ts(4 hunks)e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts(2 hunks)fiftyone/core/cli.py(12 hunks)fiftyone/core/collections.py(18 hunks)fiftyone/core/config.py(1 hunks)fiftyone/core/dataset.py(7 hunks)fiftyone/core/metadata.py(13 hunks)fiftyone/core/models.py(22 hunks)fiftyone/core/odm/__init__.py(1 hunks)fiftyone/core/odm/database.py(3 hunks)fiftyone/core/sample.py(1 hunks)fiftyone/core/stages.py(15 hunks)fiftyone/core/utils.py(1 hunks)fiftyone/core/validation.py(1 hunks)fiftyone/factory/repos/delegated_operation.py(5 hunks)fiftyone/factory/repos/delegated_operation_doc.py(5 hunks)fiftyone/operators/__init__.py(1 hunks)fiftyone/operators/_types/__init__.py(1 hunks)fiftyone/operators/_types/pipeline.py(1 hunks)fiftyone/operators/delegated.py(11 hunks)fiftyone/operators/executor.py(9 hunks)fiftyone/operators/operations.py(1 hunks)fiftyone/operators/operator.py(2 hunks)fiftyone/operators/panel.py(1 hunks)fiftyone/server/app.py(1 hunks)fiftyone/server/decorators.py(3 hunks)fiftyone/server/routes/__init__.py(2 hunks)fiftyone/server/routes/sample.py(1 hunks)fiftyone/utils/caltech.py(1 hunks)fiftyone/utils/eval/base.py(2 hunks)fiftyone/utils/eval/classification.py(2 hunks)fiftyone/utils/labels.py(12 hunks)fiftyone/utils/sam.py(4 hunks)fiftyone/utils/sam2.py(3 hunks)fiftyone/utils/torch.py(2 hunks)fiftyone/utils/ultralytics.py(1 hunks)fiftyone/utils/utils3d.py(3 hunks)fiftyone/zoo/datasets/base.py(4 hunks)fiftyone/zoo/models/__init__.py(1 hunks)install.bash(0 hunks)install.sh(1 hunks)
💤 Files with no reviewable changes (1)
- install.bash
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/app/src/components/Teams.tsxapp/packages/core/src/components/Grid/Grid.tsxapp/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/evaluation/overview/index.tsxe2e-pw/src/oss/poms/field-visibility/field-visibility.tsapp/packages/looker/src/elements/common/tags.tsapp/packages/app/src/components/Nav.tsxapp/packages/looker-3d/src/hooks/use-fo-loaders.tsapp/packages/operators/src/index.tsapp/packages/looker/src/overlays/detection.tsapp/packages/app/src/pages/datasets/DatasetPage.tsxe2e-pw/src/oss/specs/smoke-tests/quickstart.spec.tsapp/packages/app/src/useSetters/onSetFieldVisibilityStage.tse2e-pw/src/oss/poms/sidebar.tsapp/packages/looker-3d/src/fo3d/MediaTypeFo3d.tsxapp/packages/app/src/components/Setup.tsxapp/packages/embeddings/src/usePlotSelection.tsxapp/packages/components/src/components/ErrorBoundary/index.tsapp/packages/core/src/plugins/SchemaIO/components/DashboardPNGExport.tsxapp/packages/operators/src/types-internal.tsapp/packages/operators/src/usePanelEvent.tsapp/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsxapp/packages/app/src/Sync.tsxapp/packages/looker/src/overlays/classifications.tsapp/packages/app/src/components/AnalyticsConsent.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/index.tsxapp/packages/app/src/routing/RouterContext.tsapp/packages/app/src/components/Analytics.tsxe2e-pw/src/oss/specs/smoke-tests/field-visibility.spec.tsapp/packages/components/src/components/index.tsapp/packages/core/src/plugins/SchemaIO/components/DashboardView.tsxapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.tsapp/packages/operators/src/built-in-operators.tsapp/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx
🧠 Learnings (10)
📚 Learning: 2024-09-27T23:14:11.193Z
Learnt from: sashankaryal
PR: voxel51/fiftyone#4863
File: app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx:39-48
Timestamp: 2024-09-27T23:14:11.193Z
Learning: In `DynamicGroupCarousel`, using `useRecoilValue(fos.groupByFieldValue)` causes the flashlight to behave unexpectedly—specifically, it resets to the front when scrolling the carousel and selecting samples. Therefore, `setInterval` is currently used as a workaround to update `groupByValue`.
Applied to files:
app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx
📚 Learning: 2024-09-27T23:14:46.454Z
Learnt from: sashankaryal
PR: voxel51/fiftyone#4863
File: app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx:74-74
Timestamp: 2024-09-27T23:14:46.454Z
Learning: In `DynamicGroupCarousel`, setting the `key` prop on `DynamicGroupsFlashlightWrapper` to `groupByValue` is intentional to force a remount when `groupByValue` changes.
Applied to files:
app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx
📚 Learning: 2025-10-16T17:24:11.562Z
Learnt from: brimoor
PR: voxel51/fiftyone#6373
File: docs/source/dataset_zoo/datasets/ucf101.rst:32-0
Timestamp: 2025-10-16T17:24:11.562Z
Learning: In FiftyOne documentation, dataset zoo classes follow the pattern `fiftyone.zoo.datasets.base.{DatasetName}Dataset` and should be referenced using `:class:` directives (e.g., `:class:`UCF101Dataset <fiftyone.zoo.datasets.base.UCF101Dataset>``). This is a valid and established documentation pattern.
Applied to files:
docs/source/getting_started_guides/model_dataset_zoo/index.rstdocs/source/getting_started_guides/model_dataset_zoo/01_intro.ipynb
📚 Learning: 2025-09-12T15:01:34.588Z
Learnt from: afoley587
PR: voxel51/fiftyone#6339
File: .github/workflows/build-db.yml:112-116
Timestamp: 2025-09-12T15:01:34.588Z
Learning: For GitHub Actions workflows using pypa/gh-action-pypi-publish with trusted publishing: only `contents: read` and `id-token: write` permissions are required. `actions: read` is only needed when downloading artifacts from other workflow runs/repositories (not the same workflow run). `attestations: write` is only needed if uploading attestations to GitHub's attestation store, not for standard PyPI publishing which uploads attestations to PyPI automatically.
Applied to files:
.github/workflows/publish.yml.github/workflows/build-db.yml
📚 Learning: 2025-01-30T16:40:26.003Z
Learnt from: swheaton
PR: voxel51/fiftyone#5448
File: fiftyone/factory/repos/delegated_operation_doc.py:0-0
Timestamp: 2025-01-30T16:40:26.003Z
Learning: In FiftyOne's DelegatedOperationDocument class, all instance attributes are automatically serialized through `__dict__` in the `to_pymongo()` method, eliminating the need for explicit field inclusion in serialization.
Applied to files:
fiftyone/factory/repos/delegated_operation_doc.py
📚 Learning: 2025-06-16T21:08:14.104Z
Learnt from: swheaton
PR: voxel51/fiftyone#6023
File: tests/unittests/factory/delegated_operation_doc_tests.py:55-57
Timestamp: 2025-06-16T21:08:14.104Z
Learning: The DelegatedOperationDocument class in fiftyone/factory/repos/delegated_operation_doc.py inherits from object (plain Python class), not from Pydantic BaseModel. Using setattr to modify its attributes in tests is appropriate and does not bypass any validation.
Applied to files:
fiftyone/factory/repos/delegated_operation_doc.py
📚 Learning: 2025-06-16T21:07:49.946Z
Learnt from: swheaton
PR: voxel51/fiftyone#6023
File: fiftyone/operators/executor.py:1326-1332
Timestamp: 2025-06-16T21:07:49.946Z
Learning: In fiftyone/operators/executor.py, the ExecutionOptions.__init__ method intentionally ignores the allow_distributed_execution parameter by hardcoding _allow_distributed_execution to False. This is deliberate scaffolding behavior to maintain API compatibility while reserving distributed execution functionality for FiftyOne Enterprise only.
Applied to files:
fiftyone/operators/executor.py
📚 Learning: 2025-09-10T03:01:27.501Z
Learnt from: swheaton
PR: voxel51/fiftyone#6334
File: fiftyone/core/expressions.py:2727-2741
Timestamp: 2025-09-10T03:01:27.501Z
Learning: FiftyOne requires MongoDB >= 6.0 as documented at https://docs.voxel51.com/user_guide/config.html#using-a-different-mongodb-version
Applied to files:
docs/source/deprecation.rst
📚 Learning: 2025-09-30T02:34:55.348Z
Learnt from: brimoor
PR: voxel51/fiftyone#6365
File: fiftyone/core/dataset.py:10084-10087
Timestamp: 2025-09-30T02:34:55.348Z
Learning: MongoDB $merge requires a non-partial unique index that fully covers the target collection’s “on” fields; a partial unique index will not satisfy this requirement and causes errors. Avoid recommending partial unique indexes for $merge.
Applied to files:
fiftyone/core/dataset.py
📚 Learning: 2025-09-10T19:53:08.432Z
Learnt from: AdonaiVera
PR: voxel51/fiftyone#6337
File: docs/source/tutorials/dinov3.ipynb:121-124
Timestamp: 2025-09-10T19:53:08.432Z
Learning: The `foz.load_zoo_dataset()` function supports loading datasets from external sources including GitHub URLs (e.g., "https://github.com/voxel51/coco-2017"), which is different from loading built-in zoo datasets by name (e.g., "coco-2017"). GitHub URL loading allows access to custom datasets not available in the standard FiftyOne Dataset Zoo.
Applied to files:
docs/source/getting_started_guides/model_dataset_zoo/01_intro.ipynb
| uses: actions/checkout@v5 | ||
|
|
||
| - uses: actions/checkout@v4 | ||
| - uses: actions/checkout@v5 |
There was a problem hiding this comment.
Remove duplicate checkout step.
Lines 10 and 12 both perform actions/checkout@v5, which is redundant and wasteful. Remove the duplicate at line 12.
Apply this diff to remove the duplicate:
- name: Checkout repository
uses: actions/checkout@v5
- - uses: actions/checkout@v5
- uses: dorny/paths-filter@v3📝 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.
| uses: actions/checkout@v5 | |
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v5 | |
| uses: actions/checkout@v5 | |
| - uses: dorny/paths-filter@v3 |
🤖 Prompt for AI Agents
.github/workflows/lint-app.yml around lines 10 to 12 contains a duplicate
actions/checkout@v5 step; remove the second occurrence (the one at line 12) so
only a single checkout action remains, keeping surrounding indentation and job
structure intact.
| attach ?? | ||
| getEntryResource({ | ||
| environment, | ||
| routes, | ||
| location: location as FiftyOneLocation, | ||
| hard: location.state.event !== "modal", | ||
| handleError, | ||
| }); | ||
| } catch (e) { |
There was a problem hiding this comment.
Restore reuse for modal/slice/spaces transitions
Switching the hard flag to location.state.event !== "modal" prevents getEntryResource() from ever taking the reuse path unless the incoming event is exactly "modal". As a result, closing a modal (next event becomes undefined) or dispatching the "slice"/"spaces" events now sets hard = true, so we bypass isReusable even though those events are in SKIP_EVENTS specifically to keep the existing page/resource alive. This regression forces a full reload and wipes the cached entry/state on the very interactions we were trying to make seamless. Please keep hard false whenever either the current or next entry event is one of the skip events (modal, slice, spaces), or otherwise restore the previous behaviour so we still reuse the resource in those flows.
🤖 Prompt for AI Agents
In app/packages/app/src/routing/RouterContext.ts around lines 91 to 99, the
`hard` flag is currently set to `location.state.event !== "modal"`, which
prevents reuse for modal/slice/spaces flows; change it to compute the event(s)
and only set `hard` true when neither the current nor the next entry event is in
SKIP_EVENTS (e.g. modal, slice, spaces). Concretely, derive currentEvent and
nextEvent from location/state/nextEntry (as applicable), check if either is in
SKIP_EVENTS, and set `hard = !([currentEvent, nextEvent].some(e =>
SKIP_EVENTS.has(e)))` so getEntryResource can take the reuse path for
modal/slice/spaces transitions.
| messages = error.errors.map((e: any) => ({ | ||
| message: e.message, | ||
| content: "\n\n" + e.extensions.stack.join("\n"), | ||
| })); |
There was a problem hiding this comment.
Type safety: Avoid any for GraphQL errors.
The GraphQL error mapping uses any type (line 61) and accesses nested properties (e.extensions.stack) without validation. This could crash if the error structure differs from expectations.
Consider adding type guards or null checks:
if (error instanceof GraphQLError) {
- messages = error.errors.map((e: any) => ({
+ messages = error.errors.map((e) => ({
message: e.message,
- content: "\n\n" + e.extensions.stack.join("\n"),
+ content: e.extensions?.stack ? "\n\n" + e.extensions.stack.join("\n") : "",
}));
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx around
lines 61 to 64, the mapping uses the unsafe any type and directly accesses
e.extensions.stack which can crash for unexpected GraphQL error shapes; replace
any with a typed interface for GraphQLError (with optional extensions and
stack), add type guards/optional chaining and runtime checks (e.g., ensure
error.errors is an array, check typeof e.message === "string" and
Array.isArray(e.extensions?.stack)) and provide safe fallbacks (default message
and empty content or joined stack only when present) so the code never assumes
nested properties exist.
| useLayoutEffect(() => { | ||
| clearModal(); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add clearModal to useLayoutEffect dependencies.
The useLayoutEffect at line 141 has an empty dependency array but references clearModal. While this runs only on mount (likely the intent), React will warn about the missing dependency. Add clearModal to the deps array to follow React best practices, or add an eslint-disable comment if the empty array is intentional.
useLayoutEffect(() => {
clearModal();
- }, []);
+ }, [clearModal]);📝 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.
| useLayoutEffect(() => { | |
| clearModal(); | |
| }, []); | |
| useLayoutEffect(() => { | |
| clearModal(); | |
| }, [clearModal]); |
🤖 Prompt for AI Agents
In app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx around
lines 141 to 143, the useLayoutEffect calls clearModal but uses an empty
dependency array which triggers a React/ESLint warning; update the dependency
array to include clearModal (i.e., [clearModal]) so the hook follows React best
practices, or if the intent is to run only on mount and you want to suppress the
rule, add a one-line eslint-disable-next-line react-hooks/exhaustive-deps
comment above the useLayoutEffect; choose one approach and apply it
consistently.
| useEffect(() => { | ||
| trackEvent("uncaught_app_error", { | ||
| error: props?.error?.message || props?.error?.name || props?.error, | ||
| stack: props?.error?.stack, | ||
| messages: props?.error?.errors?.map((e: any) => e.message), | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Add null safety for nested error property access.
Lines 170-172 access props?.error?.errors?.map() but the optional chaining doesn't prevent the error if errors is undefined. The map call should be guarded.
useEffect(() => {
trackEvent("uncaught_app_error", {
error: props?.error?.message || props?.error?.name || props?.error,
stack: props?.error?.stack,
- messages: props?.error?.errors?.map((e: any) => e.message),
+ messages: props?.error?.errors?.map((e: any) => e.message) ?? [],
});
}, []);📝 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.
| useEffect(() => { | |
| trackEvent("uncaught_app_error", { | |
| error: props?.error?.message || props?.error?.name || props?.error, | |
| stack: props?.error?.stack, | |
| messages: props?.error?.errors?.map((e: any) => e.message), | |
| }); | |
| }, []); | |
| useEffect(() => { | |
| trackEvent("uncaught_app_error", { | |
| error: props?.error?.message || props?.error?.name || props?.error, | |
| stack: props?.error?.stack, | |
| messages: props?.error?.errors?.map((e: any) => e.message) ?? [], | |
| }); | |
| }, []); |
🤖 Prompt for AI Agents
In app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx around
lines 168 to 174, the code calls props?.error?.errors?.map(...) which can still
throw if errors is undefined or not an array; guard the map by first verifying
errors is an array (e.g., const messages = Array.isArray(props?.error?.errors) ?
props.error.errors.map((e:any)=>e?.message) : undefined or an empty array) and
pass that safe messages value into trackEvent; ensure you handle null/undefined
so map is never invoked on a non-array.
| to_db_name = lambda spec: "_".join("%s_%s" % (f, o) for f, o in spec) | ||
| normalize = lambda name: name.replace("-1", "1") | ||
|
|
||
| is_frame_index = all(is_frame_fields) | ||
| db_name = to_db_name(index_spec) | ||
|
|
||
| if single_field_index: | ||
| if len(input_spec) == 1: | ||
| # We use field name, not pymongo name, for single field indexes | ||
| index_name = input_spec[0][0] | ||
| else: | ||
| index_name = "_".join("%s_%s" % (f, o) for f, o in index_spec) | ||
|
|
||
| if is_frame_index: | ||
| index_name = self._FRAMES_PREFIX + index_name | ||
|
|
||
| normalize = lambda name: name.replace("-1", "1") | ||
| _index_name = normalize(index_name) | ||
| if any(_index_name == normalize(name) for name in index_info.keys()): | ||
| # Satisfactory index already exists | ||
| return index_name | ||
| index_name = db_name | ||
| if is_frame_index: | ||
| index_name = self._FRAMES_PREFIX + index_name | ||
|
|
||
| # Setting `w=0` sets `acknowledged=False` in pymongo | ||
| write_concern = None if wait else WriteConcern(w=0) | ||
| _index_name, _ = self._handle_frame_field(index_name) | ||
|
|
||
| if is_frame_index: | ||
| coll = self._dataset._get_frame_collection( | ||
| write_concern=write_concern | ||
| ) | ||
| coll = self._dataset._frame_collection | ||
| coll_name = self._dataset._frame_collection_name | ||
| else: | ||
| coll = self._dataset._get_sample_collection( | ||
| write_concern=write_concern | ||
| coll = self._dataset._sample_collection | ||
| coll_name = self._dataset._sample_collection_name | ||
|
|
||
| # Check for existing indexes | ||
| _existing_name = None | ||
| _existing_db_name = None | ||
| _existing_unique = None | ||
| for _name, _info in self.get_index_information().items(): | ||
| if normalize(_name) == normalize(index_name): | ||
| _existing_name = _name | ||
| _existing_db_name = to_db_name(_info["key"]) | ||
| _existing_unique = _info.get("unique", False) | ||
|
|
There was a problem hiding this comment.
Normalization step breaks legitimate index names
Blindly doing name.replace("-1", "1") corrupts real index names whenever a field itself contains the substring "-1" (for example a field called "feature-1" yields an index name "feature-1_1", which is transformed into "feature1_1"). That causes us to misidentify existing indexes, so we may silently reuse/drop the wrong index or fail to replace the intended one. Please compare the key specs without mutating the strings. One way is to compare just the field components of each spec:
- to_db_name = lambda spec: "_".join("%s_%s" % (f, o) for f, o in spec)
- normalize = lambda name: name.replace("-1", "1")
+ to_db_name = lambda spec: "_".join("%s_%s" % (f, o) for f, o in spec)
+ fields_only = lambda spec: tuple(f for f, _ in spec)
@@
- for _name, _info in self.get_index_information().items():
- if normalize(_name) == normalize(index_name):
+ for _name, _info in self.get_index_information().items():
+ if fields_only(_info["key"]) == fields_only(index_spec):
_existing_name = _name
_existing_db_name = to_db_name(_info["key"])
_existing_unique = _info.get("unique", False)This keeps the ability to detect differing sort orders while preserving the actual field names.
| def _execute_operator_in_child_process( | ||
| operation_id, log=False, log_queue=None | ||
| ): | ||
| """Worker function to be run in a separate 'spawned' process. | ||
| This function receives a simple ID, not a complex object, to avoid | ||
| serialization (pickling) errors. It instantiates its own service to create | ||
| fresh resources and then fetches the operation document from the database. | ||
| Args: | ||
| operation_id: the string ID of the operation to execute | ||
| log (False): the optional boolean flag to log the execution | ||
| log_queue (None): a multiprocessing queue to send log records to | ||
| """ | ||
| # On POSIX systems, become the session leader to take control of any | ||
| # subprocesses. This allows the parent to terminate the entire process | ||
| # group reliably. | ||
| if hasattr(os, "setsid"): | ||
| try: | ||
| os.setsid() | ||
| except Exception: | ||
| pass | ||
|
|
||
| if log_queue: | ||
| _configure_child_logging(log_queue) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| service = DelegatedOperationService() | ||
| result = None | ||
|
|
||
| try: | ||
| operation = service.get(operation_id) | ||
| if not operation: | ||
| logger.error( | ||
| "Operation %s not found in child process. Aborting.", | ||
| operation_id, | ||
| ) | ||
| return | ||
| if log: | ||
| logger.info( | ||
| "\nRunning operation %s (%s) in child process", | ||
| operation.id, | ||
| operation.operator, | ||
| ) | ||
|
|
||
| result = asyncio.run(service._execute_operator(operation)) | ||
|
|
||
| service.set_completed(doc_id=operation.id, result=result) | ||
| if log: | ||
| logger.info("Operation %s complete", operation.id) | ||
| except Exception: | ||
| result = ExecutionResult(error=traceback.format_exc()) | ||
| service.set_failed(doc_id=operation_id, result=result) | ||
| if log: | ||
| logger.error("Operation %s failed\n%s", operation_id, result.error) | ||
|
|
There was a problem hiding this comment.
Consider handling "operation not found" scenario.
If the operation is not found at lines 76-81, the function logs an error and returns early without marking the operation as failed. While this is a defensive check, if the operation ID was valid when passed but is deleted before the child process fetches it, the parent process may be left waiting indefinitely for a result.
Consider whether this scenario is possible and if so, whether additional coordination is needed between parent and child.
🤖 Prompt for AI Agents
In fiftyone/operators/delegated.py around lines 44 to 99, the child process
currently logs and returns when the operation cannot be fetched (lines ~76-81),
which can leave the parent waiting; change this to mark the operation as failed
(call service.set_failed) with an ExecutionResult describing "operation not
found" (include operation_id and stack/context as appropriate) so the parent is
notified, and still log the event; ensure this code path is safe if the service
call itself fails by wrapping the set_failed call in a try/except and logging
any errors without raising.
| outputs.append( | ||
| { | ||
| "boxes": input_boxes, | ||
| "boxes": torch.tensor(boxes_xyxy), |
There was a problem hiding this comment.
Specify device when creating the boxes tensor.
The tensor is created without specifying a device, which can cause device mismatch errors when mixed with GPU tensors from sam_predictor.device.
Apply this diff to fix the device mismatch:
- "boxes": torch.tensor(boxes_xyxy),
+ "boxes": torch.tensor(boxes_xyxy, device=sam_predictor.device),📝 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.
| "boxes": torch.tensor(boxes_xyxy), | |
| "boxes": torch.tensor(boxes_xyxy, device=sam_predictor.device), |
🤖 Prompt for AI Agents
In fiftyone/utils/sam.py around line 284, the boxes tensor is created without a
device which can cause device-mismatch errors; update the tensor creation to
pass the predictor device (e.g., use torch.tensor(boxes_xyxy,
device=sam_predictor.device)) so the boxes tensor is allocated on the same
device as sam_predictor.
| box_xyxy = fosam._to_abs_boxes( | ||
| np.array([detection.bounding_box]), | ||
| self._curr_frame_width, | ||
| self._curr_frame_height, | ||
| chunk_size=1, | ||
| ) | ||
| box = np.round(box_xyxy.squeeze(axis=0)).astype(int) |
There was a problem hiding this comment.
Avoid chunking overhead for single-box conversion.
Calling _to_abs_boxes with chunk_size=1 for a single detection adds unnecessary chunking overhead. The chunking logic is designed for batches of boxes, not single instances.
Apply this diff to simplify the single-box conversion:
- box_xyxy = fosam._to_abs_boxes(
- np.array([detection.bounding_box]),
- self._curr_frame_width,
- self._curr_frame_height,
- chunk_size=1,
- )
- box = np.round(box_xyxy.squeeze(axis=0)).astype(int)
+ bbox = detection.bounding_box
+ x, y, w, h = bbox
+ x1 = x * self._curr_frame_width
+ y1 = y * self._curr_frame_height
+ x2 = (x + w) * self._curr_frame_width
+ y2 = (y + h) * self._curr_frame_height
+ box = np.round([x1, y1, x2, y2]).astype(int)Alternatively, if you prefer to reuse _to_abs_boxes, remove the unnecessary chunk_size parameter:
box_xyxy = fosam._to_abs_boxes(
np.array([detection.bounding_box]),
self._curr_frame_width,
self._curr_frame_height,
- chunk_size=1,
)The default chunk_size of 1e6 will handle a single box efficiently without the overhead of explicitly specifying chunk_size=1.
📝 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.
| box_xyxy = fosam._to_abs_boxes( | |
| np.array([detection.bounding_box]), | |
| self._curr_frame_width, | |
| self._curr_frame_height, | |
| chunk_size=1, | |
| ) | |
| box = np.round(box_xyxy.squeeze(axis=0)).astype(int) | |
| box_xyxy = fosam._to_abs_boxes( | |
| np.array([detection.bounding_box]), | |
| self._curr_frame_width, | |
| self._curr_frame_height, | |
| ) | |
| box = np.round(box_xyxy.squeeze(axis=0)).astype(int) |
🤖 Prompt for AI Agents
In fiftyone/utils/sam2.py around lines 401 to 407, the call to
fosam._to_abs_boxes passes chunk_size=1 which forces unnecessary chunking for a
single detection; remove the explicit chunk_size argument so the function uses
its default large chunk size (or replace with a direct scalar conversion for a
single box), then keep the existing rounding and squeeze/astype steps unchanged
to produce the int box coordinates.
| MONGODB_BUILD="mongodb-macos-x86_64-$MONGODB_VERSION" | ||
| curl "https://fastdl.mongodb.org/osx/${MONGODB_BUILD}.tgz" --output mongodb.tgz | ||
| tar -zxvf mongodb.tgz | ||
| mv "${MONGODB_BUILD}/bin/"* ./bin/ | ||
| rm -rf mongodb.tgz "${MONGODB_BUILD}" | ||
| elif [ "$OS" = "Linux" ]; then | ||
| MONGODB_BUILD="mongodb-linux-x86_64-ubuntu2204-$MONGODB_VERSION" | ||
| curl "https://fastdl.mongodb.org/linux/${MONGODB_BUILD}.tgz" --output mongodb.tgz | ||
| tar -zxvf mongodb.tgz | ||
| mv "${MONGODB_BUILD}/bin/"* ./bin/ | ||
| rm -rf mongodb.tgz "${MONGODB_BUILD}" |
There was a problem hiding this comment.
Handle Apple Silicon / non‑x86 architectures
Hard-coding the x86_64 build breaks on Apple Silicon and other architectures. Detect uname -m and pick the matching MongoDB distribution (or fail early with guidance).
- if [ "$OS" = "Darwin" ]; then
- MONGODB_BUILD="mongodb-macos-x86_64-$MONGODB_VERSION"
+ ARCH=$(uname -m)
+ if [ "$OS" = "Darwin" ]; then
+ if [ "$ARCH" = "arm64" ]; then
+ MONGODB_BUILD="mongodb-macos-arm64-$MONGODB_VERSION"
+ else
+ MONGODB_BUILD="mongodb-macos-x86_64-$MONGODB_VERSION"
+ fi
elif [ "$OS" = "Linux" ]; then
- MONGODB_BUILD="mongodb-linux-x86_64-ubuntu2204-$MONGODB_VERSION"
+ case "$ARCH" in
+ x86_64) MONGODB_BUILD="mongodb-linux-x86_64-ubuntu2204-$MONGODB_VERSION" ;;
+ aarch64) MONGODB_BUILD="mongodb-linux-aarch64-ubuntu2204-$MONGODB_VERSION" ;;
+ *) echo "ERROR: unsupported architecture $ARCH for MongoDB install"; exit 1 ;;
+ esacCommittable suggestion skipped: line range outside the PR's diff.
improve error details for when an operator returns non-serializable value
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx (3)
144-146: Add clearModal to useLayoutEffect dependencies.This issue was previously identified: The
useLayoutEffectreferencesclearModalbut has an empty dependency array, which will trigger a React/ESLint warning about missing dependencies.As suggested in the previous review, add
clearModalto the dependency array:useLayoutEffect(() => { clearModal(); - }, []); + }, [clearModal]);
61-64: Type safety: Avoidanyfor GraphQL errors.This issue was previously identified: The GraphQL error mapping uses
anytype and accesses nested properties (e.extensions.stack) without validation, which could crash if the error structure differs from expectations.As suggested in the previous review, add type guards or null checks:
if (error instanceof GraphQLError) { - messages = error.errors.map((e: any) => ({ + messages = error.errors.map((e) => ({ message: e.message, - content: "\n\n" + e.extensions.stack.join("\n"), + content: e.extensions?.stack ? "\n\n" + e.extensions.stack.join("\n") : "", })); }
171-177: Add null safety for nested error property access.This issue was previously identified: The code accesses
props?.error?.errors?.map()but the optional chaining doesn't prevent an error iferrorsis not an array.As suggested in the previous review, guard the map operation:
useEffect(() => { trackEvent("uncaught_app_error", { error: props?.error?.message || props?.error?.name || props?.error, stack: props?.error?.stack, - messages: props?.error?.errors?.map((e: any) => e.message), + messages: Array.isArray(props?.error?.errors) + ? props.error.errors.map((e: any) => e.message) + : [], }); }, []);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx(2 hunks)app/packages/operators/src/components/OperatorPromptOutput.tsx(2 hunks)fiftyone/core/utils.py(3 hunks)fiftyone/operators/server.py(2 hunks)fiftyone/operators/utils.py(2 hunks)fiftyone/server/decorators.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsxapp/packages/operators/src/components/OperatorPromptOutput.tsx
🔇 Additional comments (8)
fiftyone/operators/utils.py (1)
119-156: LGTM! Well-structured error handling and response serialization.The new
create_operator_responsefunction properly centralizes ExecutionResult serialization with appropriate error handling:
- Re-raises
HTTPExceptionunchanged- Logs non-HTTP exceptions and returns structured 500 error with stack trace
- Provides helpful error message guiding users toward JSON-serializable return values
fiftyone/core/utils.py (2)
3197-3215: LGTM! Clean numpy type handling and async response creation.The new
Encoderclass properly serializes numpy floating and integer types to native Python types, andcreate_responseappropriately delegates JSON serialization to a sync task to avoid blocking the async event loop.
2147-2158: LGTM! Robust conflict resolution for duplicate filenames.The enhanced logic properly handles existing filenames that use the
-<count>suffix pattern by incrementing the count until an available filename is found. This prevents collisions whenignore_existingis False.fiftyone/server/decorators.py (2)
23-24: Good documentation addition.The docstring clearly describes the decorator's purpose.
40-52: Verify error response structure consistency.The error response structure here includes
"kind"and"stack"fields, whilecreate_operator_responseinfiftyone/operators/utils.pyincludes"kind","error","message", and"stack"fields. Consider standardizing these error response structures for consistency across the codebase.fiftyone/operators/server.py (1)
110-110: Good refactor, but flag inconsistency in ExecuteOperatorAsGenerator.The change to use
create_operator_responsecentralizes error handling appropriately. However, note that line 177 in theExecuteOperatorAsGenerator.postmethod still usesresult.to_json()directly. For consistency and unified error handling, that should also usecreate_operator_response.app/packages/operators/src/components/OperatorPromptOutput.tsx (1)
13-13: Verify executor exists when outputFields is present.Line 13 accesses
operatorPrompt.executor.resultwithout null-checkingexecutor. While the guard on line 11 ensuresoutputFieldsis truthy before reaching this line, it doesn't verify thatexecutorexists.Please confirm that when
outputFieldsis present,executoris guaranteed to exist. If not, add a null check:- const { result } = operatorPrompt.executor; + const { result } = operatorPrompt.executor ?? {};Or move the guard to also check for executor:
- if (!outputFields && !executorError && !resolveError) return null; + if ((!outputFields || !operatorPrompt.executor) && !executorError && !resolveError) return null;app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx (1)
44-133: LGTM: Clean extraction of error display logic.The refactoring to extract
ErrorDisplayMarkupas a separate component is well-structured. The separation of concerns (rendering vs. side effects) and the typedErrorDisplayPropsinterface improve maintainability.Note: Please address the type safety issues flagged in lines 61-64 (already noted in previous reviews) to ensure this component handles unexpected error structures gracefully.
| const defaultMsg = "Error occurred during operator execution"; | ||
| const message = error?.bodyResponse?.error; | ||
| const reason = message ? defaultMsg + ". " + message : defaultMsg; |
There was a problem hiding this comment.
Add type safety for error message extraction.
Line 15 accesses error?.bodyResponse?.error without validating the type. If bodyResponse.error is an object rather than a string, the concatenation on line 16 will produce "Error occurred during operator execution. [object Object]".
Consider adding type validation:
const defaultMsg = "Error occurred during operator execution";
- const message = error?.bodyResponse?.error;
+ const message = typeof error?.bodyResponse?.error === "string"
+ ? error.bodyResponse.error
+ : undefined;
const reason = message ? defaultMsg + ". " + message : defaultMsg;📝 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.
| const defaultMsg = "Error occurred during operator execution"; | |
| const message = error?.bodyResponse?.error; | |
| const reason = message ? defaultMsg + ". " + message : defaultMsg; | |
| const defaultMsg = "Error occurred during operator execution"; | |
| const message = typeof error?.bodyResponse?.error === "string" | |
| ? error?.bodyResponse?.error | |
| : undefined; | |
| const reason = message ? defaultMsg + ". " + message : defaultMsg; |
release notes for 1.9.0 and 2.12.0
What changes are proposed in this pull request?
Note
Merge after 2:00 PM EST on 10/17/2025
Release version 1.9.0 of fiftyone.
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements
Documentation