-
Notifications
You must be signed in to change notification settings - Fork 563
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
No wait summary field index creation #5128
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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
|
bafbc51
to
4a4b2d3
Compare
4a4b2d3
to
d79b934
Compare
@@ -1898,7 +1901,7 @@ def create_summary_field( | |||
|
|||
if create_index: | |||
for _field_name in index_fields: | |||
self.create_index(_field_name) | |||
self.create_index(_field_name, wait=wait) |
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.
Have you benchmarked the cost of create_index()
vs _populate_summary_field()
? Note that we create the index before populating the field here.
I bet running the aggregation to populate the field takes longer, which would mean that create_summary_field(wait=False)
would not actually return fast.
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.
That's my fear too; I hope that at least setting wait=False
will allow the execution of this function to terminate a little bit earlier...
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.
Probably the right approach is allowing the users to schedule summary field operations, but it's currently out of scope for v2.2.0; one thing we can do from UI/UX side is to add a user warning to summary field operations so at least the users are aware of it.
What changes are proposed in this pull request?
Added a param (
wait
) to pass to index creation for summary fieldsHow is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyone
Python library changes