Add plural flags --nodes, --tags, and --load-versions to kedro run

[AhdraMeraliQB]

Signed-off-by: Ahdra Merali ahdra.merali@quantumblack.com

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

This PR completes #2244 by adding pluralised flags for the node, tag, and load-version options on kedro run

For Reviewers:

• _split_load_versions was added to handle input strings, as previously --load-version would process the input as tuples due to multiple=True.
• .strip() is used to trim the whitespaces from user input
• I couldn't find any unit tests for the individual tags; I've added tests for the new flags but have not duplicated them for the old singular flags as they will just be deleted in 0.19.0. These tests check inputs with a singular value, multiple comma-separated values, and multiple comma-and-space-separated values.
• The test for --load-versions is subsumed under the test for _split_load_versions just as testing --load-version was by test_reformat_load_versions.
• For the period between 0.18.5 and 0.19.0 users will be able to use both versions of the flags in their kedro run commands. Whilst I don't think they will, is it worth considering limiting this behaviour?

Development notes

The helper function _split_load_versions was added as --load-version and --load-versions return different types, the singular returns a tuple and the plural a string. This is as a result of using multiple=True for the singular flag.

The both new and old tags have been tested manually to ensure they (still) behave as expected.

• Add plural flags
• Test they work for single inputs
• Test they work for multiple, comma-separated inputs
• Test behaviour against whitespaces
  N/B: Tags are robust against whitespaces
• Trim whitespaces for load-versions
• Trim whitespaces for nodes
• Add tests for plural flags (single input, multiple input)
• Make sure tests check whitespaces handling in input
• Test behaviour when using both singular and plural flags in one command - it works!
• Add test for _split_load_versions
• Add additional tests (if necessary)

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 - to be handled in Change docs to use plural flags in kedro run suggestions and examples  #2246
• Added a description of this change in the RELEASE.md file - to be handled in Add deprecation warning for --node, --tag and --load-version flags #2303
• Added tests to cover my changes

[AhdraMeraliQB]

Question for reviewers: Do we want to restrict users from using both --node and --nodes while both are available?

[AhdraMeraliQB]

This is named this way for differentiation from the already existing node_names - in #2245 I'd suggest keeping node_names as the parameter name

[antonymilne]

Fine to keep node_names for now in #2245 but as part of #2247 I think we should consider just calling this nodes. (Just while I remember, Ivan's argument for why we might not want to do this: it's deliberately written as node_names to deter people from trying to pass node objects themselves to the underlying session.run. My argument for it is consistency and simplicity, particularly when we eventually return to #1423)

[merelcht]

This looks good! I think the one thing missing is a test (or tests) to check that providing both --node and --nodes and tags, and load versions, works properly as well. Because now we're not really checking that these lines work:

tag = tag + tags
node_names = node_names + nodes_names
load_version = {**load_version, **load_versions}

[antonymilne]

Really great PR 👍 Super clear explanations and excellent testing ⭐ I'm a little bit confused about _get_values_as_tuple, but that's not really a result of your changes. I wonder if that could be simplified after (or as part of) the ticket to remove the old flags, since it will no longer need to handle anything more complicated than just a single string (?). Happy to chat more about this anyway so we can try to figure it out!

Question for reviewers: Do we want to restrict users from using both --node and --nodes while both are available?

I think it's fine as it is. I don't think anyone will really try to do that, it's only going to be possible for a limited period of time, plus they will already get the deprecation warning and things will work ok when you use both. So not worth the extra code required to handle it - doing the + to combine arguments together like you do now is more than enough I think.

[antonymilne]

Fine to keep node_names for now in #2245 but as part of #2247 I think we should consider just calling this nodes. (Just while I remember, Ivan's argument for why we might not want to do this: it's deliberately written as node_names to deter people from trying to pass node objects themselves to the underlying session.run. My argument for it is consistency and simplicity, particularly when we eventually return to #1423)

[antonymilne]

Minor, but do we need these if/else clauses? Since _get_values_as_tuple("") == tuple("") == tuple() already?

[AhdraMeraliQB]

Good catch - was following convention from the earlier implementation but looks like they weren't needed there either. My suspicion is that multiple=True implies a default value of an empty tuple 🤔

[AhdraMeraliQB]

I was unable to find any documentation on the matter but manual testing does expose that values on options where multiple=True will be tuples if the default is unspecified, even when a value is not provided. I'll remove the unnecessary ifs.

[antonymilne]

Cool, that's what I suspected about multiple=True 👍 Just to double check though, it's ok to remove the if/else for both the multiple=True and the multiple=False versions of these arguments?

[AhdraMeraliQB]

Yep! For the other flags their input is immediately handled by the callback function which goes on to return a list, and it's this list that get's passed to _get_values_as_tuple, which splits them safely

[antonymilne]

Do we need the [value] here rather than value? Elsewhere we call _get_values_as_tuple just with a single string. (Edit: is that true? Maybe it gets tuple-ified due to multiple=True? But then that wouldn't make sense for the instances you've added 🤔 )

Also do we need the if/else here? Wouldn't _reformat_load_versions work the same just passing the empty value into it directly?

After we've removed the old load_version we should definitely just move _reformat_load_versions code into this instead of having two separate functions.

[AhdraMeraliQB]

@AntonyMilneQB

Do we need the [value] here rather than value? Elsewhere we call _get_values_as_tuple just with a single string.

Without wrapping value in a list _get_values_as_tuples will treat each character in the string as a value. The function is never called on a single string as multiple=True tuplifies the values of --node and --tag, and the callback functions for --nodes and --tags both return lists.

Also do we need the if/else here? Wouldn't _reformat_load_versions work the same just passing the empty value into it directly?

The if/else isn't needed as _reformat_load_versions would still work - the idea was to prevent calling another method if unnecessary. Which approach would be considered more pythonic?

After we've removed the old load_version we should definitely just move _reformat_load_versions code into this instead of having two separate functions.

Agreed

[antonymilne]

The if/else isn't needed as _reformat_load_versions would still work - the idea was to prevent calling another method if unnecessary. Which approach would be considered more pythonic?

Overall I think without the if/else. The performance overhead of calling another function here is very small compared to the (also small) additional complexity of branching in the code. It's a small thing, but I'd only skip calling the function here if it was particularly expensive.

[AhdraMeraliQB]

@AntonyMilneQB

I'm a little bit confused about _get_values_as_tuple, but that's not really a result of your changes. I wonder if that could be simplified after (or as part of) the ticket to remove the old flags, since it will no longer need to handle anything more complicated than just a single string (?)

It should be able to be removed 🤔 it's functionality then wouldn't extend beyond split_string, if the return type is changed from list to tuple. Definitely worth evaluating in #2245

[antonymilne]

@AhdraMeraliQB

It should be able to be removed 🤔 it's functionality then wouldn't extend beyond split_string, if the return type is changed from list to tuple. Definitely worth evaluating in #2245

This would be a nice simplification. It would also be nice if we could put all this argument-munging into the callbacks rather than needing to do them inside the run function itself. Please could you add any of these proposed simplifications to #2245 (or wherever you think is appropriate)? I think we should try and do them sooner rather than later while they're fresh in our minds.

[merelcht]

Great work! 👍 Don't forget to update the release notes.

[AhdraMeraliQB]

@merelcht

Great work! 👍 Don't forget to update the release notes.

The changes were combined in the release notes of #2303: "Deprecate (singular flags) in favour of (plural flags)" - should this be elaborated on further?

[merelcht]

The changes were combined in the release notes of #2303: "Deprecate (singular flags) in favour of (plural flags)" - should this be elaborated on further?

Yes, I would separately add that these flags have been added. That way it's also easier to trace which PR the changes were added in.