Fix for compatibility with Kedro 0.18.4 onwards#22
Fix for compatibility with Kedro 0.18.4 onwards#22Zubin Roy (Prospect-Zubin) wants to merge 1 commit into
Conversation
…megaConfigLoader which replaced the legacy ConfigLoader and TemplatedConfigLoader previously used. Updates also allow for backwards integration for versions of kedro <= 0.18.4. Updates also added user flexibility for choice of docker image platform architecture which allows for multi-architecture docker images. As a result of the codebase changes updates to the affected unit tests were made.
| import kedro | ||
| from kedro.framework.session import KedroSession | ||
| from packaging import version |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sure happy to make that change. What is the benefit of doing it that way rather than using packaging.version?
There was a problem hiding this comment.
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
Dmitry Sorokin (DimedS)
left a comment
Dmitry Sorokin (DimedS)
left a comment
There was a problem hiding this comment.
Thank you for the PR, your interest in the plugin, and the time you've spent improving it, Zubin Roy (@Prospect-Zubin)! I really like the idea of adding platform support and have asked a question about incorporating the OmegaConf Loader.
Additionally, I noticed that starting from Kedro version 0.19, a few other changes are needed to make kedro-sagemaker work properly. I plan to address those in a separate PR once we reach an agreement on this one.
|
|
||
| if current_version > required_version: | ||
| return KedroSageMakerPluginConfig.parse_obj( | ||
| self.context.config_loader["parameters"] |
There was a problem hiding this comment.
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.
| rv = subprocess.run( | ||
| [ | ||
| "docker", | ||
| "buildx", |
4 participants
Updates to allow integration with kedro 0.18.4 > onwards which uses OmegaConfigLoader which replaced the legacy ConfigLoader and TemplatedConfigLoader previously used. Updates also allow for backwards integration for versions of kedro <= 0.18.4.
Updates also added user flexibility for choice of docker image platform architecture which allows for multi-architecture docker images.
As a result of the codebase changes updates to the affected unit tests were made.
This is my first PR for a public codebase so any feedback or comments is appreciated :)