Skip to content
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

DM-47790: Switch to vo-models for nearly all UWS responses #347

Merged
merged 2 commits into from
Dec 7, 2024

Conversation

rra
Copy link
Member

@rra rra commented Dec 4, 2024

Finish the conversion to using vo-models to generate nearly all UWS responses. Templating remains only to generate the error VOTable document.

Due to limitations in pydantic-xml plus the complexity of the type system, clients will have to provide an additional argument to the UWS configuration specifying the qualified type of the vo_models.uws.JobSummary class used to represent jobs. Clients will also have to implement a new to_xml_model method on their ParametersModel class.

@rra rra requested a review from stvoutsin December 4, 2024 22:59
@rra
Copy link
Member Author

rra commented Dec 4, 2024

This is still a bit awkward because internally list[UWSJobParameter] is still used because of the database schema. Once this is switched to use Wobbly, I'll be able to eliminate that in favor of storing just the API model in the database, and then this should be more straightforward (although alas still requiring three separate models).

@rra rra force-pushed the tickets/DM-47790 branch 4 times, most recently from 33d249f to 373ef6d Compare December 5, 2024 19:26
Copy link
Member

@stvoutsin stvoutsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks great to me.

Initially at first glance it felt like there may be some increased complexity and burden of having to manage multiple models to represent the same params and keep them in sync, but ultimately they all seem necessary so I can't really think of an alternative pattern that would be better.

I like the separation of concerns between worker & API models, the API vs XML seems necessary I guess though that's the bit that in an ideal world we can merge if it weren't for the obscuring that you noted.

safir/src/safir/uws/_service.py Outdated Show resolved Hide resolved
@@ -127,11 +128,12 @@ async def get_job(
uws_factory: Annotated[UWSFactory, Depends(uws_dependency)],
) -> Response:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to specify the schema for the response using the JobSummary model here (for the Openapi spec):

@uws_router.get(
    "/{job_id}",
    response_model=JobSummary,
    description="Get details of a specific job.",
    responses={
        200: {
            "description": "Successful Response",
            "content": {
                "application/xml": {
                    "schema": JobSummary.model_json_schema(),
                },
            },
        }
    },
    summary="Job details",
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After re-reading your documentation I guess this section implies that we shouldn't because the parameters would be obscured in the output?

Unfortunately, the same model cannot be used for 1 and 3 even for simple applications because the XML model requires additional structure that obscures the parameters and should not be included in the JSON API model

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, huh, I was assuming that this would give you a schema that was only suitable for JSON and not for XML, but it looks like the XML isn't that horrible. I'll go ahead and do this; at the least, it doesn't seem like it would hurt. Thanks!

docs/user-guide/uws/define-models.rst Outdated Show resolved Hide resolved
safir/src/safir/uws/_service.py Show resolved Hide resolved
given signing object. If no object is supplied, no URLs will be
included in the result list.
wait_seconds
If given, wait up to this many seconds for the status to change
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using something like tenacity (https://tenacity.readthedocs.io/en/latest/) to handle & hide the backoff and retry logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping to get rid of this retry loop entirely by doing something a bit more sophisticated, since we should at least block on completion of the arq job before we start waiting for the database update to happen. If I end up still needing a retry loop in Wobbly after that, I'll take a look. Thanks!

rra added 2 commits December 6, 2024 15:41
Finish the conversion to using vo-models to generate nearly all
UWS responses. Templating remains only to generate the error
VOTable document.

Due to limitations in pydantic-xml plus the complexity of the
type system, clients will have to provide an additional argument
to the UWS configuration specifying the qualified type of the
`vo_models.uws.JobSummary` class used to represent jobs. Clients
will also have to implement a new `to_xml_model` method on their
`ParametersModel` class.
Provide a schema for the routes returning vo-models Pydantic XML
models. These may not be entirely correct for the XML schema, but
they seem better than nothing. Do not return a schema for the
`/parameters` route, since we don't have easy access to the correct
parameters model in the handler.
@rra rra force-pushed the tickets/DM-47790 branch from 373ef6d to 81fe8c8 Compare December 7, 2024 00:07
@rra
Copy link
Member Author

rra commented Dec 7, 2024

Thanks for the review! It will be a bit simpler once we switch to Wobbly, although the separate XML model is still needed because it's the canonical representation of the flattened string view instead of the structured data view. But the list of UWS parameters goes away, and that makes everything a bit simpler. (Propagation of types through this code is really challenging, though.)

@rra rra merged commit 8662035 into main Dec 7, 2024
6 checks passed
@rra rra deleted the tickets/DM-47790 branch December 7, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants