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

This PR add modular-spaceflights repository as a demo-project within Kedro-Viz #696

Merged
merged 4 commits into from
Jan 14, 2022

Conversation

limdauto
Copy link
Collaborator

@limdauto limdauto commented Jan 14, 2022

Description

As discussed, this would make it easier to:

  • Build CI for demo & preview branches
  • Iterate quicker on features that require support from a real project
  • Easier dev env setup

Development notes

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

@datajoely
Copy link
Contributor

not sure if it matters but the images in the readmes will be broken since they point to the .tours directory which wasn't inlcuded

@limdauto
Copy link
Collaborator Author

@datajoely oh yea good point. Let me fix it. I excluded the .tours because I don't think it's necessary for Kedro-Viz purpose. It's really nice for the starter though.

@tynandebold
Copy link
Member

Does this do anything other than add in the demo-project/ files?

@limdauto
Copy link
Collaborator Author

@tynandebold it doesn't do anything yet. Just add the demo project file. More followup PRs coming.

Copy link
Contributor

@antonymilne antonymilne 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 just tried this out and seems to run end to end but I'm a bit confused about the requirements files:

  • What is demo_requirements.txt for? It gives kedro==0.17.5, which won't actually work for this project that uses sum(pipelines) - we need kedro==0.17.6 as in requirements.in.
  • Should we remove kedro-telemetry from the requirements?
  • Should we remove kedro-viz from the requirements? So that it doesn't install off PyPI when we want to use the local kedro-viz

Also, do we want some mock data in there as well? I think @studioswong had some here, but we can always add it on later. We'll need to change the gitignore and dockerignore accordingly.

@limdauto
Copy link
Collaborator Author

@AntonyMilneQB Ah, the demo_requirements.txt is used for deployment to demo.kedro.org. It's a trimmed down version of requirements.in so the container is smaller. Let me fix that. I will change that to prod_requirements.txt too and add a comment.

Good point about kedro-viz and kedro-telemetry; I will remove them. However, I think since this is will be used through running server.py, not kedro viz, we are probably okay eihter way.

Regarding test data, I thought we would want it on a shared, neutral environment like a gitpod? If not, I can include the data I have locally which is what we currently see on demo.kedro.org

@limdauto
Copy link
Collaborator Author

@AntonyMilneQB I have updated it with the test data and remove kedro-viz as well as kedro-telemetry as dependency.

@studioswong
Copy link
Contributor

@limdauto think it might be good to also add in a note in either the readme or the contributing docs about this new folder and demo project setup

@limdauto
Copy link
Collaborator Author

@studioswong yep as mentioned I will follow up with another PR. I think it would be good to review that separately. I just want to commit the code as-is in this PR so we don't have to verify anything else.

@tynandebold tynandebold self-requested a review January 14, 2022 16:33
@limdauto limdauto merged commit b173364 into main Jan 14, 2022
@limdauto limdauto deleted the feat/demo-project branch January 14, 2022 16:41
Copy link
Contributor

@studioswong studioswong left a comment

Choose a reason for hiding this comment

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

Thanks for setting this up!

One thing - it would be nice to name the demo project with a more specfic name such as demo-modular-spaceflights so it's easier to reference that to the modular spaceflight project.. Maybe one for your next PR

@limdauto
Copy link
Collaborator Author

@studioswong interesting. I actually specifically changed it from modular-spaceflights to demo-project to make it a little less specific since we might deviate from that one and to make it clear that this is used to deploy demo.kedro.org. However, happy to change it in the next PR.

@tynandebold
Copy link
Member

My 2¢: I like the ambiguity of demo-project as it doesn't attach us to spaceflights or animals or randomData or the like. I gotta think we won't always be using spaceflights, especially when we build out more robust, real-world demos.

@antonymilne
Copy link
Contributor

Agreed with @tynandebold on this - I'd keep it just as demo-project to keep in general. In future maybe we'd actually want this to be a directory with multiple projects: one for testing modular pipelines, one for testing some new feature we're developing, one for the demo, etc. It seems very plausible that a pipeline you develop for testing purposes (e.g. if you wanted to try 1000s of nodes) isn't necessarily the same as the one you use for a nice looking demo. And in that case each project would have more specific names.

@antonymilne
Copy link
Contributor

Though I guess the demo-project used on the demo branch that powers demo.kedro.org doesn't have to be the same as the one on main or any other branch.

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.

None yet

5 participants