Feature/slicing pipeline #1980

Huongg · 2024-07-11T12:38:10Z

Description

Fixes #1974,

User Story: As a Kedro user, I want to preview which nodes are included in my selection for slicing so that I can understand what my slice will be before creating a new slice

Development notes

This is a feature branch, and won't merge to main until all the other functionalities are merged and ready.

Architecture diagram as below:

QA notes

When testing on your machine locally, or through gitpod, please note that this PR only covers the functionalities below:

Chart Highlighting: The two nodes selected with Shift held should be highlighted with a darker blue background, and any nodes in between should be highlighted with a lighter blue background.
Nodes List Highlighting: The nodes list on the left-hand side should also highlight all the nodes included in the newly selected pipeline.
Call to Action: A button at the bottom should display the number of nodes selected and include a "Slice" button.
Outside Click: Clicking outside the selection will un-select all the nodes, and go back to the default/previous page. This includes clicking on any other nodes outside the sliced pipeline

Pending design tasks that are waiting for @stephkaiser . Please ignore them when you review the PR

Node List Highlighting: the issue happens when the node list is currently highlighting, and you try to hover on other node. The hover style will cover the highlighting style

The slice button icon

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

src/components/flowchart/components/slice-pipeline-action/slice-pipeline-action.js

rashidakanchwala · 2024-07-25T09:35:24Z

src/components/flowchart/draw.js

@@ -136,6 +150,16 @@ export const drawNodes = function (changed) {
    focusMode,
    hoveredFocusMode,
  } = this.props;
+  const { filteredPipelineState } = this.state;
+
+  const fromToFilteredPipeline = createNodeStateMap(nodes, [


these names are a bit confusing. I cannot come up with better names but below is the best I could.
I am also ok if we stick to these names but lets leave a comment on top to define what they do

// Map the state of nodes selected by the user (from and to nodes) const selectedNodesState = createNodeStateMap(nodes, [ filteredPipelineState.from, filteredPipelineState.to, ]); // Map the state of nodes that fall within the range between the selected nodes const nodesInRangeState = createNodeStateMap( nodes, filteredPipelineState.range );

let me have a think, I'll come back to you on this

rashidakanchwala · 2024-07-25T09:37:19Z

src/components/flowchart/draw.js

+ * @param {Array} values - Array of values to check against node IDs.
+ * @returns {Object} An object mapping node IDs to booleans.
+ */
+function createNodeStateMap(nodes, values) {


rashidakanchwala · 2024-07-25T12:16:49Z

src/components/flowchart/flowchart.js

@@ -90,6 +102,34 @@ export class FlowChart extends Component {

  componentDidUpdate(prevProps) {
    this.update(prevProps);
+
+    const isFilteredPipelineChanged =


can we add this code logic to update(prevProps = {}) in line 138?

and then do if changed(filteredPipeline) { } like the rest

src/components/flowchart/flowchart.js

src/store/normalize-data.js

src/selectors/filtered-pipeline.js

src/actions/slice.js

src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js

.eslintrc.json

src/components/flowchart/flowchart.js

jitu5 · 2024-07-26T16:14:42Z

@Huongg I observed few things when testing locally.

I can selected to same level nodes, Is it expected in this PR and we will handle this in separate PR?

Not sure about expected behaviour, If I try to hide the visibility of currently highlighted nodes from node list, it does work so we might need to disable visibility button of highlighted nodes?

Huongg · 2024-07-26T18:36:31Z

@Huongg I observed few things when testing locally.

I can selected to same level nodes, Is it expected in this PR and we will handle this in separate PR?

* Not sure about expected behaviour, If I try to hide the visibility of currently highlighted nodes from node list, it does work so we might need to disable visibility button of highlighted nodes?

Hi @jitu5 this is expected, it's the same behaviour from framework side. I've checked with @noklam and he confirmed it. So when user slices 2 nodes not linked to each other, this is what will be shown on the chart

ravi-kumar-pilla · 2024-07-30T20:34:57Z

Hi @Huongg , This looks great. I have few questions, which I am not sure if it is by design -

Should the from node be always on an upper level compared to to node ? i.e., in our demo_project can I select from as split_data and to as companies ?
When I select a modular_pipeline using shift it gets expanded but none of the nodes are selected. Is it by design that shift only works with nodes ?

Thank you

ravi-kumar-pilla · 2024-07-30T20:51:09Z

src/actions/slice.js

+
+export const RESET_SLICE_PIPELINE = 'RESET_SLICE_PIPELINE';
+
+export const resetSlicePipeline = () => ({


[Nit] - just to have an opposite for applySlice

export const resetSlice = () => ({
type: RESET_SLICE,
slice: { from: null, to: null },
});

src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js

src/components/node-list/index.js

src/store/normalize-data.js

src/reducers/slice.js

ravi-kumar-pilla · 2024-07-30T21:46:52Z

src/selectors/disabled.js

  ) =>
    arrayToObject(nodeIDs, (id) => {
      let isDisabledViaModularPipeline =
        disabledModularPipeline[nodeModularPipelines[id]];

+      let isDisabledViaFilters = false;
+      if (slicedPipeline.length > 0 && isSliceApplied) {


Suggested change

if (slicedPipeline.length > 0 && isSliceApplied) {

if (isSliceApplied && slicedPipeline.length > 0) {

could you please let me know what happens if getNodeDisabled returns True/False and how can a node be disabled via a slice ? Is this used for a sliced view ?

hey i dont think getNodeDisabled would return true or false, it will return an object with IDs and its value, eg:

{ 1e3cc50a :false 3b199c6b: false 3fb71518: true }

so if its false, the node/element on the chart will be faded out. So any nodes in slicedPipeline will have the value of true, and the rest will be false. Does it make sense?

src/selectors/disabled.js

Huongg · 2024-07-31T08:30:08Z

Hi @Huongg , This looks great. I have few questions, which I am not sure if it is by design -

Should the from node be always on an upper level compared to to node ? i.e., in our demo_project can I select from as split_data and to as companies ?

When I select a modular_pipeline using shift it gets expanded but none of the nodes are selected. Is it by design that shift only works with nodes ?

Thank you

hey @ravi-kumar-pilla,

first point is covered in this user story 1973, we're going to explore changing the order in the command line and let the user selects the bottom node to top node.

for second point, currently it's only working with node and dataset in the MVP 1 i think, we can certainly explore the modular_pipeline but will need to check with the framework team to see if this is something would work in framework too

rashidakanchwala · 2024-08-14T09:45:39Z

src/components/flowchart/components/sliced-pipeline-action-bar/sliced-pipeline-action-bar.js

@@ -0,0 +1,27 @@
+import React from 'react';


I feel like this should be in components folder not flowchart folder.

rashidakanchwala

Look's great @Huongg . I have created #2037 and #2038 as I found a few design inconsistencies when reviweing the PR (not related to slicing but from before) and we can work on it eventually.

ravi-kumar-pilla · 2024-08-14T17:19:04Z

Awesome feature for Viz and looking forward to the release. Thank you @Huongg .... looks great !! 💯

jitu5

Some point we discussed during the review process like click on action bar event and slicing during embedded mode. May be we can improvise on it later but for this PR Great work @Huongg !!

Huongg self-assigned this Jul 11, 2024

Huongg marked this pull request as ready for review July 16, 2024 09:48

Huongg requested review from rashidakanchwala and jitu5 as code owners July 16, 2024 09:48

Huongg changed the title ~~[DRAFT] Feature/slicing pipeline~~ Feature/slicing pipeline Jul 16, 2024

Huongg requested review from stephkaiser, ravi-kumar-pilla and SajidAlamQB July 16, 2024 09:48

rashidakanchwala reviewed Jul 25, 2024

View reviewed changes

src/components/flowchart/components/slice-pipeline-action/slice-pipeline-action.js Outdated Show resolved Hide resolved

rashidakanchwala reviewed Jul 25, 2024

View reviewed changes

src/components/flowchart/flowchart.js Outdated Show resolved Hide resolved

ravi-kumar-pilla reviewed Jul 25, 2024

View reviewed changes

src/store/normalize-data.js Outdated Show resolved Hide resolved

ravi-kumar-pilla reviewed Jul 25, 2024

View reviewed changes

src/selectors/filtered-pipeline.js Outdated Show resolved Hide resolved

jitu5 reviewed Jul 26, 2024

View reviewed changes