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

Spike: proposal for better default node names #3575

Open
astrojuanlu opened this issue Jan 30, 2024 · 10 comments
Open

Spike: proposal for better default node names #3575

astrojuanlu opened this issue Jan 30, 2024 · 10 comments

Comments

@astrojuanlu
Copy link
Member

astrojuanlu commented Jan 30, 2024

Description

When a node has no name, the default is to use a string representation for it. For example:

'split_data([model_input_table; params: model_options]) -> [X_train;X _test;y_train;y_test]'

This poses a series of problems:

Context

I contend that 'split_data([model_input_table; params: model_options]) -> [X_train;X _test;y_train;y_test]' is a bad name because:

  • A human would never name their nodes like that
  • It's more of a description or a __repr__ rather than a "name"

Plus all the problems discussed above.

Task

Come up with a proposal for a better default node name, taking into account:

  • backwards compatibility
  • filtering by node should be user with the new default
  • requirements outlined in discussion

Possible Implementation

Use func.__name__ as default node names.

@deepyaman
Copy link
Member

Use func.__name__ as default node names.

Don't node names need to be unique? Even if not ideal, the current approach guarantees uniqueness, while the latter very frequently won't.

@astrojuanlu
Copy link
Member Author

Correct: the current approach guarantees uniqueness under the current assumptions of "no two nodes can output the same dataset".

More strawman proposals:

  • f"{func.__name__}_{uuid4()}
  • f"{func.__name__}_0 (and continues with _1, _2 if node functions are reused)
  • f"{func.__name__}_{simplify(self.outputs())} (for instance split_data__X_train_X_test_y_train_y_test)

More ideas?

@noklam
Copy link
Contributor

noklam commented Feb 7, 2024

It may be obvious, but can I ask why node name need to be unique? I feel like they exists for different purposes, I would like to map them out and see if there are opportunities to combine them. There would be at least 2 different purpose, one is for internal working, the other for human-readable.

Currently there are different names.

  • node.name (with namespace)
  • node.short_name
  • node._name
  • node._unique_key (hashable)
  • node._func_name

Noted some of this already exist in Node API, though it may not be what you expected.

def _get_readable_func_name(func: Callable) -> str:
    """Get a user-friendly readable name of the function provided.

    Returns:
        str: readable name of the provided callable func.
    """

    if hasattr(func, "__name__"):
        return func.__name__

Re: @deepyaman point, I think is fair for any medium size pipeline using namespace, but I think there are also many pipelines without repeated node function.

p.s. I am asking this just because I am lazy, I will do the digging at some point, but if someone already know that could save some time.

@astrojuanlu
Copy link
Member Author

Agreed with the sentiment. I imagine a "node ID" should be unique, but not sure why we want names to be unique.

@noklam
Copy link
Contributor

noklam commented Feb 9, 2024

This is my understanding:

  • node._unique_key = node.name + inputs + outputs

This definition is a bit leaky, because I will think combination of (func, inputs + outputs) is already an unique key. I test this and Kedro thinks they are valid:

  • node(func, "x", None) and node(func, "x", None, name="abc") , Kedro treat them as "different node"
  • If you extend this, does adding "namespace" to the name of the function but keep the same input and output (assume it's None because output cannot be duplicate anyway) makes it a different Node?

I tend to say it's NO, they are the same.

Back to the original idea, _unique_key should be used as the "Node ID". You may be surprised that _validate_duplicate_nodes is using node.name to compare instead of node._unique_key. I do not understand why this is the case, but seems that a lot of the __eq__ , __lt__ was created for toposort, Kedro doesn't use this to compare node.

If we offload this responsibility to _unique_key, maybe we can use node.name to be something more human-readable, or even allow it to be non-unique. namespace.func is not a bad choice for default node name. The node name doesn't matter to Kedro, but only to the user.

What if user provide a name but there are multiple nodes return with the same name? This affect things such as %load_node <node_name>, kedro run --to-nodes <node_name>

2 options:

  • allow user to provide the non human readable name, which no one can remember what is it unless they copy from the log when the pipeline fails and Kedro throw a resume suggestion.
  • In case of ambiguity, Kedro don't do anything. Ask user to give the node a name to make it unique.

Con:

  • Technically a breaking change, but does anyone rely on the node.name?

Pro:

  • Keep the node uniqueness guarantee
  • sensible default name that works 95%, in case of ambiguity, prompt user to add name.

Note that having duplicate names doesn't mean that you need to change the name immediately, you only have to do so if you have to specify it as an argument (smaller chance)

@astrojuanlu
Copy link
Member Author

You may be surprised that _validate_duplicate_nodes is using node.name to compare instead of node._unique_key

Maybe this is a good place to start?

seems that a lot of the __eq__ , __lt__ was created for toposort, Kedro doesn't use this to compare node.

Now that we switched to graphlib #3728, would it be interesting to try to do some "tree-shaking" and see if we can remove (after deprecation) some of these methods?

@noklam
Copy link
Contributor

noklam commented Apr 8, 2024

The comment was mostly based on the investigation that I have done 2 months ago, they are documented a bit more in details here: https://noklam.github.io/blog/posts/default_node_name/2024-02-08-default-node-name.html

Now that we switched to graphlib #3728, would it be interesting to try to do some "tree-shaking" and see if we can remove (after deprecation) some of these methods?

We can definitely try this, but this wouldn't address the problem described in this issue, it may helps with removing the API surface.

The main proposal of #3575 (comment) is that can we loosen up to make node.name non-unique, and offload that uniqueness checking to node._unique_key instead.

@astrojuanlu
Copy link
Member Author

Related: one user told me today that he found node names "weird" and "useless", given that they were inferred from function names already.

@astrojuanlu
Copy link
Member Author

When discussing kedro-org/kedro-viz#2036 today, we realised that a few more surprising things about names:

  • The current automatically generated node names aren't visible in the Viz UI (too ugly to display?)
  • Kedro is prepending {namespace}. to user-specified node names
  • It was confusing that kedro --to-nodes=function_name wasn't working, and the reason was that the node had no name

This is on top of what we already knew:

  • It's very annoying having to name a node every time you want to use it for something (see my previous comment)
  • The current approach to automatically naming nodes is very problematic (see examples)
    • And now we will add one more problem, because when the slicing functionality is live, these automatically generated node names will start popping up on the generated kedro run commands.

I think we need to plan what to do about this.

@lordsoffallen
Copy link
Contributor

Also adding a similar use where it doesnt work if name is not specified:

From slack chat:

I noticed something that when i define the following node:

node(
    func=sample_func,
    # name="sample_func"    -> does not work when this is commented
    inputs="epubdf",
    outputs="result"
)

and then run this test:

kedro_session.run(node_names=['sample_func'])

I get an error saying name doesnt exists but when i specify name parameter in node itself it works.

@merelcht merelcht changed the title Default node names are problematic Spike: proposal for better default node names Sep 23, 2024
@merelcht merelcht added this to the Improve Developer Experience milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

5 participants