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.

Sign up for GitHub

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

Jump to bottom

No wait summary field index creation #5128

Closed
wants to merge 1 commit into from
+4 −1
Closed

No wait summary field index creation #5128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion fiftyone/core/dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,7 @@ def create_summary_field(
group_by=None,
read_only=True,
create_index=True,
wait=True,
):
"""Populates a sample-level field that records the unique values or
numeric ranges that appear in the specified field on each sample in
Expand Down Expand Up @@ -1756,6 +1757,8 @@ def create_summary_field(
read_only (True): whether to mark the summary field as read-only
create_index (True): whether to create database index(es) for the
summary field
wait (True): whether to wait for index creation to complete before
returning
Returns:
the summary field name
Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

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.


field = self.get_field(field_name)
field.info[_SUMMARY_FIELD_KEY]["last_modified_at"] = field.created_at
Expand Down
Loading