Rework nested models

[loichuder]

Fix #46

[codecov]

Codecov Report

❌ Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/ewokssphinx/ewoks_task_utils.py	92.00%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[loichuder]

Works fine with ewoksndreg and ewoks3dxrd

[woutdenolf]

I would propose parse_pydantic_model to take the class, not the qualname:

def parse_pydantic_model(model_cls: Type[BaseModel] | None) -> list[ParameterDescription]:
    parameters = []
    if model_cls is None:
        return parameters

    if not issubclass(model_cls, BaseModel):
        raise ValueError("Not a pydantic model")
    for name, field_info in model_cls.model_fields.items():
        parameters.append(_parse_pydantic_field(name, field_info))
    return parameters

Not sure why parse_pydantic_model this is not a private method. I think it should be.

Then we can avoid the qualname-import roundtrip

input_model = task.get("input_model")
if input_model is not None:
    input_model = _import_model(input_model)
    inputs = parse_pydantic_model(input_model)
else:
    inputs = _parse_parameter_names(
        task.get("required_input_names", []),
        task.get("optional_input_names", []),
    )

output_model = task.get("output_model")
if output_model:
    output_model = _import_model(output_model)
    outputs = parse_pydantic_model(output_model)
else:
    outputs = _parse_parameter_names(task.get("output_names", []), [])

submodels = extract_submodels(input_model, output_model)

The extract_submodels method (should probably be private as well)

def extract_submodels(*models: Type[BaseModel] | None) -> dict[str, List[TaskParameter]]:
    models = tuple(m for m in models if m is not None)

    if not models:
        return {}

    annotation = reduce(lambda a, b: a | b, models)

    return {
        qualname(model_cls): parse_pydantic_model(model_cls)
        for model_cls in _iter_models(annotation, set())
        if model_cls not in models
    }

[loichuder]

Thanks for the review!

I changed extract_submodels accordingly but it felt more natural to me to still have a single model as input. I also made all functions private as advised.

[woutdenolf]

it felt more natural to me to still have a single model as input

The reason I propose extract_submodels to work on a single annotation that combines both models is that the ordering of the result will be better and clearer imo: order is provided as declared (depth-first).

If you call extract_submodels for both models individually you have to merge the results and make an extra decision on the ordering and the duplicates. This is a problem when the inputs and outputs have shared models. For example

class Shared(BaseModel):

class Model1(BaseModel):
    name: str
    shared: Shared

class Model2(BaseModel)
    name: str
    shared: Shared

class InputModel(BaseInputModel):
    var: Model1

class OutputModel(BaseOutputModel):
    var: Model2

• When extract_submodels runs on one single annotation, the order is as declared: Model1, Shared, Model2.
• When extract_submodels runs on the individual models and we merge later: Model1, Model2, Shared.

When I start reading the docs I would want to know everything about InputModel and Model1 which involves Shared before I continue to OutputModel and Model2. That's how I would read the code as well.

Edit: even if we would prefer breadth-first instead of depth-first like we have now, we would still want only one extract_submodels imo.

[loichuder]

The reason I propose extract_submodels to work on a single annotation that combines both models is that the ordering of the result will be better and clearer imo: order is provided as declared (depth-first).

I hadn't thought of that! Yes, in this case, you are right, it is best to do as you said.

[loichuder]

@woutdenolf Should be good now!