-
Notifications
You must be signed in to change notification settings - Fork 589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge release/v0.24.1
to main
#4460
Conversation
* bump version for v0.24.1 * bump desktop * Teams integration (#4386) * teams merge step * add merge event * skip merge call, use pull number * add assignee * use actor * only develop for now * use branch * rm teams requirement * bump interval * add fetch depth * add bot author * use develop * alphabetize --------- Co-authored-by: Benjamin Kane <[email protected]>
* fix operator prompt hidden for executing, output, and error * add e2e tests for operators prompt * operator drawer prompt tweaks * mv to asserter * lint --------- Co-authored-by: Benjamin Kane <[email protected]>
Cherry picking PRs into v0.24.1
Teams 1.7.1/OSS 0.24.1 release notes
WalkthroughThe recent updates introduce various enhancements and fixes across multiple files. Key changes include new GitHub Actions workflows, updates to the FiftyOne App and its operators, improved handling of 3D scene assets, and adjustments to metadata computation. Additionally, new test cases and documentation updates have been added, along with several bug fixes and optimizations. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub
participant FiftyOneRepo
participant FiftyOneTeamsRepo
Developer->>GitHub: Push changes to develop branch
GitHub->>FiftyOneRepo: Trigger PR workflow
FiftyOneRepo->>GitHub: Run teams job
GitHub->>FiftyOneTeamsRepo: Trigger merge-oss.yml workflow
FiftyOneTeamsRepo-->>GitHub: Complete workflow
GitHub-->>FiftyOneRepo: Notify workflow completion
FiftyOneRepo-->>Developer: Teams job completed
Poem
Warning Review ran into problemsProblems (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Outside diff range and nitpick comments (42)
tests/unittests/metadata_tests.py (1)
Line range hint
8-8
: Remove unused import to clean up the code.- import json
e2e-pw/src/shared/assets/plugins/e2e/__init__.py (3)
26-47
: Ensure proper documentation for theE2ESayHelloInModal
class to enhance maintainability.Consider adding docstrings to explain the purpose and usage of each method within this class.
49-70
: Ensure proper documentation for theE2ESayHelloInDrawer
class to enhance maintainability.Consider adding docstrings to explain the purpose and usage of each method within this class.
72-95
: Ensure proper documentation for theE2EProgress
class to enhance maintainability.Consider adding docstrings to explain the purpose and usage of each method within this class, especially the asynchronous
execute
method which involves complex operations.app/packages/operators/src/utils.ts (1)
Line range hint
80-111
: RefactorgetOperatorPromptConfigs
to improve readability and maintainability.Consider breaking down this function into smaller, more focused functions. This can help isolate functionality and make the code easier to understand and maintain.
e2e-pw/src/oss/specs/operators/prompt.spec.ts (3)
57-85
: Ensure consistent use of assertions in test cases.Consider using consistent assertion styles and methods across all test cases to improve readability and maintainability.
87-118
: Ensure consistent use of assertions in test cases.Consider using consistent assertion styles and methods across all test cases to improve readability and maintainability.
120-138
: Ensure consistent use of assertions in test cases.Consider using consistent assertion styles and methods across all test cases to improve readability and maintainability.
package/desktop/setup.py (2)
Line range hint
15-16
: Remove duplicate imports ofos
andshutil
.- import os - import shutilTools
Ruff
16-16: Redefinition of unused
shutil
from line 11 (F811)
Line range hint
68-70
: Replace lambda with a function definition for better readability and maintainability.- is_platform = lambda os, isa=None: platform.startswith(os) and (not isa or platform.endswith(isa)) + def is_platform(os, isa=None): + return platform.startswith(os) and (not isa or platform.endswith(isa))Tools
Ruff
16-16: Redefinition of unused
shutil
from line 11 (F811)tests/unittests/patches_tests.py (2)
Line range hint
171-171
: Avoid using single-letter variable names likel
as they can be ambiguous and hard to understand.- for l in view2.values("ground_truth.label") + for label in view2.values("ground_truth.label")Also applies to: 475-475
Line range hint
537-537
: Avoid equality comparisons toTrue
; useif F("crowd"):
for truth checks.- view3 = view.match(F("crowd") == True).set_field( + view3 = view.match(F("crowd")).set_field(Also applies to: 579-579
fiftyone/core/patches.py (4)
Line range hint
561-592
: Consider documenting the new parameterspersistent
and_generated
.The parameters
persistent
and_generated
have been added to the function signature but are not described in the docstring. Would you like me to draft an update for the docstring to include these parameters?
Line range hint
654-654
: Simplify the truth check forother_fields
.- if other_fields == True: - other_fields = [f for f in src_schema if f not in curr_schema] + if other_fields: + other_fields = [f for f in src_schema if f not in curr_schema]
Line range hint
683-738
: Consider documenting the new parameterspersistent
and_generated
.The parameters
persistent
and_generated
have been added to the function signature but are not described in the docstring. Would you like me to draft an update for the docstring to include these parameters?
Line range hint
805-805
: Simplify the truth check forother_fields
.- if other_fields == True: - other_fields = [f for f in src_schema if f not in curr_schema] + if other_fields: + other_fields = [f for f in src_schema if f not in curr_schema]fiftyone/core/video.py (5)
Line range hint
617-617
: Useif not sample_frames:
instead ofif sample_frames != True:
.- if sample_frames != True: - l = locals() + if not sample_frames: + local_vars = locals()This change not only addresses the static analysis hint but also improves readability by using a more Pythonic conditional check.
Line range hint
618-618
: Rename variablel
tolocal_vars
for clarity.- l = locals() + local_vars = locals()Using
local_vars
as the variable name instead ofl
enhances readability and avoids confusion with the number1
or the letterI
.
Line range hint
707-707
: Consistently useif sample_frames:
for truth checks.- if sample_frames == True: + if sample_frames:This change simplifies the condition checks and adheres to Pythonic best practices.
Also applies to: 741-741, 748-748, 776-776, 810-810, 813-813, 858-858
Line range hint
789-789
: Useis not None
for None checks.- if support != None: + if support is not None:Using
is not None
is the recommended way to check forNone
in Python, as it is clearer and more idiomatic.
Line range hint
1032-1032
: Useif not sample_frames:
for false checks.- if sample_frames == False: + if not sample_frames:This change simplifies the condition checks and adheres to Pythonic best practices.
Also applies to: 1037-1037
fiftyone/core/clips.py (4)
Line range hint
119-119
: Replace the bareexcept
with a specific exception.Using a bare
except
can catch unexpected exceptions and obscure programming errors. It's a good practice to catch specific exceptions to avoid these issues.- except: + except ValueError:
Line range hint
542-542
: Use direct truthiness checks for boolean conditions.Pythonic style favors direct truthiness checks over explicit comparisons to
True
. This makes the code cleaner and easier to read.- if other_fields == True: - exclude_fields.clear() + if other_fields: + exclude_fields.clear()Also applies to: 723-723
Line range hint
1055-1055
: Useis not None
for None checks.Using
is not None
is the recommended way to check forNone
in Python, as it is clearer and more idiomatic.- if index != None: - index = int(index) + if index is not None: + index = int(index)
Line range hint
1125-1125
: Clarify ambiguous variable namel
.The variable name
l
is ambiguous and can be confused with the number1
or the letterI
. Consider renaming it to a more descriptive name.- for s, l in ranges: + for start, end in ranges:tests/intensive/cvat_tests.py (6)
Line range hint
13-14
: Remove unused imports to clean up the code.- from collections import defaultdict - import numpy as np
Line range hint
380-381
: Local variablesapi
andtask_id
are assigned but never used in the methodtest_multiple_fields
.- api = results.connect_to_api() - task_id = results.task_ids[0]
Line range hint
694-694
: The variableperson_labels
is assigned but never used in the methodtest_example_restricting_label_edits
.- person_labels = view.filter_labels( - "ground_truth", F("label") == "person" - ).values("ground_truth.detections", unwind=True)
Line range hint
816-816
: The variablestatus
is assigned but never used in the methodtest_deleted_tasks
.- status = results.get_status()
Line range hint
892-892
: Avoid using ambiguous variable names likel
. Consider renaming it tolabel
or another descriptive name.- label_map = {l: l.upper() for l in labels} + label_map = {label: label.upper() for label in labels}
Line range hint
1327-1327
: The variableresults
is assigned but never used in the methodtest_frame_start_stop_step
.- results = dataset.annotate( - anno_key, - backend="cvat", - label_field="frames.detections", - frame_start=10, - frame_stop=100, - frame_step=5, - )tests/unittests/video_tests.py (2)
Line range hint
1040-1040
: Replace lambda expression with a function definition.- filepath_fcn = lambda sample: sample.filepath + def filepath_fcn(sample): + return sample.filepath
Line range hint
2689-2689
: Consider renaming the variablel
to a more descriptive name to avoid ambiguity.- for _id, l, i, s in zip( + for _id, label, i, s in zip(Also applies to: 2954-2954, 3256-3256
fiftyone/utils/data/exporters.py (9)
Line range hint
543-543
: Avoid using bareexcept
statements.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))
Line range hint
573-573
: Avoid using bareexcept
statements.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))
Line range hint
616-616
: Avoid using bareexcept
statements.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))
Line range hint
651-651
: Avoid using bareexcept
statements.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))
Line range hint
906-906
: Avoid using bareexcept
statements.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))
Line range hint
3818-3818
: Avoid inequality comparisons toFalse
; useif include_confidence:
for truth checks.- if include_confidence == True: + if include_confidence:
Line range hint
3818-3818
: Avoid inequality comparisons toFalse
; useif include_attributes:
for truth checks.- if include_attributes == True: + if include_attributes:
Line range hint
3944-3944
: Avoid equality comparisons toTrue
; useif include_confidence:
for truth checks.- if include_confidence == True: + if include_confidence:
Line range hint
3949-3949
: Avoid equality comparisons toTrue
; useif include_attributes:
for truth checks.- if include_attributes == True: + if include_attributes:
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (18)
-
app/package.json
is excluded by!**/*.json
-
app/packages/aggregations/package.json
is excluded by!**/*.json
-
app/packages/app/package.json
is excluded by!**/*.json
-
app/packages/components/package.json
is excluded by!**/*.json
-
app/packages/core/package.json
is excluded by!**/*.json
-
app/packages/embeddings/package.json
is excluded by!**/*.json
-
app/packages/flashlight/package.json
is excluded by!**/*.json
-
app/packages/looker-3d/package.json
is excluded by!**/*.json
-
app/packages/looker/package.json
is excluded by!**/*.json
-
app/packages/map/package.json
is excluded by!**/*.json
-
app/packages/operators/package.json
is excluded by!**/*.json
-
app/packages/plugins/package.json
is excluded by!**/*.json
-
app/packages/relay/package.json
is excluded by!**/*.json
-
app/packages/spaces/package.json
is excluded by!**/*.json
-
app/packages/state/package.json
is excluded by!**/*.json
-
app/packages/utilities/package.json
is excluded by!**/*.json
-
app/yarn.lock
is excluded by!**/*.lock
,!**/*.lock
-
fiftyone/zoo/models/manifest-torch.json
is excluded by!**/*.json
Files selected for processing (36)
- .github/workflows/pr.yml (1 hunks)
- .github/workflows/push-release.yml (1 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (3 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx (1 hunks)
- app/packages/operators/src/OperatorPrompt/index.tsx (1 hunks)
- app/packages/operators/src/utils.ts (2 hunks)
- docs/source/plugins/index.rst (1 hunks)
- docs/source/release-notes.rst (3 hunks)
- docs/source/teams/cloud_media.rst (12 hunks)
- docs/source/user_guide/config.rst (1 hunks)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts (1 hunks)
- e2e-pw/src/shared/assets/plugins/e2e/init.py (2 hunks)
- e2e-pw/src/shared/assets/plugins/e2e/fiftyone.yml (1 hunks)
- fiftyone/core/clips.py (4 hunks)
- fiftyone/core/dataset.py (1 hunks)
- fiftyone/core/metadata.py (7 hunks)
- fiftyone/core/patches.py (6 hunks)
- fiftyone/core/stages.py (5 hunks)
- fiftyone/core/storage.py (3 hunks)
- fiftyone/core/threed/object_3d.py (3 hunks)
- fiftyone/core/threed/scene_3d.py (3 hunks)
- fiftyone/core/video.py (4 hunks)
- fiftyone/utils/cvat.py (7 hunks)
- fiftyone/utils/data/exporters.py (6 hunks)
- fiftyone/utils/utils3d.py (3 hunks)
- package/desktop/setup.py (1 hunks)
- setup.py (2 hunks)
- tests/intensive/cvat_tests.py (1 hunks)
- tests/unittests/metadata_tests.py (1 hunks)
- tests/unittests/patches_tests.py (2 hunks)
- tests/unittests/threed/scene_3d_tests.py (2 hunks)
- tests/unittests/utils3d_tests.py (1 hunks)
- tests/unittests/video_tests.py (3 hunks)
Files skipped from review due to trivial changes (5)
- docs/source/plugins/index.rst
- docs/source/user_guide/config.rst
- e2e-pw/src/shared/assets/plugins/e2e/fiftyone.yml
- setup.py
- tests/unittests/utils3d_tests.py
Additional context used
Path-based instructions (8)
app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/OperatorPrompt/index.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/operators/operators-prompt.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/utils.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/operators/prompt.spec.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Ruff
tests/unittests/metadata_tests.py
8-8:
json
imported but unused (F401)package/desktop/setup.py
15-15: Redefinition of unused
os
from line 10 (F811)
16-16: Redefinition of unused
shutil
from line 11 (F811)
68-70: Do not assign a
lambda
expression, use adef
(E731)fiftyone/core/metadata.py
561-561: Do not use bare
except
(E722)tests/unittests/patches_tests.py
171-171: Ambiguous variable name:
l
(E741)
475-475: Ambiguous variable name:
l
(E741)
537-537: Avoid equality comparisons to
True
; useif F("crowd"):
for truth checks (E712)
579-579: Avoid equality comparisons to
True
; useif F("crowd"):
for truth checks (E712)fiftyone/core/storage.py
8-8:
contextlib.contextmanager
imported but unused (F401)
882-882: Do not use bare
except
(E722)fiftyone/core/patches.py
285-285: Ambiguous variable name:
l
(E741)
654-654: Avoid equality comparisons to
True
; useif other_fields:
for truth checks (E712)
805-805: Avoid equality comparisons to
True
; useif other_fields:
for truth checks (E712)fiftyone/core/video.py
617-617: Avoid inequality comparisons to
True
; useif not sample_frames:
for false checks (E712)
618-618: Ambiguous variable name:
l
(E741)
707-707: Avoid equality comparisons to
False
; useif not sample_frames:
for false checks (E712)
741-741: Avoid inequality comparisons to
False
; useif sample_frames:
for truth checks (E712)
748-748: Avoid equality comparisons to
True
; useif sample_frames:
for truth checks (E712)
776-776: Avoid equality comparisons to
True
; useif sample_frames:
for truth checks (E712)
789-789: Comparison to
None
should becond is not None
(E711)
810-810: Avoid inequality comparisons to
False
; useif sample_frames:
for truth checks (E712)
813-813: Avoid equality comparisons to
True
; useif sample_frames:
for truth checks (E712)
858-858: Avoid equality comparisons to
True
; useif sample_frames:
for truth checks (E712)
1032-1032: Avoid equality comparisons to
False
; useif not sample_frames:
for false checks (E712)
1037-1037: Avoid inequality comparisons to
True
; useif not sample_frames:
for false checks (E712)fiftyone/core/clips.py
119-119: Do not use bare
except
(E722)
542-542: Avoid equality comparisons to
True
; useif other_fields:
for truth checks (E712)
723-723: Avoid equality comparisons to
True
; useif other_fields:
for truth checks (E712)
1055-1055: Comparison to
None
should becond is not None
(E711)
1125-1125: Ambiguous variable name:
l
(E741)tests/intensive/cvat_tests.py
13-13:
collections.defaultdict
imported but unused (F401)
14-14:
numpy
imported but unused (F401)
380-380: Local variable
api
is assigned to but never used (F841)
381-381: Local variable
task_id
is assigned to but never used (F841)
694-694: Local variable
person_labels
is assigned to but never used (F841)
816-816: Local variable
status
is assigned to but never used (F841)
892-892: Ambiguous variable name:
l
(E741)
1327-1327: Local variable
results
is assigned to but never used (F841)tests/unittests/video_tests.py
1040-1040: Do not assign a
lambda
expression, use adef
(E731)
2689-2689: Ambiguous variable name:
l
(E741)
2954-2954: Ambiguous variable name:
l
(E741)
3256-3256: Ambiguous variable name:
l
(E741)fiftyone/utils/data/exporters.py
543-543: Do not use bare
except
(E722)
573-573: Do not use bare
except
(E722)
616-616: Do not use bare
except
(E722)
651-651: Do not use bare
except
(E722)
906-906: Do not use bare
except
(E722)
3818-3818: Avoid inequality comparisons to
False
; useif include_confidence:
for truth checks (E712)
3818-3818: Avoid inequality comparisons to
False
; useif include_attributes:
for truth checks (E712)
3944-3944: Avoid equality comparisons to
True
; useif include_confidence:
for truth checks (E712)
3949-3949: Avoid equality comparisons to
True
; useif include_attributes:
for truth checks (E712)fiftyone/utils/cvat.py
3440-3440: Do not use bare
except
(E722)
3475-3475: Do not use bare
except
(E722)
3989-3989: Local variable
response
is assigned to but never used (F841)
4152-4152: Local variable
response
is assigned to but never used (F841)
6078-6078: Local variable
classes
is assigned to but never used (F841)
6723-6723: Local variable
formatted_track
is assigned to but never used (F841)
6859-6859: Do not use bare
except
(E722)
6936-6936: Do not use bare
except
(E722)
7412-7412: Do not use bare
except
(E722)
7421-7421: Do not use bare
except
(E722)
7426-7426: Do not use bare
except
(E722)
7832-7832: Do not use bare
except
(E722)fiftyone/core/stages.py
946-954: Do not assign a
lambda
expression, use adef
(E731)
956-970: Do not assign a
lambda
expression, use adef
(E731)
1540-1540: Comparison to
None
should becond is not None
(E711)
2504-2504: Do not assign a
lambda
expression, use adef
(E731)
2509-2509: Do not assign a
lambda
expression, use adef
(E731)
2568-2568: Comparison to
None
should becond is not None
(E711)
2570-2570: Comparison to
None
should becond is not None
(E711)
2577-2577: Comparison to
None
should becond is not None
(E711)
2588-2588: Comparison to
None
should becond is not None
(E711)
2752-2752: Comparison to
None
should becond is not None
(E711)
3920-3920: Comparison to
None
should becond is not None
(E711)
5178-5178: Comparison to
None
should becond is not None
(E711)
6620-6620: Comparison to
None
should becond is not None
(E711)
7270-7270: Do not use bare
except
(E722)
7282-7282: Do not use bare
except
(E722)
7290-7290: Do not use bare
except
(E722)
8284-8284: Do not use bare
except
(E722)fiftyone/core/dataset.py
40-40:
fiftyone.core.odm.dataset.SampleFieldDocument
imported but unused (F401)
236-236: Do not use bare
except
(E722)
342-342: Do not use bare
except
(E722)
3325-3325: Ambiguous variable name:
l
(E741)
3341-3341: Ambiguous variable name:
l
(E741)
3384-3384: Ambiguous variable name:
l
(E741)
6733-6733: Avoid equality comparisons to
False
; useif not attach_frames:
for false checks (E712)
7112-7112: Do not use bare
except
(E722)
7122-7122: Do not use bare
except
(E722)
7282-7282: Do not assign a
lambda
expression, use adef
(E731)
7282-7282: Ambiguous variable name:
l
(E741)
7576-7576: Do not use bare
except
(E722)
8396-8396: Do not assign a
lambda
expression, use adef
(E731)
GitHub Check: e2e / test-e2e
e2e-pw/src/oss/poms/operators/operators-prompt.ts
[warning] 60-60:
Unexpected usage of not.toBeVisible(). Use toBeHidden() instead
Additional comments not posted (47)
app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx (1)
16-16
: Addition ofdata-cy
attribute enhances testability.app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (2)
9-9
: Refactoring to use a singlepromptConfig
object simplifies the component's props management.
12-12
: Addition ofdata-cy
attributes enhances testability.Also applies to: 14-14
.github/workflows/pr.yml (1)
19-35
: ```shell
#!/bin/bashDescription: Verify that the
teams
job triggers correctly.Create a new branch for testing
git checkout -b test-branch
Make a dummy commit to ensure the branch has changes
echo "Test commit" > testfile.txt
git add testfile.txt
git commit -m "Test commit for workflow trigger"Push the new branch to the remote repository
git push origin test-branch
Create a pull request to
develop
and check the workflow executiongh pr create --base develop --head test-branch --title "Test PR for workflow trigger" --body "This is a test PR to verify the workflow trigger."
Approve the pull request to meet the review requirement
gh pr review --approve --body "Approving for test purposes" test-branch
Merge the pull request
gh pr merge test-branch --merge --admin
Trigger the workflow manually
gh workflow run pr.yml --ref develop
</blockquote></details> <details> <summary>.github/workflows/push-release.yml (1)</summary><blockquote> `17-37`: The new steps in the `Push Release` GitHub Actions workflow are correctly set up to handle branch management and pull request creation. ```shell #!/bin/bash # Description: Verify that the new workflow correctly handles pushes and creates pull requests. # Test: Simulate a push to a `release` branch and check the pull request creation. gh repo clone voxel51/fiftyone cd fiftyone git checkout -b release/test touch test.txt git add test.txt git commit -m "Test push" git push origin release/test gh workflow run push-release.yml --ref release/test
tests/unittests/metadata_tests.py (2)
33-35
: LGTM! Good addition to ensure duplicate files are not counted multiple times in metadata calculations.
43-49
: LGTM! Accurate calculation and validation of metadata size.e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts (1)
6-6
: LGTM! Comprehensive setup and execution for testing built-in operators.Also applies to: 10-10
app/packages/operators/src/OperatorPrompt/index.tsx (1)
Line range hint
1-1
: LGTM! Enhances flexibility and maintainability of the operator prompt system.Also applies to: 10-10
app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (1)
1-1
: LGTM! Improves testability and user experience by addingdata-cy
attributes and using theResizable
component.Also applies to: 35-35, 46-60
tests/unittests/threed/scene_3d_tests.py (2)
74-102
: Ensure that thetest_update_asset_paths
method correctly updates asset paths and verifies the updated paths.
119-132
: Verify that thetest_write_resolve_relative
method correctly resolves relative paths during the write operation.fiftyone/core/threed/object_3d.py (1)
230-230
: Ensure that the_get_asset_paths
method correctly retrieves asset paths from the object.fiftyone/core/threed/scene_3d.py (2)
189-189
: Ensure that the asset path resolution and update mechanisms in theScene
class work correctly across various methods.Also applies to: 279-290, 296-296
326-338
: Verify that theget_asset_paths
method correctly aggregates all asset paths from the scene, including those from the background.fiftyone/core/metadata.py (4)
8-9
: Imports fromcollections
anditertools
are added.
74-74
: Ensure that the HTTP request status is checked before processing the response.
139-139
: Ensure that the HTTP request status is checked before processing the response for image metadata.
217-319
: Significant enhancements toSceneMetadata
class including handling of local and URL-based scenes, a cache mechanism, and asset path handling.tests/unittests/patches_tests.py (2)
14-14
: Import offiftyone.core.patches
asfop
added.
662-720
: New testtest_make_patches_dataset
added to verify the functionality of creating a patches dataset.fiftyone/core/storage.py (2)
284-286
: LGTM! Therealpath
function correctly resolves symlinks and relative path indicators.
310-311
: LGTM! Theabspath
function correctly converts paths to their absolute form.docs/source/teams/cloud_media.rst (6)
Line range hint
394-406
: LGTM! Thedownload_media
method now correctly includes theinclude_assets
parameter.
438-443
: LGTM! Thedownload_scenes
method now correctly includes theinclude_assets
parameter.
Line range hint
470-479
: LGTM! Thedownload_context
method now correctly includes theinclude_assets
parameter.
Line range hint
520-531
: LGTM! Theget_local_paths
method now correctly includes theinclude_assets
parameter.
Line range hint
546-563
: LGTM! Thecache_stats
method now correctly includes theinclude_assets
parameter.
Line range hint
571-589
: LGTM! Theclear_media
method now correctly includes theinclude_assets
parameter.fiftyone/core/clips.py (1)
682-690
: ```shell
#!/bin/bashDescription: Extract the implementation of the
make_clips_dataset
function fromfiftyone/core/clips.py
.Extract the function definition and its body.
ast-grep --lang python --pattern 'def make_clips_dataset(
$_) { $ $$ }'</blockquote></details> <details> <summary>fiftyone/utils/utils3d.py (1)</summary><blockquote> `835-837`: Verify the path handling for robustness. Ensure that the path handling in `_get_pcd_filepath_from_scene` function correctly resolves relative paths to absolute paths, especially in different operating system environments. </blockquote></details> <details> <summary>tests/intensive/cvat_tests.py (1)</summary><blockquote> `309-309`: Ensure that the `segment_size` parameter is documented and tested. <details> <summary>Verification successful</summary> --- <!--Monologue: [task item 1/1] Verify if the `segment_size` parameter is documented and tested. [observations] The `segment_size` parameter appears in `fiftyone/utils/cvat.py` and `tests/intensive/cvat_tests.py`. [decomposition] Check if the parameter is documented in `fiftyone/utils/cvat.py` and if it is tested in `tests/intensive/cvat_tests.py`. [analysis] The `segment_size` parameter is mentioned in the docstrings in `fiftyone/utils/cvat.py` and is used in tests in `tests/intensive/cvat_tests.py`. [actions] Conclude that the `segment_size` parameter is documented and tested. --> The `segment_size` parameter is both documented and tested in the codebase. - **Documentation**: The parameter is documented in `fiftyone/utils/cvat.py`. - **Testing**: The parameter is tested in `tests/intensive/cvat_tests.py`. </details> <details> <summary>Scripts executed</summary> The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Description: Verify that the `segment_size` parameter is documented and tested. # Test: Search for the `segment_size` parameter in the documentation and tests. rg --type py 'segment_size' --files-with-matches | xargs rg 'segment_size'
Length of output: 911
tests/unittests/video_tests.py (3)
2203-2257
: The new testtest_make_frames_dataset
is well-structured and covers the functionality of creating a frames dataset from a video dataset. Ensure that the assertions are correctly validating the intended properties.
3286-3354
: The new testtest_make_clips_dataset
effectively checks the creation of a clips dataset from a video dataset. It's good to see thorough checks on the properties of the resulting dataset.
16-18
: Ensure that the newly added imports are utilized within the file.Verification successful
The newly added imports
fiftyone.core.clips
,fiftyone.core.odm
, andfiftyone.core.video
are indeed utilized within the filetests/unittests/video_tests.py
. The aliasesfoc
,foo
, andfov
are used in various parts of the code.
foo
is used in multiple instances, such asfoo.get_db_conn()
.fov
is used infov.make_frames_dataset(dataset)
.foc
is used infoc.make_clips_dataset(dataset, "events")
.This confirms that the imports are necessary and correctly utilized.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the newly added imports are utilized within the file. # Test: Search for usage of the newly imported modules. Expect: At least one usage of each module. rg --type python $'fiftyone.core.clips|fiftyone.core.odm|fiftyone.core.video' tests/unittests/video_tests.pyLength of output: 140
Script:
#!/bin/bash # Description: Verify that the newly added imports are utilized within the file. # Test: Search for usage of the newly imported modules. Expect: At least one usage of each module. rg 'fiftyone.core.clips|fiftyone.core.odm|fiftyone.core.video' tests/unittests/video_tests.pyLength of output: 195
Script:
#!/bin/bash # Description: Verify that the aliases of the newly added imports are utilized within the file. # Test: Search for usage of the aliases `foc`, `foo`, and `fov`. Expect: At least one usage of each alias. rg '\bfoc\b|\bfoo\b|\bfov\b' tests/unittests/video_tests.pyLength of output: 4351
docs/source/release-notes.rst (2)
6-54
: Release notes are clear and well-documented with appropriate references to pull requests.
Line range hint
2-2
: Release notes update is concise and includes relevant pull request references.
fiftyone/utils/cvat.py (5)
3055-3056
: Well-documented parameters for image quality and caching.
3077-3078
: Well-documented parameters for task naming and occluded attributes.
3675-3677
: Correct implementation of thejob_annotation_url
method.
4315-4316
: Correct handling of filename ordering for compatibility with older CVAT versions.
4906-4917
: Correct implementation of the_get_job_ids
method, handling different server versions appropriately.
fiftyone/core/stages.py (5)
7482-7485
: The addition of_generated=True
tomake_patches_dataset
correctly implements the handling of temporary datasets as intended in the PR.
7629-7632
: The addition of_generated=True
tomake_evaluation_patches_dataset
is consistent with the handling of temporary datasets and aligns with the changes in other parts of the code.
7789-7792
: The addition of_generated=True
tomake_clips_dataset
ensures consistency in handling temporary datasets across different dataset creation functions.
7926-7930
: The addition of_generated=True
andtrajectories=True
tomake_clips_dataset
is appropriate. Ensure that thetrajectories
parameter is handled correctly in the dataset creation logic.
8114-8116
: The addition of_generated=True
tomake_frames_dataset
aligns with the consistent handling of temporary datasets across the application.
@@ -3,6 +3,55 @@ FiftyOne Release Notes | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please complete the entry for the tracking utility to provide full context.
server_id_map.get("shapes", {}), | ||
class_map, | ||
attr_id_map, | ||
frames, | ||
ignore_types, | ||
frame_stop, | ||
frame_step, | ||
assigned_scalar_attrs=scalar_attrs, | ||
track_index=track_index, | ||
track_group_id=track_group_id, | ||
immutable_attrs=immutable_attrs, | ||
occluded_attrs=_occluded_attrs, | ||
group_id_attrs=_group_id_attrs, | ||
) | ||
label_field_results = self._merge_results( | ||
label_field_results, track_shape_results | ||
label_field_results, shape_results | ||
) | ||
|
||
frames_metadata = {} | ||
for cvat_frame_id, frame_data in frame_id_map[ | ||
task_id | ||
].items(): | ||
sample_id = frame_data["sample_id"] | ||
if "frame_id" in frame_data and len(frames) == 1: | ||
frames_metadata[sample_id] = frames[0] | ||
break | ||
|
||
if len(frames) > cvat_frame_id: | ||
frame_metadata = frames[cvat_frame_id] | ||
else: | ||
frame_metadata = None | ||
|
||
frames_metadata[sample_id] = frame_metadata | ||
for track_index, track in enumerate(tracks, 1): | ||
label_id = track["label_id"] | ||
shapes = track["shapes"] | ||
track_group_id = track.get("group", None) | ||
for shape in shapes: | ||
shape["label_id"] = label_id | ||
|
||
immutable_attrs = track["attributes"] | ||
|
||
track_shape_results = self._parse_shapes_tags( | ||
"track", | ||
shapes, | ||
frame_id_map[task_id], | ||
label_type, | ||
_id_map, | ||
server_id_map.get("tracks", {}), | ||
class_map, | ||
attr_id_map, | ||
frames, | ||
ignore_types, | ||
frame_stop, | ||
frame_step, | ||
assigned_scalar_attrs=scalar_attrs, | ||
track_index=track_index, | ||
track_group_id=track_group_id, | ||
immutable_attrs=immutable_attrs, | ||
occluded_attrs=_occluded_attrs, | ||
group_id_attrs=_group_id_attrs, | ||
) | ||
label_field_results = self._merge_results( | ||
label_field_results, track_shape_results | ||
) | ||
|
||
# Polyline(s) corresponding to instance/semantic masks need to | ||
# be converted to their final format | ||
self._convert_polylines_to_masks( | ||
label_field_results, label_info, frames_metadata | ||
) | ||
frames_metadata = {} | ||
for cvat_frame_id, frame_data in frame_id_map[ | ||
task_id | ||
].items(): | ||
sample_id = frame_data["sample_id"] | ||
if "frame_id" in frame_data and len(frames) == 1: | ||
frames_metadata[sample_id] = frames[0] | ||
break | ||
|
||
if len(frames) > cvat_frame_id: | ||
frame_metadata = frames[cvat_frame_id] | ||
else: | ||
frame_metadata = None | ||
|
||
frames_metadata[sample_id] = frame_metadata | ||
|
||
# Polyline(s) corresponding to instance/semantic masks | ||
# need to be converted to their final format | ||
self._convert_polylines_to_masks( | ||
label_field_results, label_info, frames_metadata | ||
) | ||
|
||
annotations = self._merge_results( | ||
annotations, {label_field: label_field_results} | ||
) | ||
annotations = self._merge_results( | ||
annotations, {label_field: label_field_results} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complex method handling job annotations. Consider refactoring for better readability and maintainability.
persistent=False, | ||
_generated=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add documentation for new parameters persistent
and _generated
.
The parameters persistent
and _generated
have been added to the function signature but are not documented in the docstring. It's important to explain the purpose and usage of these parameters to maintain good code documentation practices.
persistent=False, | ||
_generated=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider documenting the _generated
parameter in the function docstring.
The _generated
parameter is used in the function but is not mentioned in the docstring. This could lead to confusion for developers trying to understand the function's behavior. Consider adding a description for this parameter to improve code readability and maintainability.
Also applies to: 655-656
scene_dir = os.path.dirname(original_scene_path) | ||
for i, asset_path in enumerate(asset_paths): | ||
if not fos.isabs(asset_path): | ||
asset_path = fos.join(scene_dir, asset_path) | ||
asset_paths[i] = fos.resolve(asset_path) | ||
asset_paths[i] = fos.abspath(fos.join(scene_dir, asset_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the path concatenation uses a platform-independent method.
- asset_paths[i] = fos.abspath(fos.join(scene_dir, asset_path))
+ asset_paths[i] = fos.abspath(os.path.join(scene_dir, asset_path))
This change ensures that the path concatenation is handled correctly across different operating systems.
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.
asset_paths[i] = fos.abspath(fos.join(scene_dir, asset_path)) | |
asset_paths[i] = fos.abspath(os.path.join(scene_dir, asset_path)) |
|
||
try: | ||
return _get_metadata(filepath, media_type) | ||
return _get_metadata(filepath, media_type, cache=cache) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using bare except
statements as they can obscure real errors. Specify the exception type.
- except:
+ except Exception as e:
+ # Handle or log the exception e
Committable suggestion was skipped due to low confidence.
Tools
Ruff
561-561: Do not use bare
except
(E722)
await expect(this.panelPom.locator).toBeVisible(); | ||
} | ||
async isClosed() { | ||
await expect(this.panelPom.locator).not.toBeVisible(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace not.toBeVisible()
with toBeHidden()
for consistency.
- await expect(this.panelPom.locator).not.toBeVisible();
+ await expect(this.panelPom.locator).toBeHidden();
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.
await expect(this.panelPom.locator).not.toBeVisible(); | |
await expect(this.panelPom.locator).toBeHidden(); |
Tools
GitHub Check: e2e / test-e2e
[warning] 60-60:
Unexpected usage of not.toBeVisible(). Use toBeHidden() instead
job_resp_json = job_resp_json["results"] | ||
|
||
job_ids = [j["id"] for j in job_resp_json] | ||
job_ids = self._get_job_ids(task_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a timeout to the polling mechanism to prevent potential infinite loops.
+ timeout = 30 # seconds
+ start_time = time.time()
while not job_ids:
job_ids = self._get_job_ids(task_id)
if not job_ids:
+ if time.time() - start_time > timeout:
+ raise TimeoutError("Timeout waiting for job IDs")
time.sleep(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.
job_ids = self._get_job_ids(task_id) | |
timeout = 30 # seconds | |
start_time = time.time() | |
while not job_ids: | |
job_ids = self._get_job_ids(task_id) | |
if not job_ids: | |
if time.time() - start_time > timeout: | |
raise TimeoutError("Timeout waiting for job IDs") | |
time.sleep(1) |
@@ -7806,7 +7806,7 @@ def _get_frames_pipeline(sample_collection): | |||
view = None | |||
|
|||
if dataset._is_clips: | |||
# Clips datasets use `sample_id` to associated with frames, but now as | |||
# Clips datasets use `sample_id` to associate with frames, but now as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential typo in variable name.
- pipeline = sample_collection._pipeline(attach_frames=True) + [
+ pipeline = coll._pipeline(attach_frames=True) + [
It appears that sample_collection
might be intended to be coll
based on the context provided. Please verify and correct if necessary.
Committable suggestion was skipped due to low confidence.
* add an effect to refetch saved views on dataset change * rm saved view attr usage for state, handle group slice --------- Co-authored-by: Benjamin Kane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (4)
fiftyone/server/mutation.py (1)
Line range hint
172-172
: Consider revising variable names and exception handling for clarity and safety.- except: + except Exception as e:- "{}".format(view_name) + f"{view_name}"Also applies to: 207-207, 274-276
app/packages/operators/src/operators.ts (3)
Line range hint
64-64
: Move default parameters to follow required parameters in the constructor.- constructor(public params: any = {}, public _currentContext: any, public hooks: any = {}, public executor: Executor = null) + constructor(public _currentContext: any, public params: any = {}, public hooks: any = {}, public executor: Executor = null)
Line range hint
155-157
: Simplify boolean assignments in the constructor.- this.executeAsGenerator = options.executeAsGenerator || false; - this.dynamic = options.dynamic || false; - this.unlisted = options.unlisted || false; + this.executeAsGenerator = Boolean(options.executeAsGenerator); + this.dynamic = Boolean(options.dynamic); + this.unlisted = Boolean(options.unlisted);
Line range hint
255-255
: Clarify the use ofthis
in the static method by using the class name.- const operator = this.fromJSON(json); + const operator = Operator.fromJSON(json);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- app/packages/core/src/components/MainSpace/MainSpace.tsx (1 hunks)
- app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1 hunks)
- app/packages/operators/src/OperatorInvocationRequestExecutor.tsx (1 hunks)
- app/packages/operators/src/constants.ts (1 hunks)
- app/packages/operators/src/operators.ts (2 hunks)
- app/packages/spaces/src/hooks.ts (3 hunks)
- app/packages/spaces/src/state.ts (1 hunks)
- fiftyone/server/events/initialize.py (3 hunks)
- fiftyone/server/mutation.py (4 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/operators/src/constants.ts
Additional context used
Path-based instructions (6)
app/packages/operators/src/OperatorInvocationRequestExecutor.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/MainSpace/MainSpace.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/spaces/src/state.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/spaces/src/hooks.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/operators/src/operators.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Ruff
fiftyone/server/events/initialize.py
141-141: Do not use bare
except
(E722)
156-156: Do not use bare
except
(E722)
165-165: Do not use bare
except
(E722)
197-197: Do not use bare
except
(E722)
229-229: Do not use bare
except
(E722)fiftyone/server/mutation.py
20-20:
fiftyone.core.utils
imported but unused (F401)Remove unused import:
fiftyone.core.utils
172-172: Ambiguous variable name:
l
(E741)
207-207: Do not use bare
except
(E722)
274-276: Use f-string instead of
format
call (UP032)Convert to f-string
Biome
app/packages/spaces/src/hooks.ts
[error] 50-50: void is not valid as a constituent in a union type (lint/suspicious/noConfusingVoidType)
Remove void
[error] 244-244: Use !== instead of !=.
!= is only allowed when comparing againstnull
(lint/suspicious/noDoubleEquals)!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==
[error] 249-249: Use !== instead of !=.
!= is only allowed when comparing againstnull
(lint/suspicious/noDoubleEquals)!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==app/packages/operators/src/operators.ts
[error] 64-64: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 97-97: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 98-98: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 155-155: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
[error] 157-157: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
[error] 210-210: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 249-249: void is not valid as a constituent in a union type (lint/suspicious/noConfusingVoidType)
Remove void
[error] 255-255: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 472-472: Use === instead of ==.
== is only allowed when comparing againstnull
(lint/suspicious/noDoubleEquals)== is only allowed when comparing against null
Using == may be unsafe if you are relying on type coercion
Unsafe fix: Use ===
[error] 474-474: Use === instead of ==.
== is only allowed when comparing againstnull
(lint/suspicious/noDoubleEquals)== is only allowed when comparing against null
Using == may be unsafe if you are relying on type coercion
Unsafe fix: Use ===
[error] 629-629: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 646-646: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 725-725: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 764-764: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (4)
app/packages/operators/src/OperatorInvocationRequestExecutor.tsx (1)
18-18
: Ensure proper error handling in theRequestExecutor
component.The
RequestExecutor
component should handle potential errors that might occur during the execution process. Consider adding error handling mechanisms or verify thatonError
is adequately handling all possible errors.app/packages/core/src/components/Sidebar/ViewSelection/index.tsx (1)
83-85
: LGTM! Ensuring data is refetched whendatasetName
changes is a good practice for keeping UI in sync with data.app/packages/spaces/src/hooks.ts (1)
34-37
: LGTM! The addition ofclearSpaces
and the conditional state initialization inuseSpaces
are good practices for state management in React.Also applies to: 43-46
app/packages/operators/src/operators.ts (1)
7-7
: Import ofQueueItemStatus
from./constants
is correctly placed and used later in the file.
const pendingRequests = useMemo(() => { | ||
return requests.filter( | ||
(queueItem) => queueItem.status === QueueItemStatus.Pending | ||
); | ||
}, [requests]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the useMemo dependency array correctly reflects all variables used in the memoized value.
The dependency array for useMemo
should include all variables that affect the returned value to avoid stale closures. Here, it should include QueueItemStatus.Pending
to ensure the memoization updates correctly if the status enum changes.
- }, [requests]);
+ }, [requests, QueueItemStatus.Pending]);
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 pendingRequests = useMemo(() => { | |
return requests.filter( | |
(queueItem) => queueItem.status === QueueItemStatus.Pending | |
); | |
}, [requests]); | |
const pendingRequests = useMemo(() => { | |
return requests.filter( | |
(queueItem) => queueItem.status === QueueItemStatus.Pending | |
); | |
}, [requests, QueueItemStatus.Pending]); |
const [panelsState, setPanelsState] = usePanelsState(); | ||
const oldSpaces = useRef(spaces); | ||
const oldPanelsState = useRef(panelsState); | ||
const isMounted = useRef(false); | ||
|
||
useEffect(() => clearSpaces, [clearSpaces]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the useEffect hook is correctly set up to call the clearSpaces
function.
The useEffect
hook should call the clearSpaces
function, not just reference it. This might be causing the function not to execute as intended.
- useEffect(() => clearSpaces, [clearSpaces]);
+ useEffect(() => { clearSpaces(); }, [clearSpaces]);
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(() => clearSpaces, [clearSpaces]); | |
useEffect(() => { clearSpaces(); }, [clearSpaces]); |
export const spacesAtom = atom<{ | ||
[spaceId: string]: SpaceNodeJSON | undefined; | ||
}>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The search results show that spacesAtom
is used in a selector family spaceSelector
to get and set space states. However, there is no explicit handling of undefined
values when accessing spacesAtom
.
- In the
get
function ofspaceSelector
,get(spacesAtom)[spaceId]
directly accesses the value without checking forundefined
. - In the
set
function,spaceState
is cast toSpaceNodeJSON
without handling the case where it might beundefined
.
These usages should be reviewed and updated to handle undefined
values properly to avoid potential runtime errors.
- Ensure
undefined
values are handled in theget
function ofspaceSelector
. - Ensure
undefined
values are handled in theset
function ofspaceSelector
.
Analysis chain
Ensure that the type change in spacesAtom
does not introduce inconsistencies.
The change to allow SpaceNodeJSON
values to be undefined
in spacesAtom
might affect other parts of the application that interact with this atom. Verify that all usages of spacesAtom
correctly handle the possibility of undefined
values.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify handling of `undefined` values in `spacesAtom`.
# Test: Search for usages of `spacesAtom`. Expect: Proper handling of `undefined`.
rg --type typescript $'spacesAtom' app/packages/spaces/src/
Length of output: 93
Script:
#!/bin/bash
# Description: Verify handling of `undefined` values in `spacesAtom`.
# Test: Search for usages of `spacesAtom`. Expect: Proper handling of `undefined`.
rg --type ts 'spacesAtom' app/packages/spaces/src/
Length of output: 811
Script:
#!/bin/bash
# Description: Extract lines of code where `spacesAtom` is used to verify handling of `undefined` values.
# Extract lines where `spacesAtom` is used in `state.ts`.
rg --type ts --context 5 'spacesAtom' app/packages/spaces/src/state.ts
Length of output: 1051
state.group_slice = state.dataset.group_slice | ||
state.selected = [] | ||
state.selected_labels = [] | ||
state.view = None | ||
state.spaces = None | ||
|
||
except: | ||
state.dataset = None | ||
state.group_slice = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace bare except
statements with more specific exception handling.
Using bare except
statements can catch unexpected exceptions and obscure underlying issues. Replace them with more specific exceptions to improve error handling and maintainability.
- except:
+ except ExpectedExceptionType:
Also applies to: 156-156, 165-165, 197-197, 229-229
Committable suggestion was skipped due to low confidence.
Tools
Ruff
141-141: Do not use bare
except
(E722)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (9)
fiftyone/core/storage.py (9)
Line range hint
8-8
: Remove unused import:contextlib.contextmanager
.- from contextlib import contextmanager
Line range hint
115-116
: Use format specifiers instead of percent format.- raise ValueError("The requested operation requires a local path, but found '%s'" % path) + raise ValueError(f"The requested operation requires a local path, but found '{path}'")
Line range hint
151-151
: Removeobject
inheritance fromTempDir
class.- class TempDir(object): + class TempDir:
Line range hint
469-469
: Use format specifiers instead of percent format.- raise ValueError("Unable to load JSON from '%s'" % path_or_str) + raise ValueError(f"Unable to load JSON from '{path_or_str}'")
Line range hint
485-485
: Use format specifiers instead of percent format and improve exception handling.- raise ValueError("Unable to parse JSON file '%s'" % path) + raise ValueError(f"Unable to parse JSON file '{path}'") from None
Line range hint
530-531
: Combinewith
statements for efficiency.- with open(path, "r") as f: - with jsonlines.Reader(f) as r: + with open(path, "r") as f, jsonlines.Reader(f) as r:
Line range hint
612-615
: Use ternary operator for concise code.- if abs_paths: - fp = filepath - else: - fp = os.path.join(dirpath, filepath) + fp = filepath if abs_paths else os.path.join(dirpath, filepath)
Line range hint
978-978
: Use context handler for opening files.- with open(path, mode) as f: + with open(path, mode) as f, jsonlines.Writer(f) as w:
Line range hint
1015-1015
: Use format specifiers instead of percent format.- raise TypeError("Failed to convert %s to bytes" % type(b)) + raise TypeError(f"Failed to convert {type(b)} to bytes")
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/core/storage.py (9 hunks)
Additional context used
Ruff
fiftyone/core/storage.py
8-8:
contextlib.contextmanager
imported but unused (F401)Remove unused import:
contextlib.contextmanager
115-116: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
151-151: Class
TempDir
inherits fromobject
(UP004)Remove
object
inheritance
469-469: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
482-482: Unnecessary open mode parameters (UP015)
Remove open mode parameters
485-485: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling (B904)
485-485: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
518-518: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
530-531: Use a single
with
statement with multiple contexts instead of nestedwith
statements (SIM117)Combine
with
statements
530-530: Unnecessary open mode parameters (UP015)
Remove open mode parameters
542-543: Use a single
with
statement with multiple contexts instead of nestedwith
statements (SIM117)Combine
with
statements
556-556: Unnecessary open mode parameters (UP015)
Remove open mode parameters
612-615: Use ternary operator
fp = filepath if abs_paths else os.path.join(dirpath, filepath)
instead ofif
-else
-block (SIM108)Replace
if
-else
-block withfp = filepath if abs_paths else os.path.join(dirpath, filepath)
705-705: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
711-711:
re.split
should passmaxsplit
andflags
as keyword arguments to avoid confusion due to unintuitive argument positions (B034)
896-896: Do not use bare
except
(E722)
913-914: Use a single
with
statement with multiple contexts instead of nestedwith
statements (SIM117)
978-978: Use context handler for opening files (SIM115)
1015-1015: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
def _run(fcn, tasks, return_results=True, num_workers=None, progress=None): | ||
try: | ||
num_tasks = len(tasks) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using bare except
.
- except:
+ except Exception as e:
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
except: | |
except Exception as e: |
Tools
Ruff
896-896: Do not use bare
except
(E722)
def realpath(path): | ||
"""Converts the given path to absolute, resolving symlinks and relative | ||
path indicators such as ``.`` and ``..``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update function name in documentation.
- """Converts the given path to absolute, resolving symlinks and relative
+ """Converts the given path to real, resolving symlinks and relative
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.
def realpath(path): | |
"""Converts the given path to absolute, resolving symlinks and relative | |
path indicators such as ``.`` and ``..``. | |
def realpath(path): | |
"""Converts the given path to real, resolving symlinks and relative | |
path indicators such as ``.`` and ``..``. |
Updating release notes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- docs/source/release-notes.rst (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/source/release-notes.rst
What changes are proposed in this pull request?
Merge
release/v0.24.1
tomain
to release OSSfiftyone
v0.24.1Updates to the GitHub workflow files:
Updated the dependencies in several packages to newer versions.
Changes to source code:
Documentation updates:
Summary by CodeRabbit
New Features
teams
in the GitHub Actions workflow for enhanced CI/CD integration.QueueItemStatus
for better task status tracking in the app.fo.Dataset
class.Bug Fixes
Documentation
Refactor
QueueItemStatus
enum to a new file for better code organization.Tests