Remove desktop App #4748
Remove desktop App #4748
Conversation
WalkthroughThe changes primarily involve the removal of support for the FiftyOne Desktop application across various components of the codebase. This includes eliminating references to desktop-specific functionality, such as command-line options, configuration settings, and related logic in components. Documentation has also been updated to reflect this shift, focusing on web-based interactions and removing any mention of the desktop version of the application. Additionally, a new section has been added to the deprecation documentation to clarify the end of support for the FiftyOne Desktop package. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant CLI
participant Documentation
User->>CLI: Launch App
CLI->>App: Start Application
App->>User: Display Web Interface
Documentation-->>User: Provide Instructions
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
fiftyone/core/config.py (1)
Verify and update the reference to
desktop_appinfiftyone/utils/quickstart.py.The
desktop_appattribute is referenced in thefiftyone/utils/quickstart.pyfile. Since its initialization has been removed from theFiftyOneConfigclass, ensure that this reference is still valid or update it accordingly to prevent potential issues.
- File:
fiftyone/utils/quickstart.py- Reference:
fiftyone.config.desktop_appAnalysis chain
Line range hint
80-80: Verify the impact of removing thedesktop_appattribute initialization.The
desktop_appattribute initialization has been removed from the__init__method. This change suggests that the desktop app functionality is being deprecated.Check if any logic in the codebase depends on the
desktop_appattribute being initialized, and update it accordingly. Use the following script to search for potential usages:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential usages of the `desktop_app` attribute. # Test: Search for accesses to the `desktop_app` attribute. Expect: No concerning usages. rg --type python 'desktop_app' -A 5Length of output: 66
Script:
#!/bin/bash # Description: Search for potential usages of the `desktop_app` attribute. # Test: Search for accesses to the `desktop_app` attribute. Expect: No concerning usages. rg --type py 'desktop_app' -A 5Length of output: 379
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (17)
app/package.jsonis excluded by!**/*.jsonapp/packages/app/package.jsonis excluded by!**/*.jsonapp/packages/desktop/package.jsonis excluded by!**/*.jsonapp/packages/desktop/resources/icon.icois excluded by!**/*.icoapp/packages/desktop/resources/icon.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/1024x1024.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/128x128.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/16x16.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/24x24.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/256x256.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/32x32.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/48x48.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/512x512.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/64x64.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/resources/icons/96x96.pngis excluded by!**/*.png,!**/*.pngapp/packages/desktop/tsconfig.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
Files selected for processing (23)
- README.md (2 hunks)
- app/packages/app/src/components/Analytics.tsx (2 hunks)
- app/packages/app/src/components/Setup.tsx (2 hunks)
- app/packages/app/src/routing/RouterContext.ts (2 hunks)
- app/packages/components/src/components/ExternalLink/ExternalLink.tsx (1 hunks)
- app/packages/looker/src/elements/common/error.ts (2 hunks)
- app/packages/utilities/src/fetch.ts (2 hunks)
- app/packages/utilities/src/index.ts (2 hunks)
- docs/source/cheat_sheets/fiftyone_terminology.rst (1 hunks)
- docs/source/cli/index.rst (5 hunks)
- docs/source/deprecation.rst (1 hunks)
- docs/source/environments/index.rst (1 hunks)
- docs/source/faq/index.rst (4 hunks)
- docs/source/getting_started/install.rst (3 hunks)
- docs/source/user_guide/app.rst (1 hunks)
- docs/source/user_guide/config.rst (3 hunks)
- fiftyone/core/cli.py (11 hunks)
- fiftyone/core/config.py (1 hunks)
- fiftyone/core/service.py (2 hunks)
- fiftyone/core/session/client.py (1 hunks)
- fiftyone/core/session/session.py (18 hunks)
- setup.py (2 hunks)
- tests/intensive/similarity_tests.py (3 hunks)
Files skipped from review due to trivial changes (15)
- README.md
- app/packages/app/src/components/Setup.tsx
- app/packages/components/src/components/ExternalLink/ExternalLink.tsx
- app/packages/utilities/src/index.ts
- docs/source/cheat_sheets/fiftyone_terminology.rst
- docs/source/cli/index.rst
- docs/source/deprecation.rst
- docs/source/environments/index.rst
- docs/source/faq/index.rst
- docs/source/getting_started/install.rst
- docs/source/user_guide/app.rst
- docs/source/user_guide/config.rst
- fiftyone/core/cli.py
- fiftyone/core/session/client.py
- setup.py
Additional context used
Path-based instructions (4)
app/packages/app/src/components/Analytics.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/looker/src/elements/common/error.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/app/src/routing/RouterContext.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/utilities/src/fetch.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (14)
tests/intensive/similarity_tests.py (2)
28-28: Verify the impact of removing thedesktop=Trueargument.Ensure that removing the
desktop=Trueargument from thefo.launch_appfunction call does not adversely affect the test case or the application's behavior.Run the following script to verify the test case:
43-43: Verify the impact of removing thedesktop=Trueargument.Ensure that removing the
desktop=Trueargument from thefo.launch_appfunction call does not adversely affect the test case or the application's behavior.Run the following script to verify the test case:
Verification successful
The removal of the
desktop=Trueargument does not impact the function's behavior.The
launch_appfunction infiftyone/core/session/session.pydoes not include adesktopparameter in its signature, indicating that its removal from the test case does not affect the application's behavior. The function's behavior is controlled by other parameters such asdataset,view,remote, andconfig.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `test_object_similarity` test case. # Test: Run the test case. Expect: The test case should pass without any errors. pytest tests/intensive/similarity_tests.py -s -k test_object_similarityLength of output: 147
Script:
#!/bin/bash # Search for the definition of the `launch_app` function to understand its default behavior. ast-grep --lang python --pattern 'def launch_app($_$$$):'Length of output: 59
Script:
#!/bin/bash # Search for occurrences of `launch_app` to find its definition or import statement. rg 'launch_app' --context 5Length of output: 278083
Script:
#!/bin/bash # Extract the implementation of the `launch_app` function to understand its default behavior. ast-grep --lang python --pattern 'def launch_app($_$$$):' fiftyone/core/session/session.pyLength of output: 92
Script:
#!/bin/bash # Search for the definition of the `launch_app` function in `fiftyone/core/session/session.py`. rg 'def launch_app' -A 20 fiftyone/core/session/session.pyLength of output: 675
app/packages/app/src/components/Analytics.tsx (1)
23-23: Verify the impact of hardcoding thekindproperty to "Web".Ensure that hardcoding the
kindproperty to always be "Web" does not adversely affect the analytics tracking or reporting.Perform the following steps to verify the change:
- Search for all occurrences of the
kindproperty in the codebase.- Analyze how the
kindproperty is used in the analytics tracking and reporting.- Verify that hardcoding the
kindproperty to "Web" does not break any existing functionality or introduce inconsistencies in the analytics data.Run the following script to search for the occurrences of the
kindproperty:Verification successful
Hardcoding the
kindproperty to "Web" inAnalytics.tsxdoes not impact analytics tracking.The change is isolated to the
Analytics.tsxcomponent and does not affect other parts of the codebase related to analytics tracking or reporting. No issues were found with this modification.
- Location:
app/packages/app/src/components/Analytics.tsxat line 23Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of the `kind` property. # Test: Search for the `kind` property. Expect: Occurrences in relevant analytics tracking and reporting code. rg --type typescript $'kind'Length of output: 62
Script:
#!/bin/bash # Description: Search for occurrences of the `kind` property in TypeScript files. # Test: Search for the `kind` property. Expect: Occurrences in relevant analytics tracking and reporting code. rg 'kind' --glob '*.ts' --glob '*.tsx' -A 3Length of output: 445576
app/packages/looker/src/elements/common/error.ts (3)
66-69: LGTM!The code changes are approved.
113-113: LGTM!The code changes are approved.
Line range hint
1-1: LGTM!The removal of the
isElectronfunction is approved.app/packages/app/src/routing/RouterContext.ts (2)
14-14: LGTM!The removal of the
isElectronimport is approved as it aligns with the PR objective of removing the desktop application.
70-70: LGTM, but verify the router behavior.The removal of the Electron environment check in the
createRouterfunction is approved as it aligns with the PR objective of removing the desktop application.However, ensure that the router behaves as expected in different environments (notebook and browser) and that there are no unintended side effects due to this change.
Run the following script to verify the router behavior:
app/packages/utilities/src/fetch.ts (2)
Line range hint
1-1: LGTM!The removal of the
isElectronimport is approved as it aligns with the PR objective of removing the desktop application.
229-234: LGTM!The removal of the Electron environment check in the
getAPIfunction is approved as it aligns with the PR objective of removing the desktop application. The updated logic to handle the browser environment looks good.fiftyone/core/service.py (1)
Line range hint
1-8: LGTM!The removal of the
AppServiceclass is approved as it aligns with the PR objective of removing the desktop application. This is a significant change that indicates a shift in the architecture and service management of the FiftyOne application.fiftyone/core/session/session.py (3)
1027-1028: LGTM!The removal of the
desktopparameter and its associated documentation from thelaunch_appfunction is approved. This change aligns with the deprecation of the desktop app functionality.
1057-1057: LGTM!The removal of the
desktopparameter and the stripping of desktop-related initialization logic from theSessionconstructor is approved. This change aligns with the deprecation of the desktop app functionality, simplifying the constructor to focus on browser-based sessions.
1112-1113: LGTM!The removal of the
desktopproperty from theSessionclass is approved. This change aligns with the deprecation of the desktop app functionality, ensuring that the class no longer exposes any desktop-related properties.
What changes are proposed in this pull request?
Removes the
fiftyone-desktoppackage source, documentation and APIs. As offiftyone==0.25.0, a compatible desktop App is no longer publishedHow is this patch tested? If it is not, please explain why.
Existing coverage
Release Notes
fiftyone-desktopsupport and its corresponding APIsWhat areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores