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

add parsing of string version list of sample ids in _parse_sample_ids #3940

Closed
wants to merge 1 commit into from

Conversation

nebulae
Copy link
Contributor

@nebulae nebulae commented Dec 18, 2023

What changes are proposed in this pull request?

added check & conversion for comma delimited string of sample ids in _parse_sample_ids

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

locally & with unit test

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.

(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?

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

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 48 lines in your changes are missing coverage. Please review.

Comparison is base (c386456) 15.87% compared to head (c83df70) 15.86%.
Report is 3 commits behind head on develop.

Files Patch % Lines
...ore/src/components/Schema/SchemaSelectControls.tsx 0.00% 45 Missing ⚠️
app/packages/core/src/utils/links.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3940      +/-   ##
===========================================
- Coverage    15.87%   15.86%   -0.01%     
===========================================
  Files          731      731              
  Lines        81640    81665      +25     
  Branches      1093     1093              
===========================================
  Hits         12959    12959              
- Misses       68681    68706      +25     
Flag Coverage Δ
app 15.86% <0.00%> (-0.01%) ⬇️

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.

)
expected_stages = [
fosg.SelectGroupSlices(),
fosg.Select(f"{sample_ids[0]},{sample_ids[0]}"),
Copy link
Contributor

@brimoor brimoor Dec 18, 2023

Choose a reason for hiding this comment

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

I don't believe we've publicly documented anywhere that you can pass a comma-separated string list of IDs to Select(). Where did you encounter that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was a bug reported - TEAMS-2062 - the app passes it in as a comma delimited string in some scenarios via the viewbar, as a list of strings in other scenarios. Seemed the right place to fix it rather than identify all the places it could be used as input.

Copy link
Contributor

@brimoor brimoor Dec 20, 2023

Choose a reason for hiding this comment

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

I see. Actually this line:

"type": "list<id>|id",

is supposed to tell the App to parse "list,of,str" into ["list", "of", "str"], so it seems that is no longer working in certain cases? Or some new pathway for view stage parsing was added (didn't look at the ticket) that wasn't aware that it needed to support the type=list<> parameter definition.

cc @benjaminpkane can you help track down the root cause?

The type=list<> syntax is used in other view stages too, so I think that is actually the best place to fix the string parsing inconsistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've included a view bar fix in #3941 with c45be8a as it also contains ObjectId client-side validation

@brimoor
Copy link
Contributor

brimoor commented Jan 16, 2024

Closing as the issue has been resolved by #3940 (comment).

@brimoor brimoor closed this Jan 16, 2024
@brimoor brimoor deleted the trinity/comma_delimited_sample_ids branch January 16, 2024 21:54
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