search

roboflow / roboflow-python

The official Roboflow Python package. Manage your datasets, models, and deployments. Roboflow has everything you need to build a computer vision application.
https://docs.roboflow.com/python
Apache License 2.0
271 stars 70 forks source link

Mypy error: `local` definition is confuse #271

Closed iurisilvio closed 1 month ago

iurisilvio commented 2 months ago

local is defined as bool but later is used as base api url.

Mypy errors that highlighted the issue:

roboflow/models/instance_segmentation.py:19: error: Incompatible default for argument "local" (default has type "None", argument has type "bool")  [assignment]
roboflow/models/instance_segmentation.py:19: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
roboflow/models/instance_segmentation.py:19: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
roboflow/models/classification.py:64: error: Incompatible types in assignment (expression has type "Literal[True]", variable has type "str")  [assignment]
iurisilvio commented 1 month ago

@capjamesg I see you added typing to local in 4d863a24fde0722f41d895e7c4dc08490bfa565b.

Do you know how these local flags are used? I don't know exactly what change I should do here, maybe just change to local: Optional[str].

capjamesg commented 1 month ago

I think it should beOptional[str]. Here's an example of documentation where this function is used, and the type is a str: https://docs.roboflow.com/api-reference/inference#local-deploy-inference

capjamesg commented 1 month ago

The original intent for the function may have been to trigger between two modes, but then changed to support a URL since someone may choose to run Inference locally on a non-standard port?

iurisilvio commented 1 month ago

Hum.. makes sense! Maybe it should be str | bool and for True we set api to localhost. I'll dig deeper in old commits to understand.

iurisilvio commented 1 month ago

I found initial implementation. https://github.com/roboflow/roboflow-python/commit/61a84d41224fe191ee4e5616b3657122aaa072b4

The value is defined as optional string and passed downstream. https://github.com/roboflow/roboflow-python/blob/4558646857f1d543a57332b6eb267439dd1834b9/roboflow/core/project.py#L287

I'll check why mypy don't give me a better error and fix it.