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.

Sign up for GitHub

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

Jump to bottom

Refactor Flowchart models from dataclass to pydantic base models #1565

Merged
merged 41 commits into from
Nov 14, 2023
Merged

Refactor Flowchart models from dataclass to pydantic base models #1565

merged 41 commits into from
Nov 14, 2023

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Oct 9, 2023 •
edited
Loading

Description

Partially Resolves #1538

Development notes

  • Replaced all data classes from models/flowchart.py with pydantic models
  • Used root-validators and field validators
  • Modified pytests

QA notes

  • All features in Kedro Viz should run normally.
  • Start the app either using kedro viz command from demo-project or run the dev server using make run
  • Run pytests using make pytest

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

@ravi-kumar-pilla
create pydantic flowchart classes
a2636f3 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fixing pydantic class conversions
240a383 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
minor modifications and code comments
772ed02 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix pytests for flowchart and managers
900fb70 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix pytests
57811a7 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix lint errors and pytests
182be16 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz into ch…
62efd72 
…ore/refactor-data-classes

Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
Merge branch 'main' of https://github.com/kedro-org/kedro-viz into ch…
47cc4d7 
…ore/refactor-data-classes

Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix lint issues and merge main
d82f71d 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix initialization issues and lint issues
fe67d26 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix push issue
47d3211 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review October 11, 2023 23:19
merelcht
merelcht reviewed Oct 16, 2023
View reviewed changes
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.

Sorry for the delay in review @ravi-kumar-pilla. I don't know too much about pydantic, but generally these changes look fine to me. I have a couple of questions.

package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
merelcht
merelcht approved these changes Oct 17, 2023
View reviewed changes
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! 👍 Might be good to also get @noklam or @astrojuanlu to look at this. I think they know more about pydantic than I do 🙂

noklam
noklam reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Have the 1st review.

  1. There are changes that are not 1:1 between Pydantic and dataclass. It's less restrictively i.e. frozen class is now mutable, non-initialisable attributes are now initialisable. It probably works the same for kedro-viz but it's better to keep it strict to reduce the chance that we fall into our own traps.
  2. If re-ordering is needed, I suggest to make it in separate PR since it's quite hard to review when things get shuffled.

package/kedro_viz/models/flowchart.py Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
@noklam
Copy link
Contributor

noklam commented Nov 7, 2023

Can you update the Issue title to reflect the current status since the scope have changed and most discussion are obsoleted.

@ravi-kumar-pilla ravi-kumar-pilla changed the title Chore/refactor data classes Refactor Flowchart models from dataclass to pydantic base models Nov 7, 2023
noklam
noklam reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Left some comments, most are minor.

package/kedro_viz/models/flowchart.py Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
package/kedro_viz/models/flowchart.py Outdated Show resolved Hide resolved
@ravi-kumar-pilla
Merge branch 'main' into chore/refactor-data-classes
d7e2653
@ravi-kumar-pilla
address PR comments1
74b2930 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
Merge branch 'chore/refactor-data-classes' of https://github.com/kedr…
b0e34ef 
…o-org/kedro-viz into chore/refactor-data-classes

Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix lint issues
ca6b16f 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
add hash for pylint
2970e9e 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
merge main
50c9d08 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix lint errors
80848d5 
Signed-off-by: ravi-kumar-pilla <[email protected]>
@ravi-kumar-pilla
fix lint errors
5cc645a 
Signed-off-by: ravi-kumar-pilla <[email protected]>
noklam
noklam approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Approve as I don't want to block the PR, there are minor clean up the unnecessary hash function.

@ravi-kumar-pilla
Copy link
Contributor Author

Approve as I don't want to block the PR, there are minor clean up the unnecessary hash function.

fyi, I removed the hash function

@ravi-kumar-pilla ravi-kumar-pilla merged commit 2eb9c0a into main Nov 14, 2023
19 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the chore/refactor-data-classes branch November 14, 2023 18:59
@rashidakanchwala rashidakanchwala mentioned this pull request Nov 16, 2023
Merged
5 tasks
rashidakanchwala added a commit that referenced this pull request Nov 17, 2023
@rashidakanchwala
Release v6.7.0 (#1642)
9189973 
This is minor release with big backend refactoring work and some bug fixes.

Bug fixes and other changes
Refactor flowchart dataclasses to pydantic base models. (Refactor Flowchart models from dataclass to pydantic base models #1565)
Fix dataset factory patterns in Experiment Tracking. (Fix dataset factory patterns in Experiment Tracking #1588)
Update demo-project to use OmegaConfigLoader. (Update demo project to use OmegaConfigLoader #1590)
Improve feedback for copy to clipboard feature. (Add tooltip to shareable urls copy button #1614)
Ensure Kedro-Viz works when hosted on a URL subpath. (Fix: Kedro-Viz doesn't work when hosted via a URL subpath #1621)
Bump fastapi upper bounds. (Bump FAST API upper bounds #1634)
Fix shareable URL modal to appear across the app. (Make shareable URL modal open globally across the app.  #1639)
Add Kedro-Viz CLI command deprecation warning. (Add kedro viz deprecation warning for CLI #1641)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonymilne antonymilne antonymilne left review comments

@merelcht merelcht merelcht approved these changes

@rashidakanchwala rashidakanchwala rashidakanchwala approved these changes

@noklam noklam noklam approved these changes

@tynandebold tynandebold Awaiting requested review from tynandebold

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Spike] Clarifying Overlap: Pydantic Models vs. Data Classes in Backend
6 participants
@ravi-kumar-pilla @noklam @rashidakanchwala @antonymilne @merelcht @tynandebold