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

plugins Kedro #916

Merged
merged 8 commits into from
May 23, 2024
Merged

plugins Kedro #916

merged 8 commits into from
May 23, 2024

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented May 22, 2024

Added a plugin to convert Kedro Pipeline objects to Hamilton Driver. This allows Kedro users to benefit from the ergonomics of Hamilton and it's powerful Hamilton UI observability features.

Changes

  • added hamilton/plugins/h_kedro.py with the Pipeline to Driver conversion
  • added hamilton/examples/kedro/kedro-plugin/ with a README and a tutorial notebook

How I tested this

  • the test coverage is low, but we manually made sure pipelines could run and successfully be captured by the Hamilton UI. Issues wouldn't be surprising as Kedro users try it with less common usage patterns.
  • added tests/plugins/test_h_kedro.py. It focuses on the supporting Kedro nodes that return multiple values while Hamilton nodes return a single value (and may use @extract_fields).

@zilto zilto added enhancement New feature or request examples Related to code under `examples/` labels May 22, 2024
Copy link
Contributor

sweep-ai bot commented May 22, 2024

Sweep: PR Review

examples/kedro/README.md

Updated the description of hamilton-code/ to use "Hamilton library" and added a new entry for kedro-plugin/ to the content list.


examples/kedro/kedro-code/src/kedro_code/pipelines/data_science/nodes.py

Added a return type annotation -> None to the evaluate_model function to explicitly indicate it does not return any value.


hamilton/function_modifiers/base.py

Added "kedro" to the list of plugin modules to be loaded by the registry.


requirements-test.txt

Added the kedro package to the list of dependencies in requirements-test.txt.


@zilto
Copy link
Collaborator Author

zilto commented May 22, 2024

For Python <3.11, @extract_fields had a bug. This is fixed in 387cdc2.

Essentially, the condition isinstance(Any, type) returns False in Python < 3.11. Here's a repro of the bug on Python 3.10

from typing import Any
from hamilton.function_modifiers import extract_fields

@extract_fields(dict(B=Any, C=Any))
def A() -> dict:
    return dict(B=True, C=1)
    
if __name__ == "__main__":
    import __main__
    from hamilton import driver
    
    dr = driver.Builder().with_modules(__main__).build()
    res = dr.execute(["B", "C"])
Traceback (most recent call last):
  File "/home/tjean/projects/dagworks/hamilton/bug.py", line 6, in <module>
    @extract_fields(dict(
  File "/home/tjean/projects/dagworks/hamilton/hamilton/function_modifiers/expanders.py", line 748, in __init__
    _validate_extract_fields(fields)
  File "/home/tjean/projects/dagworks/hamilton/hamilton/function_modifiers/expanders.py", line 728, in _validate_extract_fields
    raise base.InvalidDecoratorException(
hamilton.function_modifiers.base.InvalidDecoratorException: Error, found these ['B does not declare a type. Instead it passes typing.Any.', 'C does not declare a type. Instead it passes typing.Any.']. Please pass in a dict of string to types. 

I previously discussed the issue with @elijahbenizzy in the context of this plugin, but I'm now fixing it given the bug it broader than the plugin itself

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

I think this looks good. I haven't tested it but from the demo you showed it works. I would squash commit and write up any design decisions in the commit.

@zilto zilto merged commit 96da310 into main May 23, 2024
23 checks passed
@zilto zilto deleted the plugins/kedro branch May 23, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request examples Related to code under `examples/`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants