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

Enable/disable preview for all the datasets when publishing Kedro-Viz from CLI #1894

Merged
merged 32 commits into from
May 31, 2024

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented May 8, 2024

Description

Resolves 1870

Development notes

  • --include-preview flag added for Viz CLI:
  • By default, kedro viz deploy and kedro viz build will have preview disabled for all the datasets
  • Introduced a ClassVar is_all_previews_enabled for DataNodeMetadata class which controls the enable/disable functionality for all the datasets

QA notes

  • Run kedro viz build --include-preview to enable preview for all datasets
  • Run kedro viz deploy --include-preview to enable preview for all datasets in the deployed version

NOTE: As discussed with @rashidakanchwala and @stephkaiser , kedro viz run will not have a preview flag and preview will be set to True by default. If the users want to change this behavior, they need to do this from UI. Designs will be updated soon (cc: @stephkaiser )

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

jitu5 added 5 commits May 3, 2024 13:57
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5 jitu5 self-assigned this May 8, 2024
jitu5 and others added 11 commits May 8, 2024 14:47
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
…ature/disable-preview

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla changed the title Enable users to disable preview for all when publishing Kedro-Viz Enable/disable preview for all the datasets when publishing Kedro-Viz via CLI May 16, 2024
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review May 16, 2024 23:00
@ravi-kumar-pilla ravi-kumar-pilla requested review from stephkaiser and merelcht and removed request for ravi-kumar-pilla May 16, 2024 23:01
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla changed the title Enable/disable preview for all the datasets when publishing Kedro-Viz via CLI Enable/disable preview for all the datasets when publishing Kedro-Viz with CLI May 17, 2024
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Should we make a mention in the documentation?

@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented May 17, 2024

There's also this ticket here #1705 , we can add this to next sprint.

jitu5 and others added 2 commits May 20, 2024 19:10
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

The code looks good, but I have some reservations about the naming of the new are_datasets_previewable boolean. It's getting confusing with the is_preview_disabled functionality we have.

@@ -452,7 +457,7 @@ def save_api_pipeline_response_to_fs(pipelines_path: str, remote_fs: Any):
raise exc


def save_api_responses_to_fs(path: str, remote_fs: Any):
def save_api_responses_to_fs(path: str, remote_fs: Any, are_datasets_previewable: bool):
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 personal preference, but I would change are_datasets_previewable to preview_datasets.

To me are_datasets_previewable sounds like it's checking whether it's possible to preview these datasets or not, where when reading the code it should mean whether preview has been enabled or not.

In my proposed change preview_datasets=True means preview is enabled and preview_datasets=False means it's disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, I too had a hard time renaming this :) .... so I want to make it differentiable to the is_preview_disabled which corresponds to each instance of DataNodeMetadata class, where-as the plural form are_datasets_previewable is a class variable. I personally prefer having is/are before a boolean variable/method to be clear of the return/data type.

Comment on lines 816 to 818
cls.data_node.is_preview_disabled()
or not hasattr(cls.dataset, "preview")
or not cls.are_datasets_previewable
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference here between cls.data_node.is_preview_disabled() and cls.are_datasets_previewable? Semantically, it sounds very similar, so I think we should think carefully about how these flags are named so it's easy to understand what's happening for future team members.

Also in terms of functionality, what does it mean for preview to be disabled on a node level vs class level?

Copy link
Contributor

@rashidakanchwala rashidakanchwala May 21, 2024

Choose a reason for hiding this comment

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

Agree with @merelcht.

i am also thinking for clarity we could possibly have :-

  • is_preview_disabled on node level (and explain this in the docstring, that this disabled via node level happens when the user specifies 'preview=false' in the DataCatalog)
  • is_all_previews_disabled on class level (and once again explain in docstring this is to disable previews for all nodes and can be done via CLI and UI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merelcht As discussed with @rashidakanchwala the existing is_preview_disabled will rename to is_preview_enabled and new boolean variable are_datasets_previewable to is_all_previews_enabled .

@merelcht
Copy link
Member

One thought is perhaps to name the new boolean after the new --include-preview flag, so it's clear those two are connected?

jitu5 added 3 commits May 21, 2024 18:53
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM. Can we please also create a a ticket/PR to change is_preview_disabled to is_preview_enabled so it is consistent.

thanks both @jitu5 and @ravi-kumar-pilla :D

@jitu5
Copy link
Contributor Author

jitu5 commented May 22, 2024

@rashidakanchwala Sure I will create a separate PR for change of is_preview_disabled to is_preview_enabled

@ravi-kumar-pilla ravi-kumar-pilla self-requested a review May 29, 2024 18:17
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

@jitu5
Copy link
Contributor Author

jitu5 commented May 31, 2024

@stephkaiser Requesting you approval on disable preview from CLI, few days back, I demoed it you. please let me know if you have any questions.

@stephkaiser
Copy link

@stephkaiser Requesting you approval on disable preview from CLI, few days back, I demoed it you. please let me know if you have any questions.

no questions from me! looks good, thanks Jitendra!

@jitu5 jitu5 closed this in #1927 May 31, 2024
@jitu5 jitu5 reopened this May 31, 2024
# Conflicts:
#	package/kedro_viz/models/flowchart.py
@jitu5 jitu5 merged commit 8fe5fa4 into main May 31, 2024
39 checks passed
@jitu5 jitu5 deleted the feature/disable-preview branch May 31, 2024 12:59
@SajidAlamQB SajidAlamQB mentioned this pull request Jul 25, 2024
5 tasks
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.

Enable users to disable preview for all when publishing Kedro-Viz
6 participants