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.

Sign up for GitHub

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

Jump to bottom

Update kedro pipeline create to use new /conf file structure #2856

Merged
merged 15 commits into from
Aug 7, 2023
Merged

Update kedro pipeline create to use new /conf file structure #2856

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2a342eb
add parameters_pipelineNane.yml template in the conf folder
DimedS Jul 26, 2023
c0e4fcc
del yml template from parameters folder
DimedS Jul 26, 2023
c1459dc
modify delete pipeline command to use correct path to yml files
DimedS Jul 26, 2023
8c424b1
modify test_pipeline move parameters foldername to the yml filename
DimedS Jul 27, 2023
6d60a24
collect configs to package not only from parameters folder, but from …
DimedS Jul 27, 2023
35c6d57
move parameters foldername to the yml filename, remove nesting
DimedS Jul 27, 2023
6cd430a
move parameters foldername to the yml filename, remove nesting
DimedS Jul 27, 2023
f5e69a8
Add changes to RELEASE.md
DimedS Jul 31, 2023
73be9a1
Merge branch 'main' into update-pipeline-create-conf
DimedS Jul 31, 2023
e6c5732
Update RELEASE.md
DimedS Aug 4, 2023
2d77cfe
Merge branch 'main' into update-pipeline-create-conf
DimedS Aug 4, 2023
47c2d2e
Update RELEASE.md, change to release 0.18.13
DimedS Aug 4, 2023
e0cde1b
save old structure for 'pipeline delete' command for backward compati…
DimedS Aug 4, 2023
2c1e863
modify test_pipeline for 'pipeline delete' backward compability
DimedS Aug 4, 2023
44f3b4f
Merge branch 'main' into update-pipeline-create-conf
DimedS Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* Consolidated dependencies and optional dependencies in `pyproject.toml`.
* Made validation of unique node outputs much faster.
* Updated `kedro catalog list` to show datasets generated with factories.
* Updated kedro pipeline create to use new /conf file structure.
DimedS marked this conversation as resolved.
Show resolved Hide resolved

## Documentation changes
* Recommended `ruff` as the linter and removed mentions of `pylint`, `isort`, `flake8`.
Expand Down
9 changes: 8 additions & 1 deletion kedro/framework/cli/micropkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,9 +613,16 @@ def _package_micropkg(
)
# as the source distribution will only contain parameters, we aren't listing other
# config files not to confuse users and avoid useless file copies
# collect configs to package not only from parameters folder, but from core conf folder also
# because parameters had been moved from foldername to yml filename
configs_to_package = _find_config_files(
package_conf,
[f"parameters*/**/{micropkg_name}.yml", f"parameters*/**/{micropkg_name}/**/*"],
[
f"**/parameters_{micropkg_name}.yml",
f"**/{micropkg_name}/**/*",
f"parameters*/**/{micropkg_name}.yml",
f"parameters*/**/{micropkg_name}/**/*",
],
Comment on lines +620 to +625
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this configurable? what if users have custom directory which isn't named "parameters"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nok hi, thank you for your review. In my opinion, currently it's not configurable. However, as you've suggested, it seems like a good idea to refactor the micropkg module code in a separate ticket. At this time, I've simply updated the default 'parameters' structure while maintaining backward compatibility.

)

source_paths = (package_source, package_tests, configs_to_package)
Expand Down
4 changes: 2 additions & 2 deletions kedro/framework/cli/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ def delete_pipeline(
pipeline_artifacts = _get_pipeline_artifacts(metadata, pipeline_name=name, env=env)

files_to_delete = [
pipeline_artifacts.pipeline_conf / confdir / f"{name}.yml"
pipeline_artifacts.pipeline_conf / f"{confdir}_{name}.yml"
for confdir in ("parameters", "catalog")
if (pipeline_artifacts.pipeline_conf / confdir / f"{name}.yml").is_file()
if (pipeline_artifacts.pipeline_conf / f"{confdir}_{name}.yml").is_file()
]
dirs_to_delete = [
path
Expand Down
0 ...ters/{{ cookiecutter.pipeline_name }}.yml → ...ters_{{ cookiecutter.pipeline_name }}.yml
View file
Open in desktop
File renamed without changes.
8 changes: 4 additions & 4 deletions tests/framework/cli/micropkg/test_micropkg_package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def assert_sdist_contents_correct(
f"{package_name}-{version}/{package_name}/README.md",
f"{package_name}-{version}/{package_name}/nodes.py",
f"{package_name}-{version}/{package_name}/pipeline.py",
f"{package_name}-{version}/{package_name}/config/parameters/{package_name}.yml",
f"{package_name}-{version}/{package_name}/config/parameters_{package_name}.yml",
f"{package_name}-{version}/tests/__init__.py",
f"{package_name}-{version}/tests/test_pipeline.py",
}
Expand Down Expand Up @@ -354,9 +354,9 @@ def test_package_modular_pipeline_with_nested_parameters(
assert (
"retail-0.1/retail/config/parameters/retail/params1.yml" in sdist_contents
)
assert "retail-0.1/retail/config/parameters/retail.yml" in sdist_contents
assert "retail-0.1/retail/config/parameters_retail.yml" in sdist_contents
assert (
"retail-0.1/retail/config/parameters/retail_banking.yml"
"retail-0.1/retail/config/parameters_retail_banking.yml"
not in sdist_contents
)

Expand Down Expand Up @@ -424,7 +424,7 @@ def test_package_pipeline_with_deep_nested_parameters(
"retail-0.1/retail/config/parameters/retail/deep/params1.yml"
in sdist_contents
)
assert "retail-0.1/retail/config/parameters/retail.yml" in sdist_contents
assert "retail-0.1/retail/config/parameters_retail.yml" in sdist_contents
assert "retail-0.1/retail/config/parameters/deep/retail.yml" in sdist_contents
assert (
"retail-0.1/retail/config/parameters/a/b/c/d/retail/params3.yml"
Expand Down
33 changes: 11 additions & 22 deletions tests/framework/cli/micropkg/test_micropkg_pull.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,7 @@ def test_pull_local_sdist(
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

self.assert_package_files_exist(source_dest)
Expand Down Expand Up @@ -160,8 +159,7 @@ def test_pull_local_sdist_compare(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)

sdist_file = (
Expand Down Expand Up @@ -189,8 +187,7 @@ def test_pull_local_sdist_compare(
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

assert not filecmp.dircmp(source_path, source_dest).diff_files
Expand Down Expand Up @@ -237,8 +234,7 @@ def test_micropkg_pull_same_alias_package_name(
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

self.assert_package_files_exist(source_dest)
Expand Down Expand Up @@ -287,8 +283,7 @@ def test_micropkg_pull_nested_destination(
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

self.assert_package_files_exist(source_dest)
Expand Down Expand Up @@ -451,8 +446,7 @@ def test_pull_tests_missing(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)
# Make sure the files actually deleted before pulling from the sdist file.
assert not source_path.exists()
Expand Down Expand Up @@ -480,8 +474,7 @@ def test_pull_tests_missing(
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

self.assert_package_files_exist(source_dest)
Expand Down Expand Up @@ -509,8 +502,7 @@ def test_pull_config_missing(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)
source_params_config.unlink()
call_micropkg_package(fake_project_cli, fake_metadata)
Expand Down Expand Up @@ -544,8 +536,7 @@ def test_pull_config_missing(
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

self.assert_package_files_exist(source_dest)
Expand Down Expand Up @@ -586,8 +577,7 @@ def test_pull_from_pypi(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)
# Make sure the files actually deleted before pulling from pypi.
assert not source_path.exists()
Expand Down Expand Up @@ -645,8 +635,7 @@ def get_all(self, name, failobj=None): # pylint: disable=unused-argument
fake_repo_path
/ settings.CONF_SOURCE
/ config_env
/ "parameters"
/ f"{pipeline_name}.yml"
/ f"parameters_{pipeline_name}.yml"
)

self.assert_package_files_exist(source_dest)
Expand Down
33 changes: 13 additions & 20 deletions tests/framework/cli/pipeline/test_pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import os
import shutil
from pathlib import Path

Expand All @@ -20,12 +19,12 @@
def make_pipelines(request, fake_repo_path, fake_package_path, mocker):
source_path = fake_package_path / "pipelines" / PIPELINE_NAME
tests_path = fake_repo_path / "src" / "tests" / "pipelines" / PIPELINE_NAME
conf_path = fake_repo_path / settings.CONF_SOURCE / request.param / "parameters"
conf_path = fake_repo_path / settings.CONF_SOURCE / request.param
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐️ nice spot, the previous test was confusing because it is not really a conf_path.


for path in (source_path, tests_path, conf_path):
path.mkdir(parents=True, exist_ok=True)

(conf_path / f"{PIPELINE_NAME}.yml").touch()
(conf_path / f"parameters_{PIPELINE_NAME}.yml").touch()
(tests_path / "test_pipe.py").touch()
(source_path / "pipe.py").touch()

Expand Down Expand Up @@ -67,8 +66,8 @@ def test_create_pipeline( # pylint: disable=too-many-locals
# config
conf_env = env or "base"
conf_dir = (fake_repo_path / settings.CONF_SOURCE / conf_env).resolve()
actual_configs = list(conf_dir.glob(f"**/{PIPELINE_NAME}.yml"))
expected_configs = [conf_dir / "parameters" / f"{PIPELINE_NAME}.yml"]
actual_configs = list(conf_dir.glob(f"**/*{PIPELINE_NAME}.yml"))
expected_configs = [conf_dir / f"parameters_{PIPELINE_NAME}.yml"]
assert actual_configs == expected_configs

# tests
Expand All @@ -92,7 +91,7 @@ def test_create_pipeline_skip_config(
assert f"Pipeline '{PIPELINE_NAME}' was successfully created." in result.output

conf_dirs = list((fake_repo_path / settings.CONF_SOURCE).rglob(PIPELINE_NAME))
assert conf_dirs == [] # no configs created for the pipeline
assert not conf_dirs # no configs created for the pipeline
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼


test_dir = fake_repo_path / "src" / "tests" / "pipelines" / PIPELINE_NAME
assert test_dir.is_dir()
Expand All @@ -117,13 +116,12 @@ def test_catalog_and_params( # pylint: disable=too-many-locals
"filepath": "data/01_raw/iris.csv",
}
}
catalog_file = conf_dir / "catalog" / f"{PIPELINE_NAME}.yml"
catalog_file.parent.mkdir()
catalog_file = conf_dir / f"catalog_{PIPELINE_NAME}.yml"
with catalog_file.open("w") as f:
yaml.dump(catalog_dict, f)

# write pipeline parameters
params_file = conf_dir / "parameters" / f"{PIPELINE_NAME}.yml"
params_file = conf_dir / f"parameters_{PIPELINE_NAME}.yml"
assert params_file.is_file()
params_dict = {"params_from_pipeline": {"p1": [1, 2, 3], "p2": None}}
with params_file.open("w") as f:
Expand All @@ -143,8 +141,7 @@ def test_skip_copy(self, fake_repo_path, fake_project_cli, fake_metadata):
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ dirname
/ f"{PIPELINE_NAME}.yml"
/ f"{dirname}_{PIPELINE_NAME}.yml"
)
path.parent.mkdir(exist_ok=True)
path.touch()
Expand All @@ -166,7 +163,7 @@ def test_skip_copy(self, fake_repo_path, fake_project_cli, fake_metadata):

assert result.exit_code == 0
assert "__init__.py': SKIPPED" in result.output
assert f"parameters{os.sep}{PIPELINE_NAME}.yml': SKIPPED" in result.output
assert f"parameters_{PIPELINE_NAME}.yml': SKIPPED" in result.output
assert result.output.count("SKIPPED") == 2 # only 2 files skipped

def test_failed_copy(
Expand Down Expand Up @@ -273,8 +270,7 @@ def test_delete_pipeline(
fake_repo_path
/ settings.CONF_SOURCE
/ expected_conf
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)

assert f"Deleting '{source_path}': OK" in result.output
Expand Down Expand Up @@ -309,8 +305,7 @@ def test_delete_pipeline_skip(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)

assert f"Deleting '{source_path}'" not in result.output
Expand Down Expand Up @@ -401,8 +396,7 @@ def test_pipeline_delete_confirmation(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)

assert "The following paths will be removed:" in result.output
Expand Down Expand Up @@ -442,8 +436,7 @@ def test_pipeline_delete_confirmation_skip(
fake_repo_path
/ settings.CONF_SOURCE
/ "base"
/ "parameters"
/ f"{PIPELINE_NAME}.yml"
/ f"parameters_{PIPELINE_NAME}.yml"
)

assert "The following paths will be removed:" in result.output
Expand Down