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

[feature] YAMLDataLoader and YAMLDataSaver as default data adapters #819

Merged
merged 5 commits into from
Apr 15, 2024

Conversation

wmoreiraa
Copy link
Contributor

@wmoreiraa wmoreiraa commented Apr 11, 2024

Adding the option to do I/O using YAML files, similar as what we have with json.

Changes

Adding YAML as default data adapter

How I tested this

Same test module as others default data adapters, 3 unit tests:

  1. Isolated load
  2. Isolated save
  3. Using both loader and saver.

Notes

Followed pattern of json, but expanded the options of applicable_types

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

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.

Looks good!

One request: we should move this to the plugins folder. We don't have yaml as a core dependency, and I don't want to add it.

So we should move this to a file like:

hamilton/plugins/yaml_extensions.py

At the top it should have some code like:

try:
    import yaml
except ImportError:
    raise NotImplementedError("yaml is not installed.")

Then at the bottom it should have some additional code like:

from hamilton import registry

COLUMN_FRIENDLY_DF_TYPE = False

def register_data_adapters():
    """Function to register the data loaders for this extension."""
    for loader in [
        YAMLDataLoader,
        YAMLDataSaver
    ]:
        registry.register_adapter(loader)


register_data_adapters()

and then in hamilton/function_modifiers/base.py add yaml to plugins_modules list so that we try to import it.

@wmoreiraa
Copy link
Contributor Author

thanks for the review @skrawcz and totally agree! Nice to learn how the plugins works too :)

@skrawcz skrawcz self-requested a review April 12, 2024 20:25
@wmoreiraa
Copy link
Contributor Author

Fixed back the PrimitiveType issue. @skrawcz lemme know if something still missing

@skrawcz skrawcz merged commit c1eeb50 into DAGWorks-Inc:main Apr 15, 2024
23 checks passed
@skrawcz
Copy link
Collaborator

skrawcz commented Apr 15, 2024

Thanks @wmoreiraa !

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.

2 participants