-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add Pydantic model for cellpose models? #401
Comments
For the record. Script from pydantic.decorator import ValidatedFunction
from pydantic.decorator import validate_arguments
from pydantic import ValidationError
from enum import Enum
from cellpose import models
from devtools import debug
from lib_args_schemas import _validate_function_signature
from lib_args_schemas import _remove_args_kwargs_properties
from lib_args_schemas import _remove_pydantic_internals
# CREATE TYPE (see also https://github.com/tiangolo/fastapi/issues/13)
ModelInCellposeZoo = Enum(
"ModelInCellposeZoo",
{value: value for value in models.MODEL_NAMES},
type=str,
)
debug(models.MODEL_NAMES)
# USE PYDANTIC DECORATOR
@validate_arguments
def task_function_2(model: ModelInCellposeZoo = "cyto2"):
pass
try:
task_function_2(model="asd")
raise RuntimeError("This line should never be executed.")
except ValidationError:
print("All good, we raised a ValidationError.")
def task_function(model: ModelInCellposeZoo = "cyto2"):
pass
# GENERATE JSON SCHEMA
_validate_function_signature(task_function)
vf = ValidatedFunction(task_function, config=None)
schema = vf.model.schema()
schema = _remove_args_kwargs_properties(schema)
schema = _remove_pydantic_internals(schema)
debug(schema) Output
|
I explored this issue a bit further, and still could not find a nice solution. Option A: Use
|
Option CSimilar to Option A, but we build the forward-reference string. from typing import Literal
from cellpose import models
from pydantic import ValidationError
from pydantic.decorator import validate_arguments
cellpose_model_strings = ", ".join(
[f'"{model_type}"' for model_type in models.MODEL_NAMES]
)
CellposeModelType = f"Literal[{cellpose_model_strings}]"
print(CellposeModelType)
@validate_arguments
def task_function(model_type: CellposeModelType = "cyto2"):
print(f"Here we go: {model_type=} is valid.")
print()
task_function("cyto2")
try:
task_function("invalid_model_type")
except ValidationError:
print("Providing and invalid model type leads to ValidationError.")
What's wrongUsing a dynamically-defined string as a forward reference does not comply with mypy:
|
After discussion with @mfranzon:
Let's move on with option A. |
Yet another problem appeared, and I think this one leads to the conclusion that for now we cannot implement the feature required in the current issue. Option A works, but what gets written into the JSON Schema is the list of More generally, the JSON Schema would depend on the specific version of Cellpose that was available upon schema-generation, which may differ from the one coming in the task-execution environment. Note that the current solution (performing this check within the task) is obviously safe in terms of schemas, since the schema only asks for a I'm closing this issue since I judge the benefit (model-type check in fractal-web) is not worth the cost (the risk of having to re-release fractal-tasks-core at "random" times, or the requirement that we pin Cellpose to a specific version in our dependencies). Feel free to re-open for further discussion. |
Returning to this issue now that we have better support for Enums / Literals in Fractal web. Thinking about this again: Cellpose is not changing default models often. And we have its version pinned anyway as: The benefit of actually seeing models in the web interface would be significant enough to warrant this complexity. Users are much more likely to try valid models if those models are a dropdown than if they have to search for the model string. And as long as we keep the cellpose version pinned, there should be a match between the manifest and what gets installed. |
I now included this in #650 using the A option from above. |
Refs:
Channel
s #386 (comment)The text was updated successfully, but these errors were encountered: