Skip to content
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

make ctx.view saved view aware for python operators #4200

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

imanjra
Copy link
Contributor

@imanjra imanjra commented Mar 26, 2024

What changes are proposed in this pull request?

make ctx.view saved view aware for python operators

How is this patch tested? If it is not, please explain why.

using an example Python operator that access ctx.view

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

make ctx.view saved view aware for python operators

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

Summary by CodeRabbit

  • New Features
    • Enhanced context management with the introduction of viewName variable, improving data handling and execution context.
    • Updated several functions to utilize the view_name property, refining operations and data fetching.
    • Improved view retrieval process by conditionally loading saved views based on the view_name parameter, ensuring more accurate and efficient data representation.

@imanjra imanjra requested a review from a team March 26, 2024 14:36
Copy link
Contributor

coderabbitai bot commented Mar 26, 2024

Walkthrough

The recent updates focus on enhancing functionality related to the handling of view contexts within an application. Specifically, the changes introduce and utilize a viewName variable across various components and functions to improve the management and retrieval of views. This adjustment aims to refine the application's ability to work with different views, ensuring more dynamic and context-aware operations.

Changes

Files Change Summary
.../operators/src/hooks.ts, .../state.ts Updated to include viewName in context dependencies and selectors, affecting context-related function outputs.
.../operators/src/operators.ts Modified several functions to incorporate view_name from currentContext, enhancing context-aware processing.
fiftyone/operators/executor.py Updated view retrieval logic to conditionally load views based on view_name, and made minor adjustments to JSON serialization.

🐇✨
Changes afoot in the realm of code,
Where viewName now takes the road.
Through hooks and states, it weaves its tale,
In Python lands, it sets the sail.
🌟 In every view, a new sight,
A rabbit's joy, in code done right.
🥕🎉

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 28.85%. Comparing base (7febb86) to head (cc1d3af).
Report is 7 commits behind head on develop.

Files Patch % Lines
app/packages/operators/src/state.ts 0.00% 6 Missing ⚠️
app/packages/operators/src/operators.ts 0.00% 5 Missing ⚠️
app/packages/operators/src/hooks.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop    #4200    +/-   ##
=========================================
  Coverage    28.84%   28.85%            
=========================================
  Files          768      769     +1     
  Lines        97035    97141   +106     
  Branches      1120     1128     +8     
=========================================
+ Hits         27991    28026    +35     
- Misses       69044    69115    +71     
Flag Coverage Δ
app 16.03% <0.00%> (+0.02%) ⬆️
python 99.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 7bf3485 and cc1d3af.
Files selected for processing (4)
  • app/packages/operators/src/hooks.ts (3 hunks)
  • app/packages/operators/src/operators.ts (5 hunks)
  • app/packages/operators/src/state.ts (5 hunks)
  • fiftyone/operators/executor.py (3 hunks)
Additional comments: 9
app/packages/operators/src/hooks.ts (1)
  • 31-31: The addition of viewName to the useOperatorThrottledContextSetter function is correctly implemented. It's properly retrieved using useRecoilValue and included in both the context object and the dependency array of useEffect. This ensures that the context accurately reflects the current state, including the saved view name, aligning with the PR's objectives.

Also applies to: 57-57, 68-68

app/packages/operators/src/state.ts (2)
  • 86-86: The addition of viewName to the globalContextSelector is correctly implemented. It's properly retrieved using get(fos.viewName) and included in the returned context object. This ensures that the global context accurately reflects the current state, including the saved view name, aligning with the PR's objectives.

Also applies to: 96-96

  • 126-126: The addition of viewName to the useExecutionContext function is correctly implemented. It's properly included in the execution context passed to operators, ensuring that operators can interact intelligently with saved views. This aligns with the PR's objectives and enhances the application's functionality.

Also applies to: 139-139, 152-153

app/packages/operators/src/operators.ts (5)
  • 446-446: The addition of view_name to the request payload in the executeOperatorAsGenerator function is correctly implemented. It ensures that the server-side execution of operators can consider the saved view name, aligning with the PR's objectives and enhancing the application's functionality.
  • 546-546: The addition of view_name to the request payload in the executeOperatorWithContext function is correctly implemented. It ensures that the execution context passed to operators includes the saved view name, aligning with the PR's objectives and enhancing the application's functionality.
  • 613-613: The addition of view_name to the request payload in the resolveRemoteType function is correctly implemented. It ensures that the type resolution process can consider the saved view name, aligning with the PR's objectives and enhancing the application's functionality.
  • 681-681: The addition of view_name to the request payload in the resolveExecutionOptions function is correctly implemented. It ensures that the execution options resolution process can consider the saved view name, aligning with the PR's objectives and enhancing the application's functionality.
  • 710-710: The addition of view_name to the request payload in the fetchRemotePlacements function is correctly implemented. It ensures that the remote placements fetching process can consider the saved view name, aligning with the PR's objectives and enhancing the application's functionality.
fiftyone/operators/executor.py (1)
  • 507-524: The implementation for resolving the view based on view_name or other parameters is clear and seems correct. However, it's important to ensure that the view_name is sanitized and validated before it's used to load a saved view to prevent potential security issues, such as injection attacks or unauthorized access to views.

Ensure that view_name is properly sanitized and validated before use. If this is handled upstream, please confirm.

@brimoor brimoor mentioned this pull request Mar 28, 2024
7 tasks
Copy link
Contributor

@swheaton swheaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worked in resolve_input(), execute(), and resolve_output() for me.

@imanjra imanjra merged commit e688343 into develop Apr 3, 2024
16 checks passed
@imanjra imanjra deleted the feat/ctx-view-know-saved branch April 3, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants