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

Replace a StopIteration exception with for-else #3273

Merged
merged 4 commits into from
Nov 6, 2023
Merged

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Nov 6, 2023

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

I have read the logic how Kedro import datasets many time, I finally realise this can be simplified with for-else. It also reveals a hidden bug.

kedro/kedro/io/core.py

Lines 380 to 396 in 38dfe6f

class_obj = config.pop("type")
if isinstance(class_obj, str):
if len(class_obj.strip(".")) != len(class_obj):
raise DataSetError(
"'type' class path does not support relative "
"paths or paths ending with a dot."
)
class_paths = (prefix + class_obj for prefix in _DEFAULT_PACKAGES)
trials = (_load_obj(class_path) for class_path in class_paths)
try:
class_obj = next(obj for obj in trials if obj is not None)
except StopIteration as exc:
raise DataSetError(
f"Class '{class_obj}' not found or one of its dependencies "
f"has not been installed."
) from exc

class_obj can be None, adding str to None result in type error but this is hidden by the load_obj

prefix + class_obj

This is only a few lines of change but take me a while to figure it out, the error was complicated due to many layers of try-except. this will help us to fix #2943 and improve kedro.io.core

Development notes

I refactor some logic in a separate PR, the class_obj was overused and make it very hard to debug.

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-by line 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

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@noklam noklam self-assigned this Nov 6, 2023
@noklam noklam added this to the Redesign Catalog and Datasets milestone Nov 6, 2023
@noklam noklam marked this pull request as ready for review November 6, 2023 11:03
@noklam noklam requested a review from merelcht as a code owner November 6, 2023 11:03
Copy link
Member

@idanov idanov left a comment

Choose a reason for hiding this comment

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

Loving it ❤️

@astrojuanlu
Copy link
Member

No comment on the implementation (deferring to other engineers), just wanted to remind that this is not a fix to #2943. Just tested it and the error messages are the same and the import errors still get shadowed.

@astrojuanlu astrojuanlu removed their request for review November 6, 2023 11:14
@merelcht
Copy link
Member

merelcht commented Nov 6, 2023

No comment on the implementation (deferring to other engineers), just wanted to remind that this is not a fix to #2943. Just tested it and the error messages are the same and the import errors still get shadowed.

@astrojuanlu It says in the PR description "this will help us to fix #2943 and improve kedro.io.core" so it's not meant as a complete fix.

@astrojuanlu
Copy link
Member

I know, but GitHub will automatically close it, so I wrote the comment for future reference 😅

@datajoely
Copy link
Contributor

so elegant and now I actually have an example when this comes up!

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.

I can imagine this took a while to figure out! Nice improvement ⭐

@noklam noklam merged commit 0e4f79e into main Nov 6, 2023
@noklam noklam deleted the noklam-patch-1 branch November 6, 2023 13:33
noklam added a commit that referenced this pull request Nov 7, 2023
* Replace a `StopIteration` exception with for-else 

Signed-off-by: Nok Lam Chan <[email protected]>

* fix the logic

Signed-off-by: Nok <[email protected]>

* bug fix

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>
deepyaman added a commit that referenced this pull request Nov 8, 2023
* Configure `ruff format`, use it instead of `black` (#3258)

* build: format code with `ruff format` over `black`

Signed-off-by: Deepyaman Datta <[email protected]>

* style(ruff-format): format code with `ruff format`

Signed-off-by: Deepyaman Datta <[email protected]>

---------

Signed-off-by: Deepyaman Datta <[email protected]>

* docs: fix typos (#3270)

Signed-off-by: Ajay Gonepuri <[email protected]>
Co-authored-by: Jo Stichbury <[email protected]>

* Update create_a_pipeline.md (#3213)

* Update create_a_pipeline.md

typo: 
--nodes=preprocess_shuttles_node needs to be specified

Signed-off-by: Pranav Dave <[email protected]>

* Use both node names

Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>

* Update create_a_pipeline.md

Signed-off-by: Jo Stichbury <[email protected]>

---------

Signed-off-by: Pranav Dave <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Jo Stichbury <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Jo Stichbury <[email protected]>

* Replace a `StopIteration` exception with for-else (#3273)

* Replace a `StopIteration` exception with for-else 

Signed-off-by: Nok Lam Chan <[email protected]>

* fix the logic

Signed-off-by: Nok <[email protected]>

* bug fix

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>

* Make `ruff format` happy

Signed-off-by: Deepyaman Datta <[email protected]>

* Make `ruff` happy

Signed-off-by: Deepyaman Datta <[email protected]>

---------

Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Ajay Gonepuri <[email protected]>
Signed-off-by: Pranav Dave <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Jo Stichbury <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]>
Co-authored-by: Ajay Gonepuri (GONAPCORP) <[email protected]>
Co-authored-by: Jo Stichbury <[email protected]>
Co-authored-by: Pranav Dave <[email protected]>
Co-authored-by: Juan Luis Cano Rodríguez <[email protected]>
Co-authored-by: Nok Lam Chan <[email protected]>
noklam added a commit that referenced this pull request Nov 9, 2023
…encies is missing (#3272)

* release note

Signed-off-by: Nok Chan <[email protected]>

* Refactor - split `class_obj` into `class_obj` and `dataset_type`

Signed-off-by: Nok Chan <[email protected]>

* bug fix

Signed-off-by: Nok Chan <[email protected]>

* Replace a `StopIteration` exception with for-else (#3273)

* Replace a `StopIteration` exception with for-else 

Signed-off-by: Nok Lam Chan <[email protected]>

* fix the logic

Signed-off-by: Nok <[email protected]>

* bug fix

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>

* If module exists, raise error even it is ModuleNotFoundError

Signed-off-by: Nok Chan <[email protected]>

* simplify load_obj

Signed-off-by: Nok Chan <[email protected]>

* minor bug fix, check if class exist

Signed-off-by: Nok Chan <[email protected]>

* remove obsolete test

Signed-off-by: Nok Chan <[email protected]>

* object is a bad type hint

Signed-off-by: Nok Chan <[email protected]>

* update error message link to installation guide

Signed-off-by: Nok Chan <[email protected]>

* add tests

Signed-off-by: Nok Chan <[email protected]>

* Change error message

Signed-off-by: Nok Chan <[email protected]>

* Update docstring

Signed-off-by: Nok Chan <[email protected]>

* fix lint

Signed-off-by: Nok Chan <[email protected]>

---------

Signed-off-by: Nok Chan <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>
JaynouOliver pushed a commit to JaynouOliver/kedro that referenced this pull request Nov 10, 2023
* Replace a `StopIteration` exception with for-else

Signed-off-by: Nok Lam Chan <[email protected]>

* fix the logic

Signed-off-by: Nok <[email protected]>

* bug fix

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: Nok <[email protected]>
Signed-off-by: Suvrakamal Das <[email protected]>
@merelcht merelcht removed this from the Redesign the API for IO (catalog) milestone Feb 27, 2024
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.

Make import failures in kedro-datasets clearer
5 participants