V0.5 #25

srikanth-scale · 2021-01-22T21:04:00Z

Working on new python API please suggest docs/improvements/cleanups

ty

srikanth-scale · 2021-01-22T21:07:10Z

Working well, tested locally but extremely ugly -> @Nastia39 need your suggestions to cleanup everything

nucleus/__init__.py

sasha-scale

Stylistically it looks pretty good to me, I haven't had a chance to check out and test yet, but I will absolutely do so later today

.vscode/settings.json

nucleus/annotation.py

nucleus/dataset.py

nucleus/errors.py

nucleus/payload_constructor.py

sasha-scale

Tested locally, was pretty intuitive to figure out.

An additional improvement is that for items upload the user must convert to json to see the result:

response.json()

Whereas for annotations/predictions you simply do print(response). It would be great to standardize this.

Will also have to think about how to communicate to existing users that they need to install this update

sasha-scale · 2021-01-27T00:51:53Z

nucleus/dataset_item.py

+
+class DatasetItem:
+
+    def __init__(self, image_location: str, reference_id: str, metadata: dict):


should make metadata field optional. Currently not optional because it doesn't have a default value

sasha-scale · 2021-01-27T00:54:39Z

nucleus/dataset_item.py

+            "image_url": self.image_url,
+            "reference_id": self.reference_id,
+            "metadata": self.metadata
+        }


nit need additional newline at bottom of file

sasha-scale · 2021-01-27T00:57:45Z

nucleus/dataset_item.py

+        path_components = path.split('/')
+        return not ('https:' in path_components  or 'http:' in path_components or 's3:' in path_components)
+
+    def _local_file_exists(self, path):


lets add a str or repr method so that when a user prints(DatasetItem) they get a nice string representation instead of <nucleus.dataset_item.DatasetItem object at 0x11b016748>

I think just doing:
def repr(self):
return str(self.to_payload)

would be sufficient

nucleus/__init__.py

… v0.5

nucleus/dataset_item.py

sasha-scale · 2021-01-29T22:59:15Z

nucleus/model.py

+        self.metadata = metadata
+        self._client = client
+
+    def create_run(self, name: str, metadata: dict, dataset: Dataset, predictions: List[BoxPrediction]) -> ModelRun:


prob can delete this method?

sasha-scale · 2021-01-29T22:59:51Z

nucleus/model_run.py

        return self._client.commit_model_run(self.model_run_id, payload)

-    def predict(self, payload: dict) -> dict:
+    def predict(self, annotations: List[Any]) -> dict:


@srikanth-scale FYI for docs, updated this method to take list of 2DBoxPredictions instead of raw JSON payload.

annotations: List[Union[Box2DPrediction, PolygonPrediction]]

… v0.5

Nastia39 · 2021-02-02T22:36:41Z

nucleus/dataset_item.py

+    def _is_local_path(self, path: str) -> bool:
+        path_components = [comp.lower() for comp in path.split("/")]
+        return not (
+            "https:" in path_components
+            or "http:" in path_components
+            or "s3:" in path_components
+        )
+
+    def _local_file_exists(self, path: str):
+        return os.path.isfile(path)


I think, there methods should be moved to utils file as there aren't a part of DatasetItem abstraction

Nastia39 · 2021-02-02T22:37:42Z

nucleus/model.py

+
+class Model:
+
+    def __init__(self, model_id: str, name: str, reference_id: str, metadata: dict, client):


metadata: Optional[dict]

Nastia39 · 2021-02-02T22:39:01Z

nucleus/model_run.py

        return self._client.commit_model_run(self.model_run_id, payload)

-    def predict(self, payload: dict) -> dict:
+    def predict(self, annotations: List[Any]) -> dict:


annotations: List[Union[Box2DPrediction, PolygonPrediction]]

Nastia39 · 2021-02-02T22:41:06Z

nucleus/dataset.py


-    def annotate(self, payload: dict) -> dict:
+    def annotate(
+        self, annotations: List[BoxAnnotation], batch_size=20


nit: batch_size: int = 20

Nastia39 · 2021-02-02T22:42:07Z

nucleus/dataset.py

-        return self._client.dataset_info(self.dataset_id)
+        return self._client.dataset_info(self.id)

    def create_model_run(self, payload: dict):


change this method to a new style as well

Nastia39 · 2021-02-02T22:43:09Z

nucleus/annotation.py

+        y: int,
+        width: int,
+        height: int,
+        metadata: dict = None,


metadata: Optional[dict] = None

Also add class for PolygonAnnotation

Nastia39 · 2021-02-02T22:47:02Z

nucleus/__init__.py

            "reference_id": str,
            "metadata": Dict[str, Any],
        }
        :return: { "model_id": str }


into v0.5

jihan-yin

Left some nits

jihan-yin · 2021-02-04T23:34:27Z

README.md

 ```python
-datasetItem1 = {"image_url": "http://<my_image_url>", "reference_id": "my_image_name.jpg",
-  "metadata": {"label": "0"}}
+dataset_item_1 = DatasetItem(image_location="./1.jpeg", reference_id="1", metadata={"key": "value"})


We should still explain or at least point out the three necessary fields necessary for a DatasetItem

jihan-yin · 2021-02-05T01:19:45Z

nucleus/__init__.py

        Creates a new dataset based on payload params:
        name -- A human-readable name of the dataset.
        Returns a response with internal id and name for a new dataset.
        :param payload: { "name": str }


Should update comment

jihan-yin · 2021-02-05T01:21:17Z

nucleus/__init__.py

    def populate_dataset(
        self,
        dataset_id: str,
-        payload: dict,


same thing about updating the function doc

I'm a little confused though on why we were using payload in the first place, instead of just passing a bunch of kwargs, especially in this case where we have both kwargs and payload. The usage seems kinda inconsistent too across this file

nucleus/annotation.py

jihan-yin · 2021-02-05T01:27:24Z

nucleus/annotation.py

+        item_id: str = None,
+        metadata: Optional[Dict] = None,
+    ):
+        if bool(reference_id) == bool(item_id):


Would there be a case of both reference and item id being populated? In which case the exception error may not be as clear. Also why don't we just check if either is equal to None?

As things stand now we actually require that exactly one of these fields be defined at time of upload. I agree this is pretty suboptimal, I think in the future we should remove this and implement precendence between the two ids instead :)

i.e. reference_id trumps dataset_item_id

jihan-yin · 2021-02-05T01:33:12Z

nucleus/model.py

+
+        model_run.predict(predictions)
+
+        return model_run


Why don't we set the new model_run as a prop of the model class? Or at least have a way for the model instance to track the different runs that have been committed

I'm worried that adding a model_run as a property will be a bit misleading, because we don't automatically pull in all of the model_runs associated with this model (other runs could have been created previously). I think the problem you're pointing out here is super valid though: how does a user fetch all model_runs associated with a model? In the future we should add API endpoints for:

get_model

delete model

get_model_runs for model

jihan-yin · 2021-02-05T01:59:59Z

nucleus/prediction.py

+    def __init__(
+        self,
+        label: str,
+        vertices: List[Any],


Do we not have a type for generic 2d points?

not yet lol, polygon annotation was a bit of an afterthought on this PR, unfortunately. Left it as a TODO for the future :) good suggestion!

… etc.

sasha-scale

Let's ship it.

Nastia39 · 2021-02-05T18:33:22Z

nucleus/model_run.py

-    def iloc(self, i: int) -> dict:
+    def iloc(
+        self, i: int
+    ):  # TODO -> List[Union[BoxPrediction, PolygonPrediction]]:


I added this to fix typing issue, has to be removed

Nastia39 · 2021-02-05T18:33:31Z

nucleus/model_run.py

-    def refloc(self, reference_id: str) -> dict:
+    def refloc(
+        self, reference_id: str
+    ):  # TODO -> List[Union[BoxPrediction, PolygonPrediction]]:


I added this to fix typing issue, has to be removed [2]

srikanth-scale added 2 commits January 17, 2021 18:56

initial commit

5b18ff5

added new get dataset items abstraction

27d901f

srikanth-scale requested review from Nastia39, rkaplan and sasha-scale January 22, 2021 21:04

typo

4ad4b76

rkaplan reviewed Jan 23, 2021

View reviewed changes

nucleus/__init__.py Outdated Show resolved Hide resolved

rkaplan reviewed Jan 23, 2021

View reviewed changes

nucleus/__init__.py Outdated Show resolved Hide resolved

sasha-scale suggested changes Jan 25, 2021

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

nucleus/annotation.py Outdated Show resolved Hide resolved

nucleus/dataset.py Outdated Show resolved Hide resolved

nucleus/errors.py Show resolved Hide resolved

nucleus/payload_constructor.py Outdated Show resolved Hide resolved

sasha-scale reviewed Jan 27, 2021

View reviewed changes

sasha-scale and others added 9 commits January 28, 2021 10:52

Merge branch 'master' into v0.5

60e0d26

addressed sashas comments

62c041b

Merge branch 'v0.5' of github.com:scaleapi/nucleus-python-client into…

6b2d4d8

… v0.5

nits

dbe9040

resolve merge

2854f8d

making strides toward better error handling

2778d12

remove some more json arguments

aec7002

cleanup

a3e3f06

add string representations

dbcbf32

sasha-scale reviewed Jan 29, 2021

View reviewed changes

nucleus/dataset_item.py Show resolved Hide resolved

sasha-scale reviewed Jan 29, 2021

View reviewed changes

sasha-scale and others added 4 commits January 29, 2021 18:41

disable warnings from connection pool

58c9b75

nits

f6f7a88

added new README.md

fc6a8bf

Merge branch 'v0.5' of github.com:scaleapi/nucleus-python-client into…

6b62e87

… v0.5

Nastia39 suggested changes Feb 2, 2021

View reviewed changes

sasha-scale added 2 commits February 3, 2021 15:36

batching for local uploads

140f729

improve interface for create_model_run

0d01cf8

finish local upload in batch

01bf67b

sasha-scale requested a review from jihan-yin February 4, 2021 21:37

sasha-scale and others added 7 commits February 4, 2021 15:14

add full support for polygon predictions/annotations

9407ca9

add full support for polygon predictions/annotations

8e2e298

cleanup 2

4171c0b

typing fixes

9c2f178

little cleanups

7a22f5f

fomratting fix

8825f54

Merge branch 'v0.5' of https://github.com/scaleapi/nucleus-python-client

54a89a2

into v0.5

jihan-yin reviewed Feb 5, 2021

View reviewed changes

Nastia39 and others added 10 commits February 4, 2021 18:06

upd docstrings

a896bc8

typo

5ddd300

static method

beae5d3

item_id wasn't used

5adeaa3

fun w/ constants

23a6425

add support for creating a dataset from a Scale annotation project

d251d43

corrections/additions to match documentation (fixing optional params,…

7e02d98

… etc.

nuke string literals

3473088

typing

cce8902

nuke the references :)

6e66c2d

sasha-scale approved these changes Feb 5, 2021

View reviewed changes

Nastia39 suggested changes Feb 5, 2021

View reviewed changes

cleanup TODOs

4e4c96a

Nastia39 merged commit ecda3c9 into master Feb 5, 2021


		class DatasetItem:

		def __init__(self, image_location: str, reference_id: str, metadata: dict):


		class Model:

		def __init__(self, model_id: str, name: str, reference_id: str, metadata: dict, client):

V0.5 #25

V0.5 #25

Uh oh!

Conversation

srikanth-scale commented Jan 22, 2021

Uh oh!

srikanth-scale commented Jan 22, 2021

Uh oh!

Uh oh!

Uh oh!

sasha-scale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sasha-scale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihan-yin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sasha-scale left a comment

Choose a reason for hiding this comment

Uh oh!

Nastia39 Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Nastia39 Feb 5, 2021 •

edited

Loading

Nastia39 Feb 5, 2021 •

edited

Loading