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

Addressing multiple PCD overlay feedback #2964

Merged
merged 21 commits into from
May 9, 2023

Conversation

benjaminpkane
Copy link
Contributor

@benjaminpkane benjaminpkane commented May 5, 2023

Resolves comments 1, 3, 4, and 5 from #2912 (comment)

@benjaminpkane benjaminpkane requested a review from brimoor May 5, 2023 16:07
@benjaminpkane benjaminpkane self-assigned this May 5, 2023
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Patch coverage: 66.47% and project coverage change: +0.37 🎉

Comparison is base (55da3a0) 61.80% compared to head (69e1f5e) 62.18%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2964      +/-   ##
===========================================
+ Coverage    61.80%   62.18%   +0.37%     
===========================================
  Files          248      250       +2     
  Lines        44705    45507     +802     
  Branches       317      319       +2     
===========================================
+ Hits         27631    28297     +666     
- Misses       17074    17210     +136     
Flag Coverage Δ
app 48.51% <66.47%> (+0.55%) ⬆️
python 99.41% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...ges/components/src/components/IconButton/index.tsx 100.00% <ø> (ø)
...ackages/components/src/components/Popout/index.tsx 33.33% <0.00%> (ø)
app/packages/core/src/components/Actions/utils.tsx 40.57% <0.00%> (ø)
app/packages/looker/src/elements/common/looker.ts 9.24% <ø> (+0.07%) ⬆️
app/packages/looker/src/elements/video.ts 9.90% <0.00%> (ø)
app/packages/looker/src/overlays/heatmap.ts 16.06% <0.00%> (ø)
...ated__/paginateDynamicGroupSamplesQuery.graphql.ts 100.00% <ø> (ø)
...es/__generated__/paginateGroupPageQuery.graphql.ts 100.00% <ø> (ø)
...ueries/__generated__/paginateGroupQuery.graphql.ts 100.00% <ø> (ø)
...rc/queries/__generated__/pcdSampleQuery.graphql.ts 100.00% <ø> (ø)
... and 56 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

I see the code changes you made here on github, but when I build + test locally, I'm not seeing the changes for some reason... is it user error?
Screen Shot 2023-05-05 at 4 30 27 PM

Also now the sample tags entry in the modal sidebar is completely missing:
Screen Shot 2023-05-05 at 4 30 40 PM

Also, unrelated but just reporting as I see things: when testing with this dataset:

import fiftyone as fo
import fiftyone.zoo as foz

dataset = fo.Dataset()
qsg = foz.load_zoo_dataset("quickstart-groups", max_samples=12)

qsg_pngs = qsg.select_group_slices("left").clone()
qsg_pcds = qsg.select_group_slices("pcd").clone()

ground_truth_label = qsg_pcds.first()["ground_truth"]

g1 = fo.Group()
g1s1 = fo.Sample(filepath=qsg_pcds.first().filepath, group=g1.element("pcd1"))
g1s1["detections"] = ground_truth_label
g1s2 = fo.Sample(filepath=qsg_pcds.skip(1).first().filepath, group=g1.element("pcd2"))
g1s3 = fo.Sample(filepath=qsg_pngs.first().filepath, group=g1.element("png"))

g2 = fo.Group()
g2s1 = fo.Sample(filepath=qsg_pcds.skip(2).first().filepath, group=g2.element("pcd1"))
g2s1["detections"] = ground_truth_label
g2s2 = fo.Sample(filepath=qsg_pcds.skip(3).first().filepath, group=g2.element("pcd2"))
g2s3 = fo.Sample(filepath=qsg_pngs.skip(1).first().filepath, group=g2.element("png"))

samples = [g1s1, g1s2, g1s3, g2s1, g2s2, g2s3]

dataset.add_samples(samples)
dataset.default_group_slice = "png"

session = fo.launch_app(dataset)

I get the following error when using the tagging menu above the grid to add some tags with one or more samples selected:

Traceback (most recent call last):
  File "/Users/Brian/dev/fiftyone/fiftyone/server/decorators.py", line 34, in wrapper
    response = await func(endpoint, request, data, *args)
  File "/Users/Brian/dev/fiftyone/fiftyone/server/routes/tagging.py", line 64, in post
    tags, items = await view._async_aggregate(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 8827, in _async_aggregate
    compiled_facet_aggs, facet_pipelines = self._build_facets(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 8929, in _build_facets
    field = self.get_field(root)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 1025, in get_field
    _, field = self._parse_field(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 1058, in _parse_field
    schema = self.get_field_schema(include_private=include_private)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 692, in get_field_schema
    schema = self._get_filtered_schema(schema)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1433, in _get_filtered_schema
    selected_fields, excluded_fields = self._get_selected_excluded_fields(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1468, in _get_selected_excluded_fields
    _view = _view._add_view_stage(stage, validate=False)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1412, in _add_view_stage
    media_type = stage.get_media_type(self)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/stages.py", line 3998, in get_media_type
    media_types = set(group_media_types.values())
AttributeError: 'NoneType' object has no attribute 'values'

I guess this means that the App is trying to append a SelectGroupSlices stage to a view that's not grouped

@brimoor
Copy link
Contributor

brimoor commented May 8, 2023

@benjaminpkane I merged develop into this branch and now I see the following error when clicking on the tags action on any dataset (eg an image dataset).

I suspect this isn't related to this PR, but I'm just using this PR as a place to report it 😅

Network Error

Code
500

Route
http://localhost:5151/tagging

Payload
{
  "dataset": "malaria-cell-images",
  "filters": {},
  "modal": false,
  "view": [],
  "label_fields": [
    "ground_truth",
    "predictions"
  ],
  "target_labels": false,
  "slices": [
    null
  ],
  "group_id": null,
  "sample_ids": null,
  "labels": null,
  "hidden_labels": null
}

Trace
Traceback (most recent call last):
  File "/Users/Brian/dev/fiftyone/fiftyone/server/decorators.py", line 34, in wrapper
    response = await func(endpoint, request, data, *args)
  File "/Users/Brian/dev/fiftyone/fiftyone/server/routes/tagging.py", line 38, in post
    view = fost.get_tag_view(
  File "/Users/Brian/dev/fiftyone/fiftyone/server/tags.py", line 30, in get_tag_view
    view = fosv.get_view(
  File "/Users/Brian/dev/fiftyone/fiftyone/server/view.py", line 120, in get_view
    view = view.select_group_slices(sample_filter.group.slices)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 5750, in select_group_slices
    return self._add_view_stage(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1404, in _add_view_stage
    stage.validate(self)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/stages.py", line 4291, in validate
    raise ValueError("%s has no groups" % type(sample_collection))
ValueError: <class 'fiftyone.core.view.DatasetView'> has no groups

@benjaminpkane
Copy link
Contributor Author

benjaminpkane commented May 8, 2023

Regarding

Traceback (most recent call last):
  File "/Users/Brian/dev/fiftyone/fiftyone/server/decorators.py", line 34, in wrapper
    response = await func(endpoint, request, data, *args)
  File "/Users/Brian/dev/fiftyone/fiftyone/server/routes/tagging.py", line 64, in post
    tags, items = await view._async_aggregate(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 8827, in _async_aggregate
    compiled_facet_aggs, facet_pipelines = self._build_facets(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 8929, in _build_facets
    field = self.get_field(root)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 1025, in get_field
    _, field = self._parse_field(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/collections.py", line 1058, in _parse_field
    schema = self.get_field_schema(include_private=include_private)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 692, in get_field_schema
    schema = self._get_filtered_schema(schema)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1433, in _get_filtered_schema
    selected_fields, excluded_fields = self._get_selected_excluded_fields(
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1468, in _get_selected_excluded_fields
    _view = _view._add_view_stage(stage, validate=False)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/view.py", line 1412, in _add_view_stage
    media_type = stage.get_media_type(self)
  File "/Users/Brian/dev/fiftyone/fiftyone/core/stages.py", line 3998, in get_media_type
    media_types = set(group_media_types.values())
AttributeError: 'NoneType' object has no attribute 'values'

This has something to do with how th media type is computed on views. Perhaps it has changed recently. make_optimized_select_view is now broken for group datasets

import fiftyone.core.view as fov
import fiftyone.zoo as foz

dataset = foz.load_zoo_dataset("quickstart-groups")
view = dataset.select_group_slices("png")
assert view.media_type == "image" # is image, correct
view = fov.make_optimized_select_view(view, [dataset.first().id])
assert view.media_type == "image" # should be image, is group

@sashankaryal
Copy link
Contributor

Re, comment (2):

Should we instead remember the slices that are currently being shown, for consistency with how everything else (I believe) is persisted?

I attempted this but this is not as easy to persist, unfortunately. I'll wait until useStateUpdate() refactoring is done; I believe @benjaminpkane is working on it. This has to do with how app state gets reset on page refresh or dataset changes.

Copy link
Contributor

@sashankaryal sashankaryal left a comment

Choose a reason for hiding this comment

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

lgtm, this pr also fixes the aggregation regressions

@brimoor brimoor changed the title Multiple PCD Comments Addressing multiple PCD overlay feedback May 9, 2023
@brimoor
Copy link
Contributor

brimoor commented May 9, 2023

@benjaminpkane thanks for tracking down the tagging issue. I updated make_optimized_select_view() in such a way that the issue is resolved. FWIW I don't think this was caused by a backend change 🤷

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.

LGTM

@benjaminpkane benjaminpkane merged commit 327789b into develop May 9, 2023
@benjaminpkane benjaminpkane deleted the feat/multiple-pcd-overlay branch May 9, 2023 13:49
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