Fix `--tools=all` not working properly #3361

SajidAlamQB · 2023-11-29T15:24:27Z

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

Related to: #3334

When you use --tools=all or --tools all in kedro new directly you don't get all addons as expected.
You can also see in the message:

You have selected the following tools:

Development notes

The problem looks to be coming from _convert_tool_names_to_numbers.

kedro/kedro/framework/cli/starters.py

Lines 469 to 490 in 7d295e8

    
           def _convert_addon_names_to_numbers(selected_add_ons_flag: str | None) -> str | None: 
        
               """Prepares add-on selection from the CLI input to the correct format 
        
               to be put in the project configuration, if it exists. 
        
               Replaces add-on strings with the corresponding prompt number. 
        
               Args: 
        
                   selected_add_ons_flag: a string containing the value for the --addons flag, 
        
                       or None in case the flag wasn't used, i.e. lint,docs. 
        
               Returns: 
        
                   String with the numbers corresponding to the desired add_ons, or 
        
                   None in case the --addons flag was not used. 
        
               """ 
        
               if selected_add_ons_flag is None: 
        
                   return None 
        
               addons = [] 
        
               for addon in selected_add_ons_flag.lower().split(","): 
        
                   addon_short_name = addon.strip() 
        
                   if addon_short_name in ADD_ONS_SHORTNAME_TO_NUMBER: 
        
                       addons.append(ADD_ONS_SHORTNAME_TO_NUMBER[addon_short_name]) 
        
               return ",".join(addons)

We don't have a check for the "all" flag. We should probably also have "none" check here as well.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

Read the contributing guidelines
Signed off each commit with a Developer Certificate of Origin (DCO)
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the RELEASE.md file
Added tests to cover my changes
Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> Signed-off-by: lrcouto <laurarccouto@gmail.com>

Signed-off-by: lrcouto <laurarccouto@gmail.com>

…to bugfix/addons=all-fix

…kedro into bugfix/addons=all-fix Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>

merelcht

Do we have any tests that should have caught this?

deepyaman · 2023-11-30T15:36:02Z

Do we have any tests that should have caught this?

+1 was going to say the same thing, then saw @merelcht already did :D

SajidAlamQB · 2023-11-30T15:39:49Z

Do we have any tests that should have caught this?

Yes, I believe we have the tests already, @lrcouto, found that this problem wasn't originally picked up by the tests as it was using the faulty function which shouldn't be a problem now that it's fixed. Right @lrcouto?

DimedS · 2023-11-30T16:10:32Z

kedro/framework/cli/starters.py

+    if selected_tools.lower() == "none":
+        return ""
+    if selected_tools.lower() == "all":
+        return ",".join(TOOLS_SHORTNAME_TO_NUMBER.values())


Maybe we just need to update the TOOLS_SHORTNAME_TO_NUMBER dictionary? Put "none" and "all" there? Or it will break something else?

I was thinking this as well but it feels a bit weird as "all" and "none" aren't tools.

AhdraMeraliQB · 2023-11-30T16:11:56Z

Do we have any tests that should have caught this?

Yes, I believe we have the tests already, @lrcouto, found that this problem wasn't originally picked up by the tests as it was using the faulty function which shouldn't be a problem now that it's fixed. Right @lrcouto?

😬 The unchecked point of failure is a little concerning

I noticed in test_starters.py we only really test the helper function _parse_tools_input. I think we can mitigate something like this again by testing the others as well - would it be in scope to add some quick unit tests for _parse_yes_no_to_bool and _validate_selection, as well as _convert_tools_names_to_numbers?

SajidAlamQB · 2023-11-30T16:16:31Z

I noticed in test_starters.py we only really test the helper function _parse_tools_input. I think we can mitigate something like this again by testing the others as well - would it be in scope to add some quick unit tests for _parse_yes_no_to_bool and _validate_selection, as well as _convert_tools_names_to_numbers?

Sounds good to me 👍

Signed-off-by: lrcouto <laurarccouto@gmail.com>

DimedS

Thanks, @lrcouto and @SajidAlamQB , great work!

AhdraMeraliQB

Love the new tests, just a few comments:

AhdraMeraliQB · 2023-12-05T17:24:56Z

tests/framework/cli/test_starters.py

@@ -1324,3 +1324,92 @@ def test_invalid_names(self, fake_kedro_cli, name):
            "Kedro project names must contain only alphanumeric symbols, spaces, underscores and hyphens and be at least 2 characters long"
            in result.output
        )
+
+
+class TestParseYesNoToBools:


Could we move the tests for parse_tools_input into their own class as well? For consistency

AhdraMeraliQB · 2023-12-05T17:25:43Z

tests/framework/cli/test_starters.py

+    def test_validate_selection_invalid_single_tool(self):
+        tools = ["8"]
+        with pytest.raises(SystemExit):
+            _validate_selection(tools)


Could we add an assert for the correct error message? E.g the implementation for test_parse_tools_invalid_selection

Signed-off-by: lrcouto <laurarccouto@gmail.com>

kedro/framework/cli/starters.py

Signed-off-by: lrcouto <laurarccouto@gmail.com>

…to bugfix/addons=all-fix

merelcht

LGTM! 👍

add fix

5faebf3

SajidAlamQB self-assigned this Nov 29, 2023

SajidAlamQB linked an issue Nov 29, 2023 that may be closed by this pull request

--addons=all doesn't behave as expected #3334

Closed

Merge branch 'develop' into bugfix/addons=all-fix

0f61dd5

SajidAlamQB assigned lrcouto Nov 29, 2023

SajidAlamQB and others added 3 commits November 30, 2023 00:59

add fix

7e7ca3f

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Rename addons to tools (#3357)

16ef432

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> Signed-off-by: lrcouto <laurarccouto@gmail.com>

Fix broken tests (#3366)

87e2e1e

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> Signed-off-by: lrcouto <laurarccouto@gmail.com>

lrcouto force-pushed the bugfix/addons=all-fix branch from ad8d5dc to 87e2e1e Compare November 30, 2023 03:59

lrcouto and others added 3 commits November 30, 2023 00:59

Merge branch 'develop' into bugfix/addons=all-fix

9ab8759

Fix variable names on _convert_tool_names_to_numbers

f127b48

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Merge branch 'bugfix/addons=all-fix' of github.com:kedro-org/kedro in…

4090586

…to bugfix/addons=all-fix

lrcouto marked this pull request as ready for review November 30, 2023 04:54

lrcouto requested a review from merelcht as a code owner November 30, 2023 04:54

SajidAlamQB added 3 commits November 30, 2023 11:27

Merge branch 'bugfix/addons=all-fix' of https://github.com/kedro-org/…

47c4118

…kedro into bugfix/addons=all-fix Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>

Merge branch 'develop' into bugfix/addons=all-fix

70985cc

Merge branch 'develop' into bugfix/addons=all-fix

a3eea61

SajidAlamQB requested a review from AhdraMeraliQB November 30, 2023 13:40

merelcht reviewed Nov 30, 2023

View reviewed changes

SajidAlamQB changed the title ~~Fix --addons=all not working properly~~ Fix --tools=all not working properly Nov 30, 2023

DimedS reviewed Nov 30, 2023

View reviewed changes

lrcouto and others added 4 commits November 30, 2023 13:30

Merge branch 'develop' into bugfix/addons=all-fix

fd7b734

Merge branch 'develop' into bugfix/addons=all-fix

6888463

Merge branch 'develop' into bugfix/addons=all-fix

143b204

Add tests for helper functions

ba0d696

Signed-off-by: lrcouto <laurarccouto@gmail.com>

SajidAlamQB requested a review from DimedS December 5, 2023 15:48

SajidAlamQB requested a review from merelcht December 5, 2023 15:48

Merge branch 'develop' into bugfix/addons=all-fix

dab4d29

DimedS approved these changes Dec 5, 2023

View reviewed changes

AhdraMeraliQB reviewed Dec 5, 2023

View reviewed changes

lrcouto and others added 6 commits December 5, 2023 18:18

Merge branch 'develop' into bugfix/addons=all-fix

d6672a3

Merge branch 'develop' into bugfix/addons=all-fix

f9802c7

Merge branch 'develop' into bugfix/addons=all-fix

b353f63

Lint

545755a

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Merge branch 'develop' into bugfix/addons=all-fix

4bc1748

Merge branch 'develop' into bugfix/addons=all-fix

93d5b50

merelcht reviewed Dec 7, 2023

View reviewed changes

kedro/framework/cli/starters.py Outdated Show resolved Hide resolved

lrcouto and others added 4 commits December 7, 2023 13:18

Merge branch 'develop' into bugfix/addons=all-fix

c875b3c

Return numbers to tools names on conversion from all

3df74ab

Signed-off-by: lrcouto <laurarccouto@gmail.com>

Merge branch 'bugfix/addons=all-fix' of github.com:kedro-org/kedro in…

a821ede

…to bugfix/addons=all-fix

Merge branch 'develop' into bugfix/addons=all-fix

d33b745

merelcht approved these changes Dec 8, 2023

View reviewed changes

SajidAlamQB added 3 commits December 8, 2023 10:48

Merge branch 'develop' into bugfix/addons=all-fix

a228843

Merge branch 'develop' into bugfix/addons=all-fix

31d7a3f

Merge branch 'develop' into bugfix/addons=all-fix

3e6666e

SajidAlamQB enabled auto-merge (squash) December 8, 2023 13:26

SajidAlamQB disabled auto-merge December 8, 2023 13:33

SajidAlamQB enabled auto-merge (squash) December 8, 2023 13:57

SajidAlamQB merged commit 074d5e1 into develop Dec 8, 2023
30 checks passed

SajidAlamQB deleted the bugfix/addons=all-fix branch December 8, 2023 14:09

SajidAlamQB mentioned this pull request Dec 8, 2023

--addons=all doesn't behave as expected #3334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `--tools=all` not working properly #3361

Fix `--tools=all` not working properly #3361

SajidAlamQB commented Nov 29, 2023 •

edited

Loading

merelcht left a comment •

edited

Loading

deepyaman commented Nov 30, 2023

SajidAlamQB commented Nov 30, 2023

DimedS Nov 30, 2023

SajidAlamQB Nov 30, 2023

AhdraMeraliQB commented Nov 30, 2023

SajidAlamQB commented Nov 30, 2023

DimedS left a comment

AhdraMeraliQB left a comment

AhdraMeraliQB Dec 5, 2023

AhdraMeraliQB Dec 5, 2023 •

edited

Loading

merelcht left a comment

	def _convert_addon_names_to_numbers(selected_add_ons_flag: str \| None) -> str \| None:
	"""Prepares add-on selection from the CLI input to the correct format
	to be put in the project configuration, if it exists.
	Replaces add-on strings with the corresponding prompt number.

	Args:
	selected_add_ons_flag: a string containing the value for the --addons flag,
	or None in case the flag wasn't used, i.e. lint,docs.

	Returns:
	String with the numbers corresponding to the desired add_ons, or
	None in case the --addons flag was not used.
	"""
	if selected_add_ons_flag is None:
	return None

	addons = []
	for addon in selected_add_ons_flag.lower().split(","):
	addon_short_name = addon.strip()
	if addon_short_name in ADD_ONS_SHORTNAME_TO_NUMBER:
	addons.append(ADD_ONS_SHORTNAME_TO_NUMBER[addon_short_name])
	return ",".join(addons)

Fix --tools=all not working properly #3361

Fix --tools=all not working properly #3361

Conversation

SajidAlamQB commented Nov 29, 2023 • edited Loading

Description

Development notes

Developer Certificate of Origin

Checklist

merelcht left a comment • edited Loading

Choose a reason for hiding this comment

deepyaman commented Nov 30, 2023

SajidAlamQB commented Nov 30, 2023

DimedS Nov 30, 2023

Choose a reason for hiding this comment

SajidAlamQB Nov 30, 2023

Choose a reason for hiding this comment

AhdraMeraliQB commented Nov 30, 2023

SajidAlamQB commented Nov 30, 2023

DimedS left a comment

Choose a reason for hiding this comment

AhdraMeraliQB left a comment

Choose a reason for hiding this comment

AhdraMeraliQB Dec 5, 2023

Choose a reason for hiding this comment

AhdraMeraliQB Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

Fix `--tools=all` not working properly #3361

Fix `--tools=all` not working properly #3361

SajidAlamQB commented Nov 29, 2023 •

edited

Loading

merelcht left a comment •

edited

Loading

AhdraMeraliQB Dec 5, 2023 •

edited

Loading