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

Create central pydantic models #11827

Merged
merged 13 commits into from
May 25, 2021

Conversation

davelopez
Copy link
Contributor

The idea is to build some of the 'central' pydantic models for HDAs, HDCAs, Jobs, etc., and provide annotations and field documentation for them.

I've tried to make a bit of research (or at least some grepping) to write a description for each field but it was a bit difficult sometimes to figure out so I've put a TODO in those cases. Please feel free to directly suggest a description in the review if you know how to describe the field or improve any of the existing :)

I've also marked some fields as deprecated (initially using False) when I feel, by the code comments around, that the model field was supporting some legacy functionality.

What did you do?

  • Added some pydantic models corresponding with the different serialization views of HDAs and some directly related models.
  • Moved the existing UserModel from lib/galaxy/managers/users.py to lib/galaxy/schema/schema.py
  • (*) Introduced the ModelClassField to represent a particular model_class in pydantic models.
  • (*) Tangentially fixing the whoami API endpoint to return the user encoded ID

(*) Those changes were originally addressed in #11315 but that PR will probably take some time to be merged so I've included them here.

Why did you make this change?

As part of the efforts to move to FastAPI, creating, annotating, and documenting the pydantic models is one of the most time-consuming/tedious tasks, but an important one if we want to provide decent documentation in OpenAPI. Also, providing the models early will speed up the migration of API routes in the long term.

How to test the changes?

  • This is a refactoring of components with existing test coverage.
    (This will be tested automatically when the models are used in the API tests as they will get serialized/deserialized.)

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].



class HDADetailed(HDASummary):
model_class: str = ModelClassField(HDA_MODEL_CLASS_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

It's really awesome to see this all in one place, it makes the duplicated and/or expensive fields very visible.

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Seeing it in one place really highlights all the suboptimal things we've done over the years ... 🔦

title="Miscellaneous Blurb",
description="TODO",
)
file_ext: str = Field( # Is this a duplicate of HDASummary.extension?
Copy link
Member

Choose a reason for hiding this comment

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

I think it is, yes.

title="Metadata Files",
description="Collection of metadata files associated with this dataset.",
)
data_type: str = Field(
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of a duplicate of ext ... ext maps to a data_type. Maybe we should deprecate this at some later point ?
Returning the actual class is also not very stable, this is probably something that should have never left the API

title="Display Applications",
description="Contains new-style display app urls.",
)
display_types: List[DisplayApp] = Field(
Copy link
Member

Choose a reason for hiding this comment

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

I would love to deprecate this AND display_apps AND visualizations... this is dictated by the data type and can be mapped that way on the client.

Copy link
Member

Choose a reason for hiding this comment

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

I thought this too but it looks like there is a filter when I was looking over #11880 which filters by dbkey.

Copy link
Member

Choose a reason for hiding this comment

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

:( that is unfortunate. We might still be able to do it on the client (and there would be great value in it, this can be the bulk of data transferred in the details view), but that is a good bit more fiddly then.

const=True,
title="HDA or LDDA",
description="Whether this dataset belongs to a history (HDA) or a library (LDDA).",
deprecated=False # TODO Should this field be deprecated in favor of model_class?
Copy link
Member

Choose a reason for hiding this comment

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

Using just one of them is probably a good idea

@davelopez davelopez force-pushed the central_pydantic_models branch from d69448c to 67c3fdc Compare May 5, 2021 17:05
@davelopez
Copy link
Contributor Author

This PR adds nearly 70 pydantic models so far.
I think this may be a solid enough base to start using them and find any missing or mistyped fields, and also make the review less cumbersome so I will open this for review now :)

I placed all the models in lib/galaxy/schema/schema.py, but I wonder if it will be better to move some of them to their own namespace (if it is available), for example, all the models related to Workflows may live in lib/galaxy/workflow/_schema.py, same for Jobs, etc. I think we may have discussed this already but I forgot what was the decision 😅

@davelopez davelopez marked this pull request as ready for review May 10, 2021 16:48
@davelopez davelopez changed the title [WIP] Create central pydantic models Create central pydantic models May 10, 2021
@github-actions github-actions bot added this to the 21.09 milestone May 10, 2021
@davelopez davelopez force-pushed the central_pydantic_models branch from b1cfcaf to a301adb Compare May 21, 2021 11:53
@mvdbeek mvdbeek merged commit e377583 into galaxyproject:dev May 25, 2021
@mvdbeek
Copy link
Member

mvdbeek commented May 25, 2021

This is excellent, that so much @davelopez!

@davelopez
Copy link
Contributor Author

Thank you @mvdbeek!

@davelopez davelopez deleted the central_pydantic_models branch May 25, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants