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

Fix comma splitting for node re-run suggestion #2012

Closed
merelcht opened this issue Nov 9, 2022 · 2 comments · Fixed by #2139
Closed

Fix comma splitting for node re-run suggestion #2012

merelcht opened this issue Nov 9, 2022 · 2 comments · Fixed by #2139
Assignees
Labels
Component: CLI Issue/PR that addresses the CLI for Kedro Issue: Bug Report 🐞 Bug that needs to be fixed

Comments

@merelcht
Copy link
Member

merelcht commented Nov 9, 2022

Description

Running kedro with the command kedro run --from-nodes "two_inputs([A0,B0]) -> [C1]" will cause an error: Pipeline does not contain nodes named ['B0]) -> [C1]', 'two_inputs([A0'].

Kedro has incorrectly split the name of the intended Node into the names of two Nodes that do not exist.

This bug prevents the user from re-running kedro from any Node with a comma in its name. This is the case for any Node that is not explicitly given a name and has more than one input or output.

Steps to Reproduce

  1. Create a kedro project
  2. Attempt to run the pipeline from a node with a comma in its name (kedro run --from-nodes 'My node, a good node')
  3. Receive the error Pipeline does not contain nodes named ['a good node', 'My node'].

Context

#1828

Implementation

@jmholzer idea of a fine-state machine csv-parsing type solution to do the string splitting rather than just blindly splitting on ,. This would basically mean that , inside () would not split the string.

@merelcht merelcht added Issue: Bug Report 🐞 Bug that needs to be fixed Component: CLI Issue/PR that addresses the CLI for Kedro labels Nov 9, 2022
@jmholzer
Copy link
Contributor

I have an implementation of this ready to go. Now that I have started manually testing my changes, I'm starting to think that once #2013 is merged, we'll never see Nodes with names that contain commas. If you try to instantiate one using node (or Node), you'll see the following error:

Image

Is there a way to instantiate a node with such a name that I am not thinking of?

@merelcht
Copy link
Member Author

merelcht commented Dec 5, 2022

I have an implementation of this ready to go. Now that I have started manually testing my changes, I'm starting to think that once #2013 is merged, we'll never see Nodes with names that contain commas. If you try to instantiate one using node (or Node), you'll see the following error:

Image

Is there a way to instantiate a node with such a name that I am not thinking of?

This will still need to be done for the Kedro 0.18.x series. But you're right that from 0.19.0 onwards we shouldn't be seeing Node names with commas anymore.

@jmholzer jmholzer self-assigned this Dec 16, 2022
@jmholzer jmholzer moved this to In Progress in Kedro Framework Dec 16, 2022
@jmholzer jmholzer moved this from In Progress to In Review in Kedro Framework Dec 20, 2022
@github-project-automation github-project-automation bot moved this from In Review to Done in Kedro Framework Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CLI Issue/PR that addresses the CLI for Kedro Issue: Bug Report 🐞 Bug that needs to be fixed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants