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

Liu 285 - Rebuilt lg_web with FastAPI #201

Merged
merged 46 commits into from
Sep 30, 2022
Merged

Liu 285 - Rebuilt lg_web with FastAPI #201

merged 46 commits into from
Sep 30, 2022

Conversation

pritchardn
Copy link
Collaborator

This MR re-implements the translator web service with Fast API.
Additionally, this MR provides new implementations for each of the translator command-line actions.

To see the live documentation of the translator, load up an instance (e.g. 0.0.0.0:8086) and head to 0.0.0.0:8086/docs.
Not only is this a clean presentation of the APIs, but allows for manual in-browser testing of each of the endpoints.
I have convinced myself that the re-implementation does introduce any breaking changes, although I would like this to be checked before merging if possible.

I will henceforth evangelise FastAPI - it is a delight to use.

Alters requirements to suit, replacing bottle with uvicorn in the process.
Moves prepare_lgt method from lg_web to translator_utils.
…l variables.

Updates /gen_pgt GET method to use query fields rather than body form fields.
Updates several api endpoints to correctly behave.
Adds algorithm parameters to gen_pgt apis (in a backwards compatible but not totally satisfying manner).
Adds reprodata wrapping to unroll endpoint.
@pritchardn pritchardn marked this pull request as ready for review September 13, 2022 05:37
@coveralls
Copy link

coveralls commented Sep 13, 2022

Coverage Status

Coverage decreased (-1.4%) to 81.247% when pulling fbb50fb on LIU-285 into 28cd9d8 on master.

Copy link
Contributor

@awicenec awicenec left a comment

Choose a reason for hiding this comment

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

The changes in general seem pretty good. I've found two issues:

  1. When trying to deploy to ood-cld the graph does get deployed but there is an error message and the execution fails as well. The error message in the translator log says: TypeError: 'str' object is not an iterator
  2. This is actually a left-over I've found when trying the really nice FastAPI docs feature: There is a stray </script> tag at the very end of matrix_vis.html.
  3. The API docs should probably also be an additional item in the Help pull-down menu

Copy link
Contributor

@rtobar rtobar left a comment

Choose a reason for hiding this comment

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

I did a quick pass through all files except the new FastAPI-based lg_web module - I can go through that with more time if needed.

daliuge-translator/setup.py Outdated Show resolved Hide resolved
daliuge-translator/test-requirements.txt Outdated Show resolved Hide resolved
daliuge-translator/test/dropmake/test_lgweb.py Outdated Show resolved Hide resolved
@pritchardn
Copy link
Collaborator Author

Addressing Andreas' comments

1. When trying to deploy to ood-cld the graph does get deployed but there is an error message and the execution fails as well. The error message in the translator log says: TypeError: 'str' object is not an iterator

Fixed, the new gen_pg endpoint was returning a streaming response - which was incorrect, this has been changed to a JSONResponse; much more sensible. The JSON is a string, however, so in the main.js file, this needs to be parsed before sending off to OOD.

2. This is actually a left-over I've found when trying the really nice FastAPI docs feature: There is a stray </script> tag at the very end of matrix_vis.html.

Easily fixed

3. The API docs should probably also be an additional item in the Help pull-down menu

Added

# Conflicts:
#	daliuge-translator/test/dropmake/test_lgweb.py
@pritchardn pritchardn merged commit 25fab6e into master Sep 30, 2022
@pritchardn pritchardn deleted the LIU-285 branch September 30, 2022 06:02
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.

None yet

4 participants