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

Inconsistency in kedro new with --config #3335

Closed
ankatiyar opened this issue Nov 23, 2023 · 4 comments · Fixed by #3499
Closed

Inconsistency in kedro new with --config #3335

ankatiyar opened this issue Nov 23, 2023 · 4 comments · Fixed by #3499

Comments

@ankatiyar
Copy link
Contributor

ankatiyar commented Nov 23, 2023

Description

Creating a new project with the --config flag doesn't work properly. There's multiple inconsistencies in the flow.

Problem 1:

#3342

Problem 2:

#3343

Problem 3:

  • There's inconsistency between the flag names --example and the fields you have to use in config.yml and prompts instead, i.e. example_pipeline.

According to the discussion below, we decided to leave it as is

Problem 4:

  • tools in config.yml and prompts takes numeric values 1,2,3 but --tools takes lint,test,docs

According to the discussion below, we decided to use short text in config.yml instead of numbers to be consistent with --tools

@DimedS
Copy link
Contributor

DimedS commented Jan 4, 2024

Problems 3 and 4 are still actual, but I'm uncertain about solution.

  • For problem 3 I think it will be better if we will unify user interface and always use "example" - for prompts, CLI and config. But if I understood correctly it will be a breaking change and I don't sure is it possible now.

  • For problem 4, looks like we decided to use numbers in prompts because user can see explanation of that numbers just before writing them in prompts and numbers allows to write faster. And for CLI it's more convenient to use short words. I think it's also more convenient to use short words in config for the same reason, but again it looks like a breaking change.

I think it should be good to discuss solution first, do you have any thoughts on that: @ankatiyar , @SajidAlamQB , @lrcouto , @AhdraMeraliQB , @noklam , @merelcht

@AhdraMeraliQB
Copy link
Contributor

AhdraMeraliQB commented Jan 5, 2024

I agree, that these are both breaking, I think the idea was these were nice-to-haves to be done before 0.19.0 that didn't make the cut.

For 3, I don't see much of an issue here, I assume --example is short for example_pipeline, not unsimilar from how --nodes is used in the cli command, but it's node_names in config: see kedro run config docs

Regarding 4, I would agree that it would be nice to have a single standard way of providing this information, but I do think this would require input from design (and given it's a breaking change, we might get user feedback on what they'd like, or if they mind, before the time for implementing this comes around).

A way to get around the breaking change would be to agree on what we would like it changed to, implement it, but keep the current system as well (with a deprecation warning at some point), removing the old syntax at 0.20.0/1.0.0. It would probably be around for quite some time though.

In summary, I think we can leave point 3. For point 4 I'm not opposed to creating a separate issue for the design -> implementation -> deprecation -> eventual-removal process, but at a Low priority

@merelcht
Copy link
Member

merelcht commented Jan 5, 2024

Technically these changes are indeed breaking, but the config.yml file is a very infrequently used feature so I personally would just ship these in a non-breaking release. That said, I feel like 3 might not be worth changing, because like @AhdraMeraliQB pointed out, other values have different names as well. We should just make clear the docs describe this clearly. Point 4 I would change. I don't think it's worth doing full user research on it, because users don't use config.yml much and many probably haven't switched to Kedro 0.19.1 yet.

@ankatiyar
Copy link
Contributor Author

+1, I think issue 3 is not that big a deal as long as the documentation makes it clear to use example_pipeline in config.yml
Regarding issue 4, I think we should use the short word forms of tools - lint, docs, test etc in config.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants