Add FastAPI routes for tours #11089

jdavcs · 2021-01-11T08:25:48Z

@mvdbeek, @davelopez: please take a look - here's my first attempt.

Note: A pydantic TourList model (as opposed to List[Tour]) was necessary for correct serialization in web/framework/decorators.py. Even though we can modify this logic to recognize a list of pydantic models, such a list cannot be serialized using the [model].json() method.

Pydantic models (currently added to tours) require explicit specification of model fields. Having an "id" field as required seems right from a design standpoint. However, if there is a reason for an "id" value to be included in the galaxy_ui tour but not in the other tours, this field can be made optional: `id: Optional[str] = None`

- Do not call tour_loader() twice - Do not return a value from a function that modifies the value of its argument in place - Rename function to reflect what it does

lib/galaxy/schema/__init__.py

davelopez

Looking great!

lib/galaxy/webapps/galaxy/api/tours.py

lib/galaxy/schema/__init__.py

lib/galaxy/tours/__init__.py

config/plugins/tours/core.history.yaml

Field not used. See code review discussion here: galaxyproject#11089 (comment)

Address comments in code review Model is revised due to change in Tour model spec: - Id field is no longer part of the yaml file spec; - However, an id is generated and added to the tour data returned as a list - As a result, in a list, a tour item must have an id, whereas in tour details, a tour item must not have an id. I've factored out "core tour" data into its own model to solve this.

jdavcs · 2021-01-11T23:05:22Z

The latest commit addresses this comment in the code review.

However, this turned out to be a good excuse to get rid of the concrete ToursRegistry class in the api module and replace it with an interface. This follows this comment; @jmchilton - does this look like the approach you were describing?

jdavcs · 2021-01-11T23:13:25Z

Note about naming the abstract class: I struggled with the naming; with ToursRegistry as the concrete class, there was nothing that felt right for the abstract class: it's a virtual class, it's not inherited, so calling it ToursRegistryBase made no sense. Besides, I don't want to "signal" to the client module that it's an abstract class (even more so for names like IToursRegistry). Yes, "abc" shadows the standard library "abc" package, but, still, I think it makes sense to have foo.Foo and foo.abc.Foo just like we now might have foo.schema.Foo. But I'm, of course, by completely open to other suggestions.

jmchilton · 2021-01-12T06:09:14Z

I love the abc approach. In terms of naming stuff my suggestion would be to...

Refactorlib/galaxy/tours/__init__.py::ToursRegistry -> lib/galaxy/tours/_impl.py::ToursRegistryImpl
Add a def build_tours_registry(config_dir: str) -> ToursRegistry factory to _impl.py.
Refactor lib/galaxy/tours/abc/__init__.py::ToursRegistry -> lib/galaxy/tours/interface.py::ToursRegistry
Make lib/galaxy/tours/__init__.py a file that just imports ._impl.py::build_tours_registry, .schema.py::(TourStep,Tour,TourDetails,etc..), and .interface.py::ToursRegistry and re-exports the whole module's interface.

At that point I would even consider renaming schema.py and interface.py to _schema.py and _interface.py to make it clear the interface is the stuff that is exported from galaxy.tours 's root __init__.py.

davelopez

Cool! I like your approach of using an ABC for the ToursRegistry :)

lib/galaxy/webapps/galaxy/api/__init__.py

jdavcs · 2021-01-13T01:02:00Z

I love the abc approach. In terms of naming stuff my suggestion would be to...

@jmchilton thank you for the detailed suggestions - I think this makes the tours interface much cleaner.

The factory function does not have a return type annotation - that's intentional; it's related to mypy not handling ABC.register - I'll open an issue shortly.

EDIT: no need for a separate issue, here's the gist. The previous commit caused a mypy error which was due to mypy not handling virtual subsclasses. References:
python/mypy#1459
python/mypy#2922
https://docs.python.org/3/library/abc.html#abc.ABCMeta

This is (again!) ready for review.

jmchilton · 2021-01-13T01:13:24Z

lib/galaxy/tours/__init__.py

+from ._schema import TourCore  # noqa
+from ._schema import TourStep  # noqa
+from ._schema import TourDetails  # noqa
+from ._schema import TourList  # noqa


Instead of #noqa please use __all___ at the end of the file.

__all__ = ("build_tours_registry", "Tour", ...)

jmchilton

Amazing! Thanks for the deep dive into this.

jdavcs added 7 commits January 10, 2021 18:05

Add test, assertions for tour API testing

0cc2632

Add FastAPI routes, Pydantic models to tours

82e3db5

Make a TourList model for correct serialization

759aca7

Refactor tour loading (minor)

dbae033

- Do not call tour_loader() twice - Do not return a value from a function that modifies the value of its argument in place - Rename function to reflect what it does

Simplify parsing pydantic models

ca5516b

Move pydantic models to schema/

6021a10

jdavcs added kind/feature area/API labels Jan 11, 2021

jdavcs added this to the 21.01 milestone Jan 11, 2021

jdavcs mentioned this pull request Jan 11, 2021

Progressively adapt old API routes to fastAPI #10889

Open

55 tasks

mvdbeek reviewed Jan 11, 2021

View reviewed changes

lib/galaxy/schema/__init__.py Outdated Show resolved Hide resolved

mvdbeek reviewed Jan 11, 2021

View reviewed changes

lib/galaxy/schema/__init__.py Outdated Show resolved Hide resolved

davelopez reviewed Jan 11, 2021

View reviewed changes

lib/galaxy/webapps/galaxy/api/tours.py Outdated Show resolved Hide resolved

lib/galaxy/schema/__init__.py Outdated Show resolved Hide resolved

lib/galaxy/schema/__init__.py Outdated Show resolved Hide resolved

davelopez reviewed Jan 11, 2021

View reviewed changes

lib/galaxy/tours/__init__.py Outdated Show resolved Hide resolved

jmchilton reviewed Jan 11, 2021

View reviewed changes

config/plugins/tours/core.history.yaml Outdated Show resolved Hide resolved

Move tour pydantic models to their own module

a7d8d67

davelopez mentioned this pull request Jan 11, 2021

Add FastAPI routes for datatypes #11098

Merged

jdavcs added 2 commits January 11, 2021 13:43

Remove id field from tour yaml files

ba1b26e

Field not used. See code review discussion here: galaxyproject#11089 (comment)

jdavcs force-pushed the dev_fastapi_tours branch from c4549d9 to 75ed305 Compare January 11, 2021 20:35

Use a registry abc instead of concrete class

f1274c2

jdavcs force-pushed the dev_fastapi_tours branch from 732cab1 to f1274c2 Compare January 11, 2021 22:53

davelopez reviewed Jan 12, 2021

View reviewed changes

lib/galaxy/webapps/galaxy/api/__init__.py Outdated Show resolved Hide resolved

mvdbeek modified the milestones: 21.01, 21.05 Jan 12, 2021

jdavcs added 2 commits January 12, 2021 13:46

Refactor tours package organization

d90b4d2

Refactor tours module implemantation

dcaa04b

jdavcs added the status/WIP label Jan 12, 2021

Work around limitations of mypy

8f3e592

jdavcs removed the status/WIP label Jan 13, 2021

jmchilton reviewed Jan 13, 2021

View reviewed changes

Define __all__ list for tours package

2a25598

jmchilton approved these changes Jan 13, 2021

View reviewed changes

jdavcs merged commit 882ad63 into galaxyproject:dev Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FastAPI routes for tours #11089

Add FastAPI routes for tours #11089

jdavcs commented Jan 11, 2021

davelopez left a comment

jdavcs commented Jan 11, 2021

jdavcs commented Jan 11, 2021

jmchilton commented Jan 12, 2021

davelopez left a comment

jdavcs commented Jan 13, 2021 •

edited

Loading

jmchilton Jan 13, 2021

jdavcs Jan 13, 2021

jmchilton Jan 13, 2021

jmchilton left a comment

Add FastAPI routes for tours #11089

Add FastAPI routes for tours #11089

Conversation

jdavcs commented Jan 11, 2021

davelopez left a comment

Choose a reason for hiding this comment

jdavcs commented Jan 11, 2021

jdavcs commented Jan 11, 2021

jmchilton commented Jan 12, 2021

davelopez left a comment

Choose a reason for hiding this comment

jdavcs commented Jan 13, 2021 • edited Loading

jmchilton Jan 13, 2021

Choose a reason for hiding this comment

jdavcs Jan 13, 2021

Choose a reason for hiding this comment

jmchilton Jan 13, 2021

Choose a reason for hiding this comment

jmchilton left a comment

Choose a reason for hiding this comment

jdavcs commented Jan 13, 2021 •

edited

Loading