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

Add warning for catch-all patterns [dataset factories] #2774

Conversation

ankatiyar
Copy link
Contributor

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Fix #2760

Development notes

Since this is a small change, I thought I'll add it to the main Dataset Factories PR #2635.

Throw a warning at the first instance that the dataset is used (and added to the catalog)

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar ankatiyar requested a review from merelcht as a code owner July 6, 2023 11:08
@ankatiyar ankatiyar requested a review from noklam July 6, 2023 11:09
@@ -386,6 +386,13 @@ def _get_dataset(
self._load_versions.get(data_set_name),
self._save_version,
)
if self._specificity(matched_pattern) == 0:
self._logger.warning(
"The dataset '%s' is using the catch-all pattern '%s'",
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to provide a bit more info on what that means. Either a link to a section in the docs which explains what the catch-all pattern is, or some more text here explaining that it overrides the default MemoryDataset creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be Info or Warning log do you think?

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 And as discussed in person: let's do it as a warning for now so people become aware of this behaviour. We can always make it info if users find it too intrusive.

@ankatiyar ankatiyar merged commit bdc953d into feature/dataset-factories-for-catalog Jul 6, 2023
@ankatiyar ankatiyar deleted the feature/catch-all-pattern-warning branch July 6, 2023 14:16
ankatiyar added a commit that referenced this pull request Jul 6, 2023
* Cleaned up and up to date version of dataset factories code

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

* Add some simple tests

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

* Add parsing rules

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Refactor

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Add some tests

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Add unit tests

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Fix test + refactor runner

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Add comments + update specificity fn

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Update function names

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Update test

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Release notes + update resume scenario fix

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* revert change to suggest resume scenario

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Update tests DataSet->Dataset

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Small refactor + move parsing rules to a new fn

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Fix problem with load_version + refactor

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* linting + small fix _get_datasets

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Remove check for existence

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Add updated tests + Release notes

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* change classmethod to staticmethod for _match_patterns

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Add test for layer

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Minor change from code review

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Remove type conversion

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Add warning for catch-all patterns [dataset factories] (#2774)

* Add warning for catch-all patterns

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

* Update warning message

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

---------

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>

---------

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Co-authored-by: Merel Theisen <merel.theisen@quantumblack.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants