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

Add the viz add-ons to kedro new #3228

Merged
merged 43 commits into from
Nov 3, 2023
Merged

Add the viz add-ons to kedro new #3228

merged 43 commits into from
Nov 3, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Oct 25, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Fix #3074 and #3075

Development notes

@SajidAlamQB Notes:

  • Refactored utils.py and unstripped pyproject.toml and requirements.txt. We now handle removing headers from pyproject.toml using the toml library.

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

@noklam noklam self-assigned this Oct 25, 2023
@noklam noklam changed the base branch from main to develop October 25, 2023 14:15
@SajidAlamQB SajidAlamQB self-assigned this Oct 31, 2023
@noklam
Copy link
Contributor Author

noklam commented Nov 1, 2023

The current test suite is still asserting text, we should read the parse the file. I think this should be merged first #3230. There are not much points to fix the broken test if we are gonna change that.

noklam and others added 3 commits November 1, 2023 11:31
Signed-off-by: Nok <[email protected]>
Signed-off-by: Nok Lam Chan <[email protected]>
Signed-off-by: SajidAlamQB <[email protected]>
Signed-off-by: SajidAlamQB <[email protected]>
@merelcht
Copy link
Member

merelcht commented Nov 1, 2023

@SajidAlamQB
Copy link
Contributor

Should https://github.com/kedro-org/kedro-starters/blob/main/spaceflights-pyspark/hooks/post_gen_project.py be updated to also mention "Kedro Viz" not in addons? The opposite of https://github.com/kedro-org/kedro-starters/blob/main/spaceflights-pandas-viz/hooks/post_gen_project.py

Nice spot! I don't think it really matters as the templates are decided in starters.py so where the add-ons work is concerned its not a problem. This was more of guard for users using the --starter flags as the default add-on in starters is none so as long as we have a check for any other add-on it won't run. But for consistency I think it would be nice to make them all aligned - it’s cleaner that way!

tests/framework/cli/test_starters.py Outdated Show resolved Hide resolved
kedro/templates/project/hooks/utils.py Outdated Show resolved Hide resolved
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.

I've tried this out and the behaviour looks good! I found a coupling between the existence of the data folder and the SQLiteSessionStore config, but let's decide on that via Slack.

The only thing that's missing is just the docstring for the helper methods, but happy to approve when that's done 👍

@noklam
Copy link
Contributor Author

noklam commented Nov 3, 2023

I have fix the tests with toml and add some docstring for the new utils question. I leave a question whether these should be all public methods and maybe worth refactor later but not necessary to fix this in this PR.

Documented my observation in #3264

noklam and others added 3 commits November 3, 2023 03:05
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.

LGTM! Great work @SajidAlamQB and @noklam ⭐ ⭐ ⭐

kedro/templates/project/hooks/utils.py Outdated Show resolved Hide resolved
Signed-off-by: SajidAlamQB <[email protected]>
Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB left a comment

Choose a reason for hiding this comment

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

All looks good to me! Good work refactoring the tests 🌟

"4": 2,
"5": 8,
"6": 3,
"1": 0, # Linting does not add any files
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart choice adding the comments!

@noklam noklam enabled auto-merge (squash) November 3, 2023 17:29
@noklam noklam merged commit 93dc1a9 into develop Nov 3, 2023
28 of 29 checks passed
@noklam noklam deleted the noklam/viz-add-on branch November 3, 2023 17:52
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.

Add kedro-viz to add-ons options
4 participants