Shareable Kedro-viz backend implementation #1498

rashidakanchwala · 2023-08-21T20:37:10Z

Description

Resolves #1520

This PR involves the backend implementation of Shareable URLs on Kedro-viz. In this PR, we have used fsspec to facilitate the uploading of the Kedro-viz build and API responses (in JSON format) to an S3 storage location. Kedro-viz, when deployed in a hosted environment, functions as a serverless application, serving static HTML content and retrieving Kedro project data from the API JSON files.

This PR solves two many issues

Hosting Kedro-viz.
Fixing the bug with the kedro viz --save-file and --load-file methods. These methods were previously not saving and loading the metadata associated with each node as well as registered pipelines flowchart. They were only saving flowchart for default pipeline

Development notes

Router.py:

This file defines two routers in this PR:

api/deploy: A POST request that sends the bucket_name and region from the frontend (FE) and forwards this information to S3Deployer.
api/package_compatibilities: A GET request sent by the FE to the BE to check if the fsspec version is greater than or equal to 2023.9.0, which is a requirement for Shareable Kedro-viz. If the user has an older version, the FE will prompt the user to update the dependency to the correct version before enabling this feature.

apps.py:

The main change in this file is the create_api_app_from_file function, which is invoked when the user runs kedro-viz --load-file xyz. Now, this function loads everything, including metadata for nodes and other registered pipeline information.

responses.py:

This file has undergone some refactoring, and one of the main changes is that it contains functions that enable the saving of backend responses to a specified filepath (either local or remote).

integrations/deployment/s3-deployer:

This class, S3Deployer, is the first of many deployer classes. As we expand to more deployer classes, we will introduce a baseDeployer class. The S3Deployer class is self-explanatory and handles the task of uploading content to S3.

server.py:

In this file, we make a change to the kedro-viz --save-file method. It now saves everything, including the flowchart, metadata, and other pipeline information, to the specified filepath.

QA notes

If you have an s3 bucket or a Minio. You can simply do a post request using curl or something else to upload your kedro-viz to s3.

curl -X POST http://localhost:4142/api/deploy \
-H "Content-Type: application/json" \
-d '{"region": "us-east-1", "bucket_name": "s3://kedroviz2"}'

Checklist

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added new entries to the RELEASE.md file
Added tests to cover my changes

Signed-off-by: Rashida Kanchwala <[email protected]>

…dro-viz into kedro-viz-save-load Signed-off-by: Rashida Kanchwala <[email protected]>

Signed-off-by: Rashida Kanchwala <[email protected]>

…dro-viz into shareable-flowchart

package/kedro_viz/api/rest/responses.py

package/kedro_viz/utils/api_tools.py

…dro-viz into shareable-flowchart

Signed-off-by: ravi-kumar-pilla <[email protected]>

ravi-kumar-pilla

Looks good to see all tests passing on Circle CI. Awesome !!

Most of the code looks good to me but I have a suggestion for the feature compatibility API design. Please have a look. Thank you !

ravi-kumar-pilla · 2023-09-20T02:28:45Z

package/kedro_viz/api/rest/router.py

-    )
+
+@router.get(
+    "/package_compatibilities",


In general, REST API routes should not contain special characters, the only exception being hyphen. It would be better if we call this as /package-compatibilities.

However, I feel we should tackle this route differently if we check for compatibility in the backend. Something like a [POST] /feature-compatibility route which accepts a json body like

{ "feature": "DEPLOY_VIZ" }

and have some kind of dictionary in the backend for feature_package_compatibility like -

feature_package_compatibility = { "DEPLOY_VIZ": { "fsspec": "2023.9.0" } }

This way we can check for feature compatibility and extend this if we need to check for multiple features in future. Since the frontend needs information on why some feature is not compatible, we can send specific message (containing the missing package dependency) or a generic message to the frontend like -

{ "feature": "DEPLOY_VIZ", "is_compatible": true, "compatible_packages": { "fsspec": "2023.9.0" } }

yup, already updated it to package-compatibilities in the FE PR and will also do the same in this PR.

So for now it's only one package compatibility that we return but yes in future there might be more. In that case I was thinking we would return a List[PackageCompatibilityAPIResponse].

Also, at this point, I am not so sure if we want to make it a POST request having said that I do feel we can better this approach in the future. Let's also hear what others thing about this.

Agree with both of you: this would be something we can scale going forward, though right now, without a use case to do so, I don't think we need to implement it at this stage.

So i read this earlier today, and thought it seemed relevant here. Hence sharing - https://www.techtarget.com/whatis/definition/You-arent-gonna-need-it#:~:text=YAGNI%20principle%20(%22You%20Aren',desired%20increased%20frequency%20of%20releases.

merelcht

I've done a first review and left some comments. I think my main points are around the "architecture" of the viz backend and where some of this new could should actually be. I'm sure this all works, but I'd be cautious about just breaking the intended architecture design that is in place now.

package/kedro_viz/api/rest/requests.py

package/kedro_viz/api/rest/responses.py

merelcht · 2023-09-20T11:50:12Z

package/kedro_viz/api/rest/responses.py

+    return version(package_name)  # pragma: no cover
+
+
+def get_package_compatibilities_response():


This seems to be very fsspec specific, so I'd include that in the name to avoid confusion. Should this be "public" method? Is anything accessing this directly or will it only be called from within the viz backend?

true, the reason I have kept this generic is incase we have more package compatibilties to test in the future.

merelcht · 2023-09-20T11:59:54Z

package/kedro_viz/api/rest/responses.py

+    )
+
+
+def write_api_response_to_fs(file_path: str, response: Any, _remote_fs: Any):


Why does _remote_fs start with an underscore?

package/kedro_viz/api/rest/responses.py

merelcht · 2023-09-20T12:06:18Z

package/kedro_viz/integrations/deployment/s3_deployer.py

+        self._upload_deploy_viz_metadata_file()
+
+    def get_deployed_url(self):
+        """Returns an S3 URL where Kedro viz is deployed"""


This also calls _deploy() so that's more than just returning the S3 URL, I'd make that clearer in the function name and doc string.

merelcht

I had another look after our discussion about the architecture. I left some minor comments, but I think it's nearly ready 🙂

merelcht · 2023-09-22T11:22:44Z

package/kedro_viz/api/rest/responses.py

+        file.write(encoded_response)
+
+
+def save_api_main_response_to_fs(main_loc: str, remote_fs: Any):


main_loc is a slightly vague variable name, I'd name this differently and also add some doc string to describe what this is.

Perhaps main_path?

merelcht · 2023-09-22T11:24:19Z

package/kedro_viz/api/rest/responses.py

+        raise exc
+
+
+def save_api_node_response_to_fs(nodes_loc: str, remote_fs: Any):


I'd change loc to path here as well if it makes sense to you.

package/kedro_viz/integrations/deployment/s3_deployer.py

package/tests/test_api/test_rest/test_responses.py

…dro-viz into shareable-flowchart

noklam

If we can't change the --save-file arguments, I think we could still update the help text to clarify it is saving multiple files instead of one.

It's confusing where I should provide a filename or a directory (maybe it should be an optional argument and have a default name?)

In addition, when I run kedro viz --save-file it try to open a new tab in my chrome browser, I think it's a bug. Should file be save as a .json suffix as seems like they are all JSON.

package/kedro_viz/api/apps.py

package/kedro_viz/api/rest/responses.py

package/kedro_viz/api/rest/router.py

noklam · 2023-09-25T12:16:29Z

package/kedro_viz/integrations/deployment/s3_deployer.py

+
+import fsspec
+from kedro.io.core import get_protocol_and_path
+from semver import VersionInfo


I see earlier you are using packaging, I think we should be consistent using either that of semver. And if I remember correctly I read it in some other issue that packaging is a better option?

Great point. I have reopened the issue and we should replace it. #1460

noklam · 2023-09-25T12:33:15Z

package/kedro_viz/api/rest/responses.py

+    encoded_response = EnhancedORJSONResponse.encode_to_human_readable(
+        jsonable_response
+    )
+
+    with remote_fs.open(file_path, "wb") as file:
+        file.write(encoded_response)
+


I was a bit surprised it is using "wb" instead of "w". I expect the result of encode_to_human_readable should be text that is readable, but seems like it is returning bytes?

yes, it's currently returning bytes. I will have to create an issue to figure the best way to make this more clearer. But it would be out of scope for the current one.

rashidakanchwala · 2023-09-25T13:12:34Z

If we can't change the --save-file arguments, I think we could still update the help text to clarify it is saving multiple files instead of one.

It's confusing where I should provide a filename or a directory (maybe it should be an optional argument and have a default name?)

In addition, when I run kedro viz --save-file it try to open a new tab in my chrome browser, I think it's a bug. Should file be save as a .json suffix as seems like they are all JSON.

@noklam -- definitely will fix the text... thanks for flagging!

Good question -- I am not sure if it's a bug.
So when you do kedro viz --save-file it does 2 things -- it opens kedro viz and it also saves your kedro-viz api files in the folder you specify.

@stephkaiser , @tynandebold , @merelcht -- what do you think of the above? To be frank; i was also initially confuse because kedro-viz would open when I did kedro viz --save-file but then I assumed this was the functionality and user's haven't highlighted this.

tynandebold · 2023-09-25T16:43:34Z

If we can't change the --save-file arguments, I think we could still update the help text to clarify it is saving multiple files instead of one.

It's confusing where I should provide a filename or a directory (maybe it should be an optional argument and have a default name?)
In addition, when I run kedro viz --save-file it try to open a new tab in my chrome browser, I think it's a bug. Should file be save as a .json suffix as seems like they are all JSON.

@noklam -- definitely will fix the text... thanks for flagging!

Good question -- I am not sure if it's a bug. So when you do kedro viz --save-file it does 2 things -- it opens kedro viz and it also saves your kedro-viz api files in the folder you specify.

@stephkaiser , @tynandebold , @merelcht -- what do you think of the above? To be frank; i was also initially confuse because kedro-viz would open when I did kedro viz --save-file but then I assumed this was the functionality and user's haven't highlighted this.

Thanks for pointing that out, Nok! The action of Viz starting and opening when you run kedro viz --save-file has always been there and wasn't introduced in this PR. We can decide if we think it's a bug or not, though either way, we don't need to implement a solution in this PR, so it shouldn't block this work.

merelcht

I think this is in a good state now. Great work @rashidakanchwala ⭐ !

IMO the Viz backend should be refactored before adding any other new big features, this PR showed some of the strange stuff going on with the pydantic models and dataclasses. It might be a good time to do that after shareable viz is merged.

noklam

Thanks! I agree with Merel that it would be great to spend some time after this to clean up the backend.

It's a nice feature to add and I am interested to make a GitHubPageDeployer after this is merged :)

rashidakanchwala and others added 20 commits February 17, 2023 12:45

nodes working

5af4d9f

Signed-off-by: Rashida Kanchwala <[email protected]>

added pipeline

283a4f5

Signed-off-by: Rashida Kanchwala <[email protected]>

Merge branch 'kedro-viz-save-load' of https://github.com/kedro-org/ke…

96bb1d2

…dro-viz into kedro-viz-save-load Signed-off-by: Rashida Kanchwala <[email protected]>

fix path

7de3f65

Signed-off-by: Rashida Kanchwala <[email protected]>

fix bug and lint

3a023f9

Signed-off-by: Rashida Kanchwala <[email protected]>

fix lint

3c2b625

Signed-off-by: Rashida Kanchwala <[email protected]>

Merge remote-tracking branch 'origin' into kedro-viz-save-load

5da7410

update responses

4762c2f

test

4bc45e5

fix rebase

6081e85

fixing stuff

d044bd1

refactored to latest

b986c85

some refactor

d0f7e65

fix lint - WIP

703b64c

Merge branch 'main' into shareable-flowchart

9ecb395

refactor work

6b01350

Merge branch 'shareable-flowchart' of https://github.com/kedro-org/ke…

8b91f7e

…dro-viz into shareable-flowchart

fix lint

30e5f8a

further refactor

fd75407

add error handling and debugging

480bd51

ravi-kumar-pilla reviewed Sep 5, 2023

View reviewed changes

package/kedro_viz/api/rest/responses.py Outdated Show resolved Hide resolved

ravi-kumar-pilla reviewed Sep 5, 2023

View reviewed changes

package/kedro_viz/utils/api_tools.py Outdated Show resolved Hide resolved

rashidakanchwala and others added 8 commits September 5, 2023 17:43

fix based on review

e94f0ec

modify upload static files logic

a6ecd7c

Merge branch 'shareable-flowchart' of https://github.com/kedro-org/ke…

1232e92

…dro-viz into shareable-flowchart

refactor upload api with latest fsspec

737af74

fix unit tests_1

9e24189

revert os logic to pathlib

ef4617f

fix static folder issue

9e6d9a5

fix format and lint errors

5b0a860

Signed-off-by: ravi-kumar-pilla <[email protected]>

packaging reqs

269ed26

rashidakanchwala marked this pull request as ready for review September 18, 2023 17:30

rashidakanchwala requested a review from tynandebold as a code owner September 18, 2023 17:30

rashidakanchwala changed the title ~~[Draft] Shareable Kedro-viz backend implementation~~ Shareable Kedro-viz backend implementation Sep 18, 2023

rashidakanchwala requested review from merelcht, noklam and idanov September 19, 2023 09:37

tynandebold mentioned this pull request Sep 19, 2023

Make Kedro-Viz shareable via a hosted URL #1487

Merged

5 tasks

ravi-kumar-pilla reviewed Sep 20, 2023

View reviewed changes

fix api endpoint

72a7e18

merelcht reviewed Sep 20, 2023

View reviewed changes

fixes based on reviews

4cea3e9

merelcht reviewed Sep 22, 2023

View reviewed changes

rashidakanchwala and others added 4 commits September 22, 2023 21:36

Merge branch 'main' into shareable-flowchart

c3de82f

changes based on reviews

33f568d

Merge branch 'shareable-flowchart' of https://github.com/kedro-org/ke…

890f92e

…dro-viz into shareable-flowchart

fix lint

3809add

noklam reviewed Sep 25, 2023

View reviewed changes

merelcht approved these changes Sep 26, 2023

View reviewed changes

rashidakanchwala added 4 commits September 26, 2023 11:12

fixes based on review

e912b92

updated cli help definition

d2de353

update filpath to directory

29a0fab

update filpath to directory

76bd6dd

noklam self-requested a review September 26, 2023 11:59

noklam approved these changes Sep 26, 2023

View reviewed changes

add s3 protocol in the backend

635634b

rashidakanchwala merged commit 1fa5a02 into main Sep 27, 2023
1 check passed

rashidakanchwala deleted the shareable-flowchart branch September 27, 2023 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shareable Kedro-viz backend implementation #1498

Shareable Kedro-viz backend implementation #1498

rashidakanchwala commented Aug 21, 2023 •

edited by tynandebold

Loading

ravi-kumar-pilla left a comment

ravi-kumar-pilla Sep 20, 2023

rashidakanchwala Sep 20, 2023 •

edited

Loading

tynandebold Sep 20, 2023

rashidakanchwala Sep 22, 2023

merelcht left a comment

merelcht Sep 20, 2023

rashidakanchwala Sep 20, 2023

merelcht Sep 20, 2023

merelcht Sep 20, 2023

merelcht left a comment

merelcht Sep 22, 2023

merelcht Sep 22, 2023

merelcht Sep 22, 2023

noklam left a comment

noklam Sep 25, 2023

rashidakanchwala Sep 26, 2023

noklam Sep 25, 2023

rashidakanchwala Sep 26, 2023

rashidakanchwala commented Sep 25, 2023

tynandebold commented Sep 25, 2023

merelcht left a comment

noklam left a comment

		return version(package_name) # pragma: no cover


		def get_package_compatibilities_response():

		)


		def write_api_response_to_fs(file_path: str, response: Any, _remote_fs: Any):

		file.write(encoded_response)


		def save_api_main_response_to_fs(main_loc: str, remote_fs: Any):

		raise exc


		def save_api_node_response_to_fs(nodes_loc: str, remote_fs: Any):

Shareable Kedro-viz backend implementation #1498

Shareable Kedro-viz backend implementation #1498

Conversation

rashidakanchwala commented Aug 21, 2023 • edited by tynandebold Loading

Description

Development notes

Router.py:

apps.py:

responses.py:

integrations/deployment/s3-deployer:

server.py:

QA notes

Checklist

ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashidakanchwala Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht 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

merelcht 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

noklam 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

rashidakanchwala commented Sep 25, 2023

tynandebold commented Sep 25, 2023

merelcht left a comment

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

rashidakanchwala commented Aug 21, 2023 •

edited by tynandebold

Loading

rashidakanchwala Sep 20, 2023 •

edited

Loading