spacetelescope / vo-models

https://vo-models.readthedocs.io/
MIT License
3 stars 1 forks source link

`Parameters` support for multi-valued parameters #21

Open rra opened 1 month ago

rra commented 1 month ago

I'm working on adopting vo-models for an IVOA SODA service, and ran into a limitation that I don't think there's currently a way to work around. The SODA service supports several multi-valued parameters (ID, POS, CIRCLE, POLYGON, etc.). I'm new to pydantic-xml, but if I'm understanding the documentation correctly, this would be modeled in a more typical XML scenario using something like:

class CutoutParameters(Parameters):
    id: list[str] = element(tag="id")
    pos: list[str] = element(tag="pos")
    circle: list[str] = element(tag="circle")
    polygon: list[str] = element(tag="polygon")

For vo-models, there's really only one child element type, so I tried:

class GenericParameters(Parameters):
    params: list[Parameter]

as a first pass, giving up for now on modeling the exact parameters (since I already have other code to handle the parsing, since it also does non-trivial transformations of the XML parameters to make them match a more reasonable JSON schema). But unfortunately this fails here:

    def __init__(__pydantic_self__, **data) -> None:  # pylint: disable=no-self-argument
        # during init -- especially if reading from xml -- we may not get the parameters in the order
        # pydantic-xml expects. This will remap the dict with keys based on the parameter id.
        parameter_vals = [val for val in data.values() if val is not None]
        remapped_vals = {}
        for param in parameter_vals:
            if isinstance(param, dict):
                remapped_vals[param["id"]] = Parameter(**param)
            else:
>               remapped_vals[param.id] = param
E               AttributeError: 'list' object has no attribute 'id'

There isn't, so far as I can tell, a way to write a model for the cutout parameters using the syntax described in the vo-models documentation because each input parameter may be a list since the same parameter can be specified more than once.

I think the implication is that multi-valued parameters are not currently supported by the Parameters class (and, unfortunately, therefore also not supported by the JobSummary class. I'd be happy to try to come up with a fix, but I don't understand the purpose of this __init__ method or what problem it's trying to solve, and I think there may be some more fundamental problem since Parameter is a class and is obviously not type-compatible with list[Parameter], so i'm not sure how this type of structure is supposed to be specified.

rra commented 1 month ago

Ah, I think this __init__ code is trying to work around strict ordering mode, and if you change the search mode to unordered, you could drop this __init__ code and then my code would have worked? I'm looking at:

https://pydantic-xml.readthedocs.io/en/latest/pages/data-binding/elements.html#elements-search-mode

rra commented 2 weeks ago

Oh, now I see why __init__ is necessary. pydantic-xml assigns input to model attributes based on the element tag, but since that tag is always the same, vo-models has to redo that classification based on the id attribute. So __init__ needs to both handle list input (since pydantic-xml may have grouped input parameters into a list model attribute) and list output as the values of the dict it constructs (to correctly represent multivalued attributes).

I think https://github.com/spacetelescope/vo-models/pull/24 should fix this problem. I also added a test case for multivalued parameters.