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

[Proposal] add util to execution context with a set_view util #4163

Closed
wants to merge 1 commit into from

Conversation

imanjra
Copy link
Contributor

@imanjra imanjra commented Mar 15, 2024

What changes are proposed in this pull request?

Add a util to execution context with a util to set_view without the need to serialize in operator. Altough, the utili is only being added with set_view, the idea is this would contain common utilities that can be reusable.

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

Using a test operator

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.

Add a util to execution context with a util to set_view without the need to serialize in operator.

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

@imanjra imanjra requested review from ritch, brimoor and a team March 15, 2024 13:07
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.03%. Comparing base (aecd864) to head (8f363f4).
Report is 18 commits behind head on release/v0.23.7.

Additional details and impacted files
@@               Coverage Diff                @@
##           release/v0.23.7    #4163   +/-   ##
================================================
  Coverage            16.03%   16.03%           
================================================
  Files                  733      733           
  Lines                82012    82012           
  Branches              1118     1118           
================================================
  Hits                 13153    13153           
  Misses               68859    68859           
Flag Coverage Δ
app 16.03% <ø> (ø)

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.

@imanjra imanjra force-pushed the feat/ctx-set-view branch from 7b97c06 to 8f363f4 Compare March 15, 2024 13:14
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

Having this utility available will be great!

@imanjra @ritch curious what you're thinking about making this (and other utils as Ibrahim points out) available as ctx.set_view() versus ctx.utils.set_view().

#4076 added ctx.target_view() directly to the ExecutionContext and it seems like we should standardize on a single pattern for this.

@ritch
Copy link
Contributor

ritch commented Mar 15, 2024

I had thought about this before and I feel strongly that it should be ctx.ops.set_view(). ops would be an instance of ContextualOperations (or similar name). The idea is that each method on that class is syntactic sugar for calling a built in operator. This would be the mechanism that we would document built in operators.

With that in mind I would suggest we finish/merge this instead: #4055

seems like we should standardize on a single pattern for this.

The concept for ExecutionContext was to be contextual metadata/references: eg dataset, view. I think target_view is consistent with that pattern. I think we should separate out the operations because that list could get quite long.

@imanjra
Copy link
Contributor Author

imanjra commented Mar 15, 2024

Oh, cool. Didn't realize that existed. Sounds good. I will close this in favour of #4055 and continue there.

@imanjra imanjra closed this Mar 15, 2024
@imanjra imanjra deleted the feat/ctx-set-view branch March 15, 2024 21:51
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.

3 participants