ynput / ayon-backend

Server codebase with API access to AYON
Apache License 2.0
22 stars 15 forks source link

Settings: Improve SettingsField #278

Closed martastain closed 3 months ago

martastain commented 3 months ago

Improve SettingsField model to accommodate additional metadata

Deprecation

A boolean flag to indicate the field is deprecated

Tags

It would greatly enhance user experience if there was an option to assign tags to specific settings and then use those tags to filter through them. For instance, one could tag a group of settings across addons with the color_management tag and then easily filter them in the UI to adjust color management-related settings across addons. This would effectively address the current issue of having related settings scattered throughout the interface without any clear links between them.

martastain commented 3 months ago

This could also help us with migration to pydantic2. For the reference, this is a comparison for arguments passed to Field:

Pydantic 1.x (what the current Field and SettingsField use)

def Field(
    default: Any = Undefined,
    *,
    default_factory: Optional[NoArgAnyCallable] = None,
    alias: Optional[str] = None,
    title: Optional[str] = None,
    description: Optional[str] = None,
    exclude: Optional[Union['AbstractSetIntStr', 'MappingIntStrAny', Any]] = None,
    include: Optional[Union['AbstractSetIntStr', 'MappingIntStrAny', Any]] = None,
    const: Optional[bool] = None,
    gt: Optional[float] = None,
    ge: Optional[float] = None,
    lt: Optional[float] = None,
    le: Optional[float] = None,
    multiple_of: Optional[float] = None,
    allow_inf_nan: Optional[bool] = None,
    max_digits: Optional[int] = None,
    decimal_places: Optional[int] = None,
    min_items: Optional[int] = None,
    max_items: Optional[int] = None,
    unique_items: Optional[bool] = None,
    min_length: Optional[int] = None,
    max_length: Optional[int] = None,
    allow_mutation: bool = True,
    regex: Optional[str] = None,
    discriminator: Optional[str] = None,
    repr: bool = True,
    **extra: Any,
) -> Any:
    pass

And version 2 - we use several extra fields, such as widget and enum_resolver that the original Field accepts as **extra. That is deprecated in pydantic 2 and it is necessary to pass extras as json_schema_extra. We could maintain (and provide type hints) to our custom arguments just by implementing SetttingsField and have forward compatibility.

def Field(  # noqa: C901
    default: Any = PydanticUndefined,
    *,
    default_factory: typing.Callable[[], Any] | None = _Unset,
    alias: str | None = _Unset,
    alias_priority: int | None = _Unset,
    validation_alias: str | AliasPath | AliasChoices | None = _Unset,
    serialization_alias: str | None = _Unset,
    title: str | None = _Unset,
    field_title_generator: typing_extensions.Callable[[str, FieldInfo], str] | None = _Unset,
    description: str | None = _Unset,
    examples: list[Any] | None = _Unset,
    exclude: bool | None = _Unset,
    discriminator: str | types.Discriminator | None = _Unset,
    deprecated: Deprecated | str | bool | None = _Unset,
    json_schema_extra: JsonDict | typing.Callable[[JsonDict], None] | None = _Unset,
    frozen: bool | None = _Unset,
    validate_default: bool | None = _Unset,
    repr: bool = _Unset,
    init: bool | None = _Unset,
    init_var: bool | None = _Unset,
    kw_only: bool | None = _Unset,
    pattern: str | typing.Pattern[str] | None = _Unset,
    strict: bool | None = _Unset,
    coerce_numbers_to_str: bool | None = _Unset,
    gt: annotated_types.SupportsGt | None = _Unset,
    ge: annotated_types.SupportsGe | None = _Unset,
    lt: annotated_types.SupportsLt | None = _Unset,
    le: annotated_types.SupportsLe | None = _Unset,
    multiple_of: float | None = _Unset,
    allow_inf_nan: bool | None = _Unset,
    max_digits: int | None = _Unset,
    decimal_places: int | None = _Unset,
    min_length: int | None = _Unset,
    max_length: int | None = _Unset,
    union_mode: Literal['smart', 'left_to_right'] = _Unset,
    fail_fast: bool | None = _Unset,
    **extra: Unpack[_EmptyKwargs],
) -> Any:
dee-ynput commented 3 months ago

Could we use Annotated instead of Field?

martastain commented 3 months ago

You mean because of pyright false alarms for defaul_factories in pydantic? Or annotating FieldInfo class?

Could you give me an example?

dee-ynput commented 3 months ago

No, sorry ^_^ I meant:

# using something like this:
from typing import Annotated
studio_name: Annotated[str, SettingsConfig(...)] = Field("yolo", title="Yolo !")

# instead of oveloading pydantic.Field
studio_name: str = SettingsField("yolo", title="Yolo !", ...)

I have no clue which python version introduced that though 😅 But it would separate our extra config from pydantic field default one, which feels appropriate.

martastain commented 3 months ago

That would mean splitting the functionality between Pydantic's FieldInfo (returned by Field / SettingsField function) and a custom SettingsConfig. While Annotatated syntax is gaining traction lately, it wasn't the case when we started building Ayon so, this change would require changing all settings models of all addons (and using two additional imports).

Honestly i don't see any benefit of enforcing Annotated syntax, but it would still be possible with the original proposal:

class MyAddonSettings(BaseAddonSettings):
    my_value: Annotated[
        int, 
        SettingsField(
            0, 
            title="My Value",
            gte=0,
            scopes=["project"]
        )
    ] = 0

while the original syntax would still work:

class MyAddonSettings(BaseAddonSettings):
    my_value: int = SettingsField(0, title="My Value", scopes=["project"])

(tbh. i find the original syntax more readable, but that's just me)

SettingsField function is just a drop-in replacement for pydantic.Field, that unifies the arguments and provides exact type hints for our custom attributes (scopes, enum_resolver, widget, ...) instead of relying on **kwargs (in pydantic 1) or json_schema_extra (pydantic 2)

BigRoy commented 3 months ago

(tbh. i find the original syntax more readable, but that's just me)

Just wanted to add that I agree - with this I had absolutely no idea what I was looking at:

# using something like this:
from typing import Annotated
studio_name: Annotated[str, SettingsConfig(...)] = Field("yolo", title="Yolo !")
martastain commented 3 months ago

BTW: not relying on kwargs will help debugging :)

server-1    | 2024-08-02 10:44:14 DEBUG      server          Initializing addon aftereffects
server-1    | 2024-08-02 10:44:14 DEBUG      server          SettingsField: unsupported argument: {'layont': 'expanded'}