Skip to content

Conversation

@msokoloff1
Copy link
Contributor

No description provided.

Copy link
Contributor

@awu-labelbox awu-labelbox left a comment

Choose a reason for hiding this comment

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

Lgtm, I only saw non-functionality related comments

Comment on lines +134 to +137
data = json.dumps({
'query': query,
'variables': params
}).encode('utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

If params is None, would it set the value of variables to undefined? Is that OK here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this should be fine. Here is a minimal example you can try that works without setting params.

from labelbox import Client
client = Client()
client.execute(""" query { organization {id} }""")

Copy link
Contributor

@awu-labelbox awu-labelbox left a comment

Choose a reason for hiding this comment

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

Lgtm -- just a reminder to remove/stub out local API_KEY and ENDPOINT in the notebook.
Thanks for making the changes!

@msokoloff1 msokoloff1 requested a review from GrantEating June 9, 2021 17:58
logger = logging.getLogger(__name__)


class AnnotationImport(DbObject):
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! glad this name made its way into the SDK, also.


@classmethod
def _build_import_predictions_query(cls, file_args: str, vars: str):
query_str = """mutation testPredictionImportsPyApi($parent_id : ID!, $name: String!, $predictionType : PredictionType!, %s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be called testPredictionImportsPyPyi still? Maybe ModelErrorAnalysisPredictionImport?

Comment on lines 134 to 136
%s : $parent_id
name: $name
%s
Copy link
Contributor

@GrantEating GrantEating Jun 9, 2021

Choose a reason for hiding this comment

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

I feel like its not a big deal, but i think these could be indented (and below)

return cls._create_from_objects(client, model_run_id, name, predictions)


class MALPredictionImport(AnnotationImport):
Copy link
Contributor

Choose a reason for hiding this comment

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

Im curious why we need this? I dont think were going to be allowing MAL imports via SDK through this mutation for a little while.

class BulkImportRequestState(Enum):
""" State of the import job when importing annotations (RUNNING, FAILED, or FINISHED).
This object is deprecated. Please use PredictionImportState instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean AnnotationImportState.

FINISHED = "FINISHED"


class ImportType(Enum):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish I'd called this variable ImportType rather than prediction type everywhere :(

@GrantEating GrantEating self-requested a review June 9, 2021 18:39
Copy link
Contributor

@GrantEating GrantEating left a comment

Choose a reason for hiding this comment

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

I left some nits but looks amazing to me.

@msokoloff1 msokoloff1 merged commit c242a1e into mea-dev Jun 10, 2021
@msokoloff1 msokoloff1 deleted the ms/annotation-import branch July 21, 2021 14:14
msokoloff1 added a commit that referenced this pull request Sep 22, 2021
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.

5 participants