Migrate Visualizations API to FastAPI #18721

arash77 · 2024-08-19T16:47:10Z

Refactor the legacy Visualizations API logic so all the logic is contained in the VisualizationsService class
Create pydantic models for the responses and payloads
Create the FastAPI routes that will replace the old ones

related to #10889

How to test the changes?

(Select all options that apply)

I've included appropriate automated tests.
This is a refactoring of components with existing test coverage.
Instructions for manual testing are as follows:
1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

…ether again

…ta as json to Visualization api

lib/galaxy/schema/visualization.py

lib/galaxy/webapps/galaxy/api/visualizations.py

lib/galaxy_test/api/test_tags.py

lib/galaxy/webapps/galaxy/services/visualizations.py

davelopez

LGTM! Thank you @arash77!

I'll leave it open for some time in case anyone else wants to add a comment.

lib/galaxy/schema/schema.py

mvdbeek · 2024-09-09T10:00:31Z

lib/galaxy/schema/visualization.py

+
+
+class VisualizationCreatePayload(Model):
+    type: Optional[SanitizedString] = Field(


This probably has a set value of things that can be filled in, we should document and restrict those values.

I couldn't find the specific set of values. Can you point me where to find them?

I guess these are the plugin types loaded into VisualizationsRegistry. Might be a little tricky to load them up since you need the fully loaded plugins registry.

So, Should I leave it like this?

Yeah, but make it a normal string. It must match the plugin type, no need to do sanitization there.

just for type in VisualizationCreatePayload?

mvdbeek · 2024-09-09T10:01:30Z

lib/galaxy/schema/visualization.py

+        title="Config",
+        description="The config of the visualization.",
+    )
+    save: Optional[bool] = Field(


What is a visualization that is not saved ?

I just added this because it existed in the old API!
But yes that is useless to have such a variable.

I don't know ? Maybe that's used to preview a visualization ? But if so it would be broken since the None as the id won't validate against the response model.

It is hardcoded from the old code to True!
https://github.com/galaxyproject/galaxy/blob/release_24.1/lib/galaxy/webapps/galaxy/api/visualizations.py#L304
I will remove it.

mvdbeek · 2024-09-09T10:03:36Z

lib/galaxy/webapps/galaxy/api/visualizations.py

+            None, title="Import ID", description="The encoded database identifier of the Visualization to import."
+        ),
+        trans: ProvidesUserContext = DependsOnTrans,
+    ) -> VisualizationCreateResponse:
        """
        POST /api/visualizations


The dosctring ends up as documentation in the schema, we don't need to include stuff like POST /api/visualizations there.

mvdbeek · 2024-09-09T10:08:52Z

lib/galaxy/webapps/galaxy/services/visualizations.py

+        else:
+            # must have a type (I've taken this to be the visualization name)
+            if not payload.type:
+                raise exceptions.RequestParameterMissingException("key/value 'type' is required")


This can't happen given the new enforced API signature, right ?

mvdbeek · 2024-09-09T10:10:45Z

lib/galaxy/webapps/galaxy/services/visualizations.py

+                with transaction(session):
+                    session.commit()
+
+        return VisualizationCreateResponse(id=str(visualization.id))


If you're not committing the visusalization visualization.id is None, in which case the VisualizationCreateResponse is invalid.

mvdbeek · 2024-09-09T10:12:24Z

lib/galaxy/webapps/galaxy/services/visualizations.py

+        try:
+            visualization = trans.sa_session.get(Visualization, visualization_id)
+        except TypeError:
+            visualization = None


There's never going to be a type error, the try/except doesn't do anything

So should I just remove the try/except?

mvdbeek · 2024-09-09T10:18:18Z

lib/galaxy/webapps/galaxy/services/visualizations.py

+        # Error checking.
+        title_err = slug_err = ""
+        if not title:
+            title_err = "visualization name is required"


In that case just don't make the title optional in the function signature ? We want to validate this as much as possible via the pydantic models for the endpoints, which you're doing.

mvdbeek · 2024-09-09T10:19:38Z

lib/galaxy/webapps/galaxy/services/visualizations.py

+            slug_builder = SlugBuilder()
+            slug_builder.create_item_slug(trans.sa_session, visualization)
+        if annotation:
+            annotation = sanitize_html(annotation)


I don't think we should or need to do this, but also you're already doing this in the incoming pydantic model.

mvdbeek · 2024-09-09T10:21:09Z

Sorry for the late review, feel free to request a review via wg-backend or in the chat.

davelopez · 2024-09-09T12:32:48Z

Sorry, I should have requested a wg-backend review instead of just commenting on it 😓

mvdbeek · 2024-09-09T12:34:48Z

No worries, the procedure was fine. It's not like these are major issues, and this doesn't need more than one review either.

arash77 added 11 commits August 15, 2024 18:27

Init Visualizations FastAPI

1248492

Merge branch 'galaxyproject:dev' into FastAPI_Visualizations

1a49ff8

Add Visualization schema and payloads & joining create and import tog…

4ba092e

…ether again

Fix missed payloads

544421b

Update import_id parameter in create method of FastAPIVisualizations

be2e56a

Remove the old Visualization mixin controller

2880a29

Add comment for creating/importing visualization

bffd8d3

Fix Schema

7c962b0

Update

f8c01b2

Fix bugs with pydantic models, add schema.ts, fix tests

1d71426

Merge branch 'galaxyproject:dev' into FastAPI_Visualizations

9be27aa

github-actions bot added area/testing area/API area/testing/api labels Aug 19, 2024

github-actions bot added this to the 24.2 milestone Aug 19, 2024

arash77 added the kind/refactoring cleanup or refactoring of existing code, no functional changes label Aug 19, 2024

remove unused import

da23038

arash77 marked this pull request as draft August 19, 2024 16:59

arash77 added 3 commits August 20, 2024 11:16

Fix mypy errors

1c908d6

Fix mypy errors

c2a1014

update openapi schema

d36c835

arash77 marked this pull request as ready for review August 20, 2024 10:56

arash77 added 2 commits August 20, 2024 16:06

Update plugin schema

a7e260b

Fix TestVisualizationTags and NotificationsIntegrationBase to send da…

b73333a

…ta as json to Visualization api

davelopez reviewed Aug 21, 2024

View reviewed changes

arash77 added 3 commits August 21, 2024 14:33

Addressing the comments

b441f97

Remove unnecessary comment

0c6106c

Using SanitizedString Type

33f744f

davelopez approved these changes Sep 5, 2024

View reviewed changes

davelopez merged commit f669bb3 into galaxyproject:dev Sep 9, 2024
54 of 56 checks passed

mvdbeek reviewed Sep 9, 2024

View reviewed changes

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

mvdbeek reviewed Sep 9, 2024

View reviewed changes

arash77 mentioned this pull request Sep 9, 2024

Update Visualization FastAPI #18792

Merged

4 tasks

arash77 deleted the FastAPI_Visualizations branch September 9, 2024 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Visualizations API to FastAPI #18721

Migrate Visualizations API to FastAPI #18721

arash77 commented Aug 19, 2024

davelopez left a comment

mvdbeek Sep 9, 2024

arash77 Sep 9, 2024

mvdbeek Sep 9, 2024

arash77 Sep 9, 2024

mvdbeek Sep 9, 2024

arash77 Sep 9, 2024 •

edited

Loading

mvdbeek Sep 9, 2024

mvdbeek Sep 9, 2024

arash77 Sep 9, 2024

mvdbeek Sep 9, 2024 •

edited

Loading

arash77 Sep 9, 2024 •

edited

Loading

mvdbeek Sep 9, 2024

mvdbeek Sep 9, 2024

mvdbeek Sep 9, 2024

mvdbeek Sep 9, 2024

arash77 Sep 9, 2024

mvdbeek Sep 9, 2024

mvdbeek Sep 9, 2024

mvdbeek Sep 9, 2024

mvdbeek commented Sep 9, 2024

davelopez commented Sep 9, 2024 •

edited

Loading

mvdbeek commented Sep 9, 2024



		class VisualizationCreatePayload(Model):
		type: Optional[SanitizedString] = Field(

Migrate Visualizations API to FastAPI #18721

Migrate Visualizations API to FastAPI #18721

Conversation

arash77 commented Aug 19, 2024

How to test the changes?

License

davelopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arash77 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvdbeek Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

arash77 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvdbeek commented Sep 9, 2024

davelopez commented Sep 9, 2024 • edited Loading

mvdbeek commented Sep 9, 2024

arash77 Sep 9, 2024 •

edited

Loading

mvdbeek Sep 9, 2024 •

edited

Loading

arash77 Sep 9, 2024 •

edited

Loading

davelopez commented Sep 9, 2024 •

edited

Loading