-
Notifications
You must be signed in to change notification settings - Fork 68
New methods to manage feature schemas and ontologies #940
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
Conversation
| ) | ||
|
|
||
|
|
||
| def test_deletes_a_feature_schema(client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to not use fixtures for tests, because the feature schema class does not have a "deletable" interface. Schema nodes have a "deleted" property, but we can't create such fixtures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot better! Thanks for adding all of the tests too.
Left one comment, and also, it'd be great if you could add return types for functions. For "delete" functions, you can add None as return type.
Also, the build is failing. Could you try rebasing the latest develop branch and try the build again? There were some recent fixes to export related tests, and I wonder if your branch is stale
labelbox/client.py
Outdated
| "Failed to update the feature schema, message: " + | ||
| str(response.json()['message'])) | ||
|
|
||
| def upsert_feature_schema(self, normalized: Dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Could you name it something more descriptive than
normalized? I think what you had before was fine:feature_schema. Now it's more obvious what type of data is needed, with the type hint. Let's also add a description of what this dictionary should be, in the docstring:
feature_schema: Dict representing the feature schema to upsert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, here are the changes 56583b5
labelbox/client.py
Outdated
| "Failed to delete the ontology, message: " + | ||
| str(response.json()['message'])) | ||
|
|
||
| def update_feature_schema_title(self, feature_schema_id: str, title: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
labelbox/client.py
Outdated
| "Failed to update the feature schema, message: " + | ||
| str(response.json()['message'])) | ||
|
|
||
| def upsert_feature_schema(self, feature_schema: Dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kkim-labelbox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
https://labelbox.atlassian.net/browse/PTDT-1109
https://labelbox.atlassian.net/browse/PTDT-1110
https://labelbox.atlassian.net/browse/PTDT-1112
https://labelbox.atlassian.net/browse/PTDT-1113
https://labelbox.atlassian.net/browse/PTDT-1131
https://labelbox.atlassian.net/browse/PTDT-1132
Introduced 7 new methods: