Remove unneeded filepath check on dataset factories #3561
Conversation
MosaicMan
force-pushed
the
cli_catalog_resolve_patterns
branch
5 times, most recently
from
d7a4d15
to
1de3086
Compare
Signed-off-by: MosaicMan <34198823+MosaicMan@users.noreply.github.com>
MosaicMan
force-pushed
the
cli_catalog_resolve_patterns
branch
from
1de3086
to
bf741b7
Compare
kedro/framework/cli/catalog.py
Outdated
| path_keys_to_check = ["filepath", "path"] | ||
|
|
||
| for key in path_keys_to_check: | ||
| if key in ds_config: | ||
| ds_config[key] = _trim_filepath( | ||
| str(context.project_path) + "/", ds_config[key] | ||
| ) | ||
|
|
There was a problem hiding this comment.
@AhdraMeraliQB @merelcht Could we get rid of this whole check instead of hard-coding this?
I see this was a design decision (#2891 (comment))
I think this was before the code went through a few changes. Even for explicit datasets, we just get the config as it is written in the catalog using the config loader so relative path instead of the absolute path so everything is uniform even without this check.
There was a problem hiding this comment.
Works for me - tried removing this check locally and it all the tests still pass.
There was a problem hiding this comment.
Ah good point @ankatiyar. If everything still works as expected and the problem with PartitionedDataset is solved when the check is removed, then that definitely seems like the best solution.
There was a problem hiding this comment.
in that case, @MosaicMan could you get rid of these lines entirely and the _trim_filepath() method below? And add this change to the release notes as well! :)
There was a problem hiding this comment.
Absolutely. No problem.
- Remove filepath check from `resolve_patterns` method. - Eliminate the associated `_trim_filepath` function. - Update release notes. These changes address redundant validations that were causing kedro-org#3560. Signed-off-by: MosaicMan <34198823+MosaicMan@users.noreply.github.com>
Signed-off-by: Ankita Katiyar <110245118+ankatiyar@users.noreply.github.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
astrojuanlu
changed the title
Signed off by MosaicMan 34198823+MosaicMan@users.noreply.github.com
Description
Addressing bug as described in #3560. Ran test suite as well as linter. Test status unchanged.
Development notes
Tweaked the path normalization logic in the resolve_patterns method of the framework.cli.catalog module
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-byline 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
RELEASE.mdfile