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

Convert tags to a SetField #80

Open
brimoor opened this issue May 20, 2020 · 5 comments
Open

Convert tags to a SetField #80

brimoor opened this issue May 20, 2020 · 5 comments
Assignees
Labels
feature Work on a feature request

Comments

@brimoor
Copy link
Contributor

brimoor commented May 20, 2020

It would be preferable for tags to be represented as sets in Python land. See below for discussion.

Also, this would be a convenient time to create our own wrappers around MongoEngine fields so that our user interface is decoupled from MongoEngine classes:

Screen Shot 2020-05-20 at 2 30 47 PM

@brimoor brimoor added the feature Work on a feature request label May 20, 2020
@tylerganter tylerganter self-assigned this May 20, 2020
@tylerganter
Copy link
Contributor

I made a PR into MongoEngine. We shall see how it goes

MongoEngine/mongoengine#2337

@brimoor
Copy link
Contributor Author

brimoor commented May 27, 2020

Can we support this directly in FiftyOne for the time being? It could internally use MongoEngine's ListField and just convert to sets at the object-level.

I have a suspicion that it won't be easy to get your PR into MongoEngine, and even then it will take time to get this into an official release so that pip-installable FiftyOne can use it

@tylerganter
Copy link
Contributor

The core reason that I did this in MongoEngine and not fiftyone is that there needs to be a notion of a BaseSet that watches modifying operations and notifies MongoEngine. If we go the route of supporting this just with 51, which we could, we'd have to change the interface for tags, similar to this add_tag and remove_tag idea which would break from the behavior of all other fields.

Can the user no longer access the sample.tags? Only sample.get_tags()?

I am working on database synchronization right now. I can keep this in the back of my mind while I do that since it is all related.

@brimoor
Copy link
Contributor Author

brimoor commented May 27, 2020

Hmm sounds like this issue will likely have to go into hibernation then. No easy path forward, and it's not THAT big of an inconvenience to use lists

@tylerganter
Copy link
Contributor

I'll keep an eye on this issue. But the easy alternatives don't seem much better than leaving it as a list

benjaminpkane added a commit that referenced this issue Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Work on a feature request
Projects
None yet
Development

No branches or pull requests

2 participants