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

Fix kedro viz --lite compatibility with Kedro 18 #2102

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

rashidakanchwala
Copy link
Contributor

@rashidakanchwala rashidakanchwala commented Sep 19, 2024

Description

This is to get Kedro-viz (mainly kedro viz --lite) to work with Kedro 18 and some Kedro 19 versions based on changes this PR (kedro-org/kedro#3920) introduced.

In July, Kedro made the _save and _load methods public. At that time, Kedro-viz did not rely on these methods. However, when we recently merged kedro viz --lite, we introduced an UnavailableDataset class, which is an AbstractDataset. This class now uses the public load and save methods. To maintain backward compatibility with older versions of the dataset, we followed a suggestion made by @deepyaman

class MyDataset(...):
    def _load(...) -> ...:
        ...

    load = _load

    def _save(...) -> ...:
        ...

    save = _save

Originally posted by @deepyaman in kedro-org/kedro#3920 (comment)

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

LGTM thanks @rashidakanchwala 👍

@jitu5 jitu5 self-requested a review September 19, 2024 09:41
@astrojuanlu
Copy link
Member

Thanks @rashidakanchwala !

My only question (not a blocker) - any clue why this wasn't caught by unit tests?

@rashidakanchwala
Copy link
Contributor Author

rashidakanchwala commented Sep 19, 2024

excellent question and I am going to create an issue around it today. Ideally; this should be caught by e2e testing which we don't have any for kedro viz --lite so I am going to ping Ravi to work on that.

Update - #2103

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

👍🏼 !

@rashidakanchwala rashidakanchwala merged commit 359fdd0 into main Sep 19, 2024
46 checks passed
@rashidakanchwala rashidakanchwala deleted the fix-lite-compatibility-issue-with-kedro18 branch September 19, 2024 11:24
@ravi-kumar-pilla
Copy link
Contributor

excellent question and I am going to create an issue around it today. Ideally; this should be caught by e2e testing which we don't have any for kedro viz --lite so I am going to ping Ravi to work on that.

Update - #2103

Thanks @rashidakanchwala for fixing the issue and creating the ticket. I did add one e2e test only against the "latest" Kedro version. I will add more in the ticket #2103 . Thank you !

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.

6 participants