Skip to content

Fix for compatibility with Kedro 0.18.4 onwards #22

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

Open
Zubin Roy (Prospect-Zubin) wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix for compatibility with Kedro 0.18.4 onwards #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion kedro_sagemaker/cli_functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def docker_autobuild(auto_build, click_context, image, mgr, yes):
if not yes and not click.confirm("Continue?", default=True):
click_context.exit(1)

if (rv := docker_build(str(mgr.context.project_path), image)) != 0:
if (rv := docker_build(str(mgr.context.project_path), image, str(mgr.plugin_config.docker.platforms))) != 0:
click_context.exit(rv)
if (rv := docker_push(image)) != 0:
click_context.exit(rv)
Expand Down
2 changes: 2 additions & 0 deletions kedro_sagemaker/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class SageMakerConfig(BaseModel):
class DockerConfig(BaseModel):
image: str
working_directory: str = "/home/kedro"
platforms: str


class AwsConfig(BaseModel):
Expand Down Expand Up @@ -66,6 +67,7 @@ class KedroSageMakerPluginConfig(BaseModel):
docker:
image: "{docker_image}"
working_directory: /home/kedro
platforms: "linux/amd64"
""".strip()

# This auto-validates the template above during import
Expand Down
23 changes: 19 additions & 4 deletions kedro_sagemaker/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
from functools import cached_property
from typing import Any, List, MutableMapping, Optional, Union

import kedro
from kedro.framework.session import KedroSession
from packaging import version
Comment on lines +11 to +13

@astrojuanlu Juan Luis Cano Rodríguez (astrojuanlu) Jan 7, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You might want to use importlib.metadata instead of relying on packaging.version and kedro.__version__ https://docs.python.org/3.9/library/importlib.metadata.html#metadata

@Prospect-Zubin Zubin Roy (Prospect-Zubin) Jan 8, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure happy to make that change. What is the benefit of doing it that way rather than using packaging.version?

@astrojuanlu Juan Luis Cano Rodríguez (astrojuanlu) Jan 8, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using packaging.version might be unavoidable if you want to parse it. The key is how to obtain the currently installed version, and importlib.metadata is the canonical way I'd say. See pallets/flask#5230


from kedro_sagemaker.config import KedroSageMakerPluginConfig
from kedro_sagemaker.constants import (
Expand Down Expand Up @@ -67,9 +69,18 @@ def __init__(

@cached_property
def plugin_config(self) -> KedroSageMakerPluginConfig:
return KedroSageMakerPluginConfig.parse_obj(
self.context.config_loader.get("sagemaker*")
)
# from this version onwards (not inclusive) config_loader uses OmegaConfigLoader which requires a different syntax
required_version = version.parse("0.18.4")
current_version = version.parse(kedro.__version__)

if current_version > required_version:
return KedroSageMakerPluginConfig.parse_obj(
self.context.config_loader["parameters"]

@DimedS Dmitry Sorokin (DimedS) Jan 14, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you please clarify how this should work? If I understand correctly, the SageMaker configuration is stored in the sagemaker.yml file, and it will not be loaded into the parameters section by OmegaConf because only the following patterns are configured to be read:

self.config_patterns = {
    "catalog": ["catalog*", "catalog*/**", "**/catalog*"],
    "parameters": ["parameters*", "parameters*/**", "**/parameters*"],
    "credentials": ["credentials*", "credentials*/**", "**/credentials*"],
    "globals": ["globals.yml"],
}

Even if it were loaded along with other parameters, Pydantic would raise a validation error during the parsing process.

)
else:
return KedroSageMakerPluginConfig.parse_obj(
self.context.config_loader.get("parameters*")
)

@cached_property
def context(self):
Expand Down Expand Up @@ -147,14 +158,18 @@ def nested_defaultdict():
return params


def docker_build(path: str, image: str) -> int:
def docker_build(path: str, image: str, platforms: str) -> int:
rv = subprocess.run(
[
"docker",
"buildx",

@Lasica Lasica Jan 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buildx build?

"build",
"--platform",
Comment thread
Lasica marked this conversation as resolved.
platforms,
path,
"-t",
image,
"--push"
],
stdout=sys.stdout,
stderr=subprocess.STDOUT,
Expand Down
2 changes: 1 addition & 1 deletion tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def test_docker_build(exit_code):
with patch(
"subprocess.run", return_value=Mock(returncode=exit_code)
) as subprocess_run:
result = docker_build(".", "my_image:latest")
result = docker_build(".", "my_image:latest","platforms")
assert exit_code == result, "Invalid exit code"
subprocess_run.assert_called_once()

Expand Down