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

Bring back ODMSample base class #127

Closed
brimoor opened this issue Jun 1, 2020 · 3 comments
Closed

Bring back ODMSample base class #127

brimoor opened this issue Jun 1, 2020 · 3 comments
Assignees
Labels
backlog Issues related to the roadmap and feature backlog

Comments

@brimoor
Copy link
Contributor

brimoor commented Jun 1, 2020

#117 went out a bit quick, but, upon closer inspection, I think we should not have deleted fiftyone.core.odm.ODMDatasetSample.

The current implementation has only fiftyone.core.odm.ODMSample and fiftyone.core.odm.NoDatasetSample, where the latter inherits directly from SerializableDocument:

class NoDatasetSample(SerializableDocument):

I think there's still value in having the following hierarchy:

fiftyone.core.odm.ODMSample
    fiftyone.core.odm.ODMDatasetSample
    fiftyone.core.odm.ODMNoDatasetSample

where the base class fiftyone.core.odm.ODMSample defines the interface that all samples support. This will make it more clear what fiftyone.core.sample.Sample is allowed to do with its backing documents, for example.

Although NoDatasetSample is now completely home brewed (no MongoEngine), it is still a "document" in the sense that it is a JSON serializable representation of a Sample. So I see no issue with using the ODMNoDatasetSample name. It is in the odm package, after all.

@brimoor brimoor added the enhancement Code enhancement label Jun 1, 2020
@tylerganter
Copy link
Contributor

I'm okay with this. The inheritance is ultimately messy no matter what we do, but I suppose this would be a bit cleaner.

@brimoor
Copy link
Contributor Author

brimoor commented Jun 1, 2020

These definitions on NoDatasetSample are especially perplexing without having an ODMSample superclass that explains why they have to be there and return None

@property
def dataset_name(self):
return None
@property
def id(self):
return None
@property
def ingest_time(self):
return None
@property
def in_db(self):
return None

@brimoor brimoor added backlog Issues related to the roadmap and feature backlog and removed enhancement Code enhancement labels Jun 1, 2020
@brimoor
Copy link
Contributor Author

brimoor commented Jun 5, 2020

This was implemented in #156

@brimoor brimoor closed this as completed Jun 5, 2020
kaixi-wang pushed a commit that referenced this issue May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues related to the roadmap and feature backlog
Projects
None yet
Development

No branches or pull requests

2 participants