Feature: show matplotlib dataset pngs #887

tynandebold · 2022-06-06T18:32:02Z

Description

Resolves #783.

Massive thank you to @rashidakanchwala for helping so much with this one.

Development notes

There are a lot of changes here, and that's because I've renamed several files to adopt a more generic approach to the modal formerly named PlotlyModal. That's now simply MetadataModal, since we may want to put other things in there.

QA notes

I've added a Matplotlib Image node to the demo project, so you'll be able to see the implementation if you open Gitpod and take a look at the preview link.

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: Tynan DeBold <[email protected]>

…ature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

Signed-off-by: Tynan DeBold <[email protected]>

…ature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

….com/kedro-org/kedro-viz into feature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

Signed-off-by: Tynan DeBold <[email protected]>

Signed-off-by: Antony Milne <[email protected]>

antonymilne · 2022-06-07T07:08:25Z

I just tried this out on gitpod but it got stuck installing requirements with

ERROR: Cannot install matplotlib==3.5.0 and matplotlib~=3.5.2 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested matplotlib~=3.5.2
    The user requested matplotlib==3.5.0

So I pushed 3c99820 which should fix this.

As far as I can tell there's no reason that matplotlib should be a Python requirement of the kedro-viz package, is there? In fact I'm not sure whether we really need these either... 🤔 But that's a separate issue.

kedro-viz/package/requirements.txt

Lines 8 to 9 in 895f894

    
           plotly>=4.0 
        
           pandas>=0.24

antonymilne

Github is doing something weird so submitting this before it loses my comments... review in progress.

package/kedro_viz/models/graph.py

antonymilne · 2022-06-07T10:28:15Z

package/tests/test_models/test_graph/test_graph_nodes.py

@@ -384,9 +384,8 @@ def test_partitioned_data_node_metadata(self):
        data_node_metadata = DataNodeMetadata(data_node=data_node)
        assert data_node_metadata.filepath == "partitioned/"

-    @patch("builtins.__import__", side_effect=import_mock)


Why was this removed? Seems to me that we should be able to run the tests without plotly installed (related to the point of removing plotly from requirements.txt).

I'm not sure. @rashidakanchwala? Should I add it back in?

so I removed it, because it wasn't actually being used. :S

Actually this is used even though patched_import is not explicitly mentioned in the test. When you @patch it decorates the test function, and doing so also passes on an argument to the test function.

But it turns out it's not needed for another reason: because plotly is a requirement of kedro-viz.

src/components/metadata-modal/metadata-modal.js

src/components/metadata-modal/metadata-modal.scss

antonymilne

This is great and all seems to work perfectly!

I will check the thing about requirements. Other than that, I just have minor comments and suggestions really - let me know if you want to discuss anything further!

antonymilne · 2022-06-07T10:51:38Z

src/components/metadata-modal/metadata-modal.js

      <div className="pipeline-plot-modal__bottom">
        <button
          className="pipeline-plot-modal__collapse-plot"
          onClick={onCollapsePlotClick}
        >
          <CollapseIcon className="pipeline-plot-modal__collapse-plot-icon"></CollapseIcon>
          <span className="pipeline-plot-modal__collapse-plot-text">
-            Collapse Plotly Visualization
+            {metadata?.plot


Minor comment, but this feels a bit weird to me:

here we're doing "if metadata has plot then do plotly; else do matplotlib"

in metadata.js we're doing "if metadata hasPlot then do plotly; if metadata hasImage then do matplotlib". This seems logically the same as graph.py with is_plot_node and is_image_node

in config.js we have shortTypeMapping which looks like it could also be used to implement the "what type of node is this?" logic

Obviously, the logic here works fine because we only two options that generate the modal, so it's safe to assume that if it's not a plotly then it's matplotlib. But it just feels a little hard to follow to me for anyone reading the codebase afresh. It would be nice if we had some consistent way of handling the "what type of node is this?" logic. Ideally I think we would just use the node's dataset_type property to switch between different behaviours rather than looking to see whether a node contains image or plot.

Possibly this is a premature optimisation since we don't have more than two special cases yet, and it would undoubtedly require a bigger rewrite than the scope of this PR. But it might at least be worth seeing if the logic in this file can be made consistent with the metadata.js logic.

Is it possible that a node with a dataset_type of plotly or matplotlib won't contain and image or a plot? If it is, I think we'll still need to check for either image or plot. In fact, this did happen to me yesterday. I added in my new node but didn't do kedro run, so the image wasn't there but the node on the flowchart was present, as was the metadata panel.

I agree with everything else. The logic isn't identical and should be. I'll update it.

Ah yes, that is a very good point. If you haven't done a kedro run yet then plot/image won't be defined.

In this case I guess there's two bits of logic we need:

something to check the dataset_type (probably using shortTypeMapping? Something like switch shortTypeMapping[dataset_type] or whatever)

something to check whether the dataset actually has image or plot defined

Got it!

For now, I've added in these two lines, which is what's done in the metadata file, as you called out above:

const hasPlot = Boolean(metadata?.plot); const hasImage = Boolean(metadata?.image);

Hopefully that makes things a little more clear. Do you think it's enough? I'm wary of over-optimizing further at this point.

Yeah I think that's perfect, no need to do any more for now. My only suggestion would be to change this

if (!visible.metadataModal || (!metadata?.plot && !metadata?.image)) {

to use the new hasPlot and hasImage variables. Same goes for the stuff on L65-67.

Also, curious: is there any reason these explicitly cast to Boolean? Seems like some things in metadata.js are and some aren't:

const hasCode = Boolean(metadata?.code); const isTranscoded = Boolean(metadata?.originalType); const showCodePanel = visible && visibleCode && hasCode; const showCodeSwitch = hasCode;

Good catch. Added those two to this, as suggested:

if (!visible.metadataModal || (!metadata?.plot && !metadata?.image)) {

The first two are cast as such because they're being converted from strings. The last two don't need it, since they're created from booleans they'll be that type, too.

package/tests/test_models/test_graph/test_graph_nodes.py

Signed-off-by: Tynan DeBold <[email protected]>

Signed-off-by: Antony Milne <[email protected]>

antonymilne · 2022-06-07T15:01:18Z

OK, tests should all be fixed now 🤞

In response to my previous question about requirements, pandas and plotly should remain as core kedro-viz requirements because they're needed for the plot shown in the metadata panel for MetricsDataSet. See #554 (comment). matplotlib shouldn't be there though. The right way to get the tests working without matplotlib installed is using the import_mock.

rashidakanchwala · 2022-06-07T15:31:43Z

OK, tests should all be fixed now 🤞

In response to my previous question about requirements, pandas and plotly should remain as core kedro-viz requirements because they're needed for the plot shown in the metadata panel for MetricsDataSet. See #554 (comment). matplotlib shouldn't be there though. The right way to get the tests working without matplotlib installed is using the import_mock.

I am confused. react-plotly.js is what is used for the plot shown in the FE.

antonymilne · 2022-06-07T16:19:30Z

@rashidakanchwala yep, but on the backend there's also some stuff done with pandas and plotly to generate the metrics plots: https://github.com/kedro-org/kedro-viz/blob/main/package/kedro_viz/models/graph.py#L668.

Signed-off-by: Tynan DeBold <[email protected]>

rashidakanchwala

great work on your python PR :) looks great!

just nit on the json file having a newline.

also, please if you get a chance to update the release docs with the missing stuff shared on slack :)

Signed-off-by: Tynan DeBold <[email protected]>

….com/kedro-org/kedro-viz into feature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

* displaying the pngs in the metadata panel Signed-off-by: Tynan DeBold <[email protected]> * renaming of files, etc; expand matplotlib img Signed-off-by: Tynan DeBold <[email protected]> * fix comment in type Signed-off-by: Tynan DeBold <[email protected]> * format fix Signed-off-by: Tynan DeBold <[email protected]> * fix import Signed-off-by: Tynan DeBold <[email protected]> * another format fix Signed-off-by: Tynan DeBold <[email protected]> * added matplotlib to reqs * Added tests * fix height issue; add matplotlib node to demo project Signed-off-by: Tynan DeBold <[email protected]> * update release.md Signed-off-by: Tynan DeBold <[email protected]> * Change requirements setup Signed-off-by: Antony Milne <[email protected]> * pr-review fixes Signed-off-by: Tynan DeBold <[email protected]> * Fix tests Signed-off-by: Antony Milne <[email protected]> * Fix tests Signed-off-by: Antony Milne <[email protected]> * cleanup Signed-off-by: Tynan DeBold <[email protected]> * update release notes Signed-off-by: Tynan DeBold <[email protected]> Co-authored-by: Rashida Kanchwala <[email protected]> Co-authored-by: Antony Milne <[email protected]> Signed-off-by: Cvetanka <[email protected]>

tynandebold and others added 13 commits May 31, 2022 14:33

displaying the pngs in the metadata panel

4cd093a

Signed-off-by: Tynan DeBold <[email protected]>

renaming of files, etc; expand matplotlib img

eabe4d6

Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

41fda4f

…ature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

fix comment in type

f48e7d3

Signed-off-by: Tynan DeBold <[email protected]>

format fix

762b172

Signed-off-by: Tynan DeBold <[email protected]>

fix import

1014fde

Signed-off-by: Tynan DeBold <[email protected]>

another format fix

1c18e80

Signed-off-by: Tynan DeBold <[email protected]>

added matplotlib to reqs

21480ee

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fe…

3199a60

…ature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

Added tests

4a35a8f

Merge branch 'feature/show-matplotlib-dataset-pngs' of https://github…

e38c4a6

….com/kedro-org/kedro-viz into feature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

fix height issue; add matplotlib node to demo project

d864bad

Signed-off-by: Tynan DeBold <[email protected]>

update release.md

5d62ded

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold requested review from rashidakanchwala and antonymilne June 6, 2022 18:32

tynandebold requested review from yetudada and limdauto as code owners June 6, 2022 18:32

tynandebold removed request for limdauto and yetudada June 6, 2022 18:32

Change requirements setup

3c99820

Signed-off-by: Antony Milne <[email protected]>

antonymilne reviewed Jun 7, 2022

View reviewed changes

antonymilne approved these changes Jun 7, 2022

View reviewed changes

pr-review fixes

19c4a58

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold added the Type: Feature label Jun 7, 2022

antonymilne added 2 commits June 7, 2022 14:57

Fix tests

d9d1fa4

Signed-off-by: Antony Milne <[email protected]>

Fix tests

06f5b9a

Signed-off-by: Antony Milne <[email protected]>

tynandebold added 2 commits June 7, 2022 18:18

cleanup

4627755

Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'main' into feature/show-matplotlib-dataset-pngs

a3b2a6d

rashidakanchwala approved these changes Jun 7, 2022

View reviewed changes

tynandebold added 2 commits June 8, 2022 09:38

update release notes

dca6345

Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'feature/show-matplotlib-dataset-pngs' of https://github…

33babf7

….com/kedro-org/kedro-viz into feature/show-matplotlib-dataset-pngs Signed-off-by: Tynan DeBold <[email protected]>

tynandebold merged commit 845353d into main Jun 8, 2022

tynandebold deleted the feature/show-matplotlib-dataset-pngs branch June 8, 2022 08:57

tynandebold mentioned this pull request Jun 16, 2022

Release v4.7.0 #917

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: show matplotlib dataset pngs #887

Feature: show matplotlib dataset pngs #887

tynandebold commented Jun 6, 2022 •

edited by gitpod-io bot

Loading

antonymilne commented Jun 7, 2022

antonymilne left a comment •

edited

Loading

antonymilne Jun 7, 2022

tynandebold Jun 7, 2022

rashidakanchwala Jun 7, 2022

antonymilne Jun 7, 2022

antonymilne left a comment

antonymilne Jun 7, 2022

tynandebold Jun 7, 2022

antonymilne Jun 7, 2022

tynandebold Jun 7, 2022

antonymilne Jun 7, 2022 •

edited

Loading

antonymilne Jun 7, 2022

tynandebold Jun 7, 2022

antonymilne commented Jun 7, 2022

rashidakanchwala commented Jun 7, 2022

antonymilne commented Jun 7, 2022

rashidakanchwala left a comment •

edited

Loading

Feature: show matplotlib dataset pngs #887

Feature: show matplotlib dataset pngs #887

Conversation

tynandebold commented Jun 6, 2022 • edited by gitpod-io bot Loading

Description

Development notes

QA notes

Checklist

antonymilne commented Jun 7, 2022

antonymilne left a comment • 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

antonymilne 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

antonymilne Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonymilne commented Jun 7, 2022

rashidakanchwala commented Jun 7, 2022

antonymilne commented Jun 7, 2022

rashidakanchwala left a comment • edited Loading

Choose a reason for hiding this comment

tynandebold commented Jun 6, 2022 •

edited by gitpod-io bot

Loading

antonymilne left a comment •

edited

Loading

antonymilne Jun 7, 2022 •

edited

Loading

rashidakanchwala left a comment •

edited

Loading