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

Validate node values #3715

Merged
merged 7 commits into from
Mar 18, 2024
Merged

Validate node values #3715

merged 7 commits into from
Mar 18, 2024

Conversation

ElenaKhaustova
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova commented Mar 14, 2024

Description

Close #2733

Development notes

Based on the assumptions from here it was decided to add extra validation at the Node level.

Now we check not only inputs/outputs (to the Node constructor) types - [String, List, Dict, None] but also types of names of variables used as inputs/outputs - [String, List[str], Dict[str, str], None].

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

@ElenaKhaustova ElenaKhaustova self-assigned this Mar 14, 2024
@@ -111,7 +111,7 @@ def dummy_node_empty_input():
return node(
func=dummy_function,
inputs=["", ""],
outputs=[None],
outputs=None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noklam is this a typo or expected? It breaks the validation added and contradicts the Node definition where we do not expect List[None] as an input:

class Node:
    """``Node`` is an auxiliary class facilitating the operations required to
    run user-provided functions as part of Kedro pipelines.
    """
    def __init__(
        self,
        func: Callable,
        inputs: str | list[str] | dict[str, str] | None,
        outputs: str | list[str] | dict[str, str] | None,
        *,
        name: str | None = None,
        tags: str | Iterable[str] | None = None,
        confirms: str | list[str] | None = None,
        namespace: str | None = None,
    ):

So I fixed it like this now and it works but should be discussed if this is an expected behaviour.

Copy link
Contributor

@noklam noklam Mar 14, 2024

Choose a reason for hiding this comment

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

@ElenaKhaustova #2408, If I remember, output doesn't support str but input does (which is the issue I link). They should be consistent, since you are looking at this already, maybe you have a quick answer for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ElenaKhaustova In this case I think changing to None is fine. It's "breaking" straightly but it's also a very small thing and easy to fix. I have never seen someone do [None] previously.

So after this validation [None] will not be accepted.

cc @merelcht

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @noklam!

As an alternative, in case we need to support [None], we at least need to update accepted types in the Node constructor.

Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
@ElenaKhaustova ElenaKhaustova marked this pull request as ready for review March 14, 2024 16:27
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Nice fix!

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.

Nice solution! 👍

@ElenaKhaustova ElenaKhaustova merged commit 32b87be into main Mar 18, 2024
41 checks passed
@ElenaKhaustova ElenaKhaustova deleted the feature/2733-validate-node-values branch March 18, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants