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

kedro-datasets: Rename MatplotlibWriter to MatplotlibDataset #353

Open
Galileo-Galilei opened this issue Sep 28, 2023 · 10 comments
Open

kedro-datasets: Rename MatplotlibWriter to MatplotlibDataset #353

Galileo-Galilei opened this issue Sep 28, 2023 · 10 comments

Comments

@Galileo-Galilei
Copy link
Member

Description

MatplotlibWriter has been the only dataset with inconsistent naming for a while. There are a couple of more subtle issues with it still to be fixed (see
#529), but I think we should use the dataset renaming (which is a breaking change whatever) as an opportunity to make it consistent with the rest of the codebase

Context

Consistency is better :)

Possible Implementation

Rename MatplotlibWriter to MatplotlibDataset.

Possible Alternatives

Don't do it ;)

This is likely a great ticket for Hacktoberfest if the project is eligible?

@noklam noklam added the good first issue Good for newcomers label Sep 28, 2023
@noklam
Copy link
Contributor

noklam commented Sep 28, 2023

Nice idea, I asked in the team and start tagging issues.

@astrojuanlu
Copy link
Member

  1. This affects Kedro-Viz, since they use MatplotlibWriter in various places. To retain backwards compatibility, they would probably have to do some try ... except with imports. cc @rashidakanchwala @tynandebold
  2. I think this goes to some of our findings in Easier CustomDataset Creation kedro#1936: that we don't really intend to implement _load for certain datasets (including this one) because they're meant to be artifacts and not really full I/O components.

Regardless, for consistency I agree we should rename it 👍🏽

@noklam
Copy link
Contributor

noklam commented Sep 29, 2023

https://linen-slack.kedro.org/t/15875966/i-have-a-plot-saved-as-an-image-in-my-catalog-yml-and-the-fi#6d98204f-5d9f-497b-88df-80a23980dcfa

An user ask why load is not supported? Maybe this is the reason why it is called Writer but not Dataset. I don't use this dataset myself so I can't remember is it always like this.

@astrojuanlu
Copy link
Member

Maybe we could have a MatplotlibFigureDataset (that saves and loads figures objects as pickles) (although maybe we shouldn't and we should tell users to use PickleDataset for this) and MatplotlibImageDataset (that only saves images and has no _load)

@Galileo-Galilei
Copy link
Member Author

Actually it totally makes sense that some dataset don't have _save method (API Dataset didn't for a while, SQLQueryDataset still doesn't...). Can I start a PR for this or the impact is too high on kedro-viz?

@astrojuanlu
Copy link
Member

Yeah I don't think it's a problem that some datasets don't have _save. What's problematic is the monkeypatching that kedro-viz does on some of them kedro-org/kedro-viz#1352

About the impact on Kedro Viz, cc @rashidakanchwala @tynandebold

@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Nov 7, 2023

Yes, it will. We will be looking at making sure Kedro-viz is not so coupled with Kedro-datasets. And we are going to prioritise this work in the second half of November most likely. Until then, we could pin kedro-viz to kedro-datasets to the version it would work with.

@astrojuanlu
Copy link
Member

I don't think this is a good first issue, there's some uncertainty still #353 (comment) removing the label, when we have more clarity we'll update this issue

@astrojuanlu astrojuanlu added datasets and removed good first issue Good for newcomers labels Jan 30, 2024
@merelcht
Copy link
Member

Kedro-Viz has made significant changes to decouple from kedro-dataset, would renaming MatplotlibWriter still be an issue @rashidakanchwala ?

@rashidakanchwala
Copy link
Contributor

nope this is not an issue. Kedro-viz only reads NewType = 'image' from preview method so it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

5 participants