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

Cookie cutter initial template #738

Merged
merged 23 commits into from
Nov 9, 2021
Merged

Conversation

eapolinario
Copy link
Collaborator

@eapolinario eapolinario commented Nov 4, 2021

TL;DR

Add a command to generate the boilerplate for flyte-ready repos.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

This PR defines a new pyflyte command called init that takes a template and a project name as arguments. These are used to call the template engine provided by cookiecutter.

In terms of where templates are stored, we defined a cookiecutter template in flyteorg/flytekit-python-template#16. Note how we have the ability to keep multiple templates in the same repo, the idea being that we're going to expose them using different templates under the pyflyte init command.

The pyflyte init command defaults to "simple-example" cookiecutter template and this template prompts the user for 2 additional inputs:

  1. app: which is the top-level directory under project-name that will store the actual workflow
  2. workflow: the name of the example workflow.

All inputs have default values.

Also worth noting that the command does not clobber existing user code, i.e. the command fails if the user attempts to overwrite a directory by re-running the command.

Here's how the help provided by click looks like:

❯ pyflyte init --help
...
Usage: pyflyte init [OPTIONS] PROJECT_NAME

  Create flyte-ready projects.

Options:
  --template TEXT  cookiecutter template to be used
  --help           Show this message and exit.

Tracking Issue

flyteorg/flyte#1799

Follow-up issue

NA

@eapolinario
Copy link
Collaborator Author

Unit test is failing. Fix in #739.

# as described in https://cookiecutter.readthedocs.io/en/1.7.2/advanced/directories.html.
# The idea is to extend the number of templates, each in their own subdirectory, for example
# a tensorflow-based example.
directory="simple-example",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I mean. This is hard coded. Or are you thinking of parameterizing this? I was suggesting that you could create
pyflyte init tensorflow pyflyte init xyz
Or otherwise we should call it
pyflyte init <project-name> --template=tensorflow
and then --template can be defaulted to simple-example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking of parametrizing this going forward, but I really liked the suggestion of making project-name part of the invocation. I'll go ahead and implement both suggestions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also updated the PR description.

@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #738 (bf60412) into master (c4f33c6) will decrease coverage by 0.00%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #738      +/-   ##
==========================================
- Coverage   85.61%   85.60%   -0.01%     
==========================================
  Files         341      342       +1     
  Lines       28915    28926      +11     
  Branches     2381     2381              
==========================================
+ Hits        24755    24763       +8     
- Misses       3527     3530       +3     
  Partials      633      633              
Impacted Files Coverage Δ
flytekit/clis/sdk_in_container/init.py 66.66% <66.66%> (ø)
flytekit/clis/sdk_in_container/pyflyte.py 83.05% <100.00%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4f33c6...bf60412. Read the comment docs.

)
app = read_user_variable("app", "myapp")
click.echo("What should be the name of your example workflow?")
workflow_name = read_user_variable("workflow", "workflow_example")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just use click for consistency http://pallets-click.readthedocs.io/en/stable/prompts.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using cookiecutter just for the templating feels less cluttered. Thanks for the suggestion!

Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario eapolinario force-pushed the cookie-cutter-initial-template branch from d606b40 to 3f91dca Compare November 5, 2021 16:03
@eapolinario
Copy link
Collaborator Author

Only the code coverage tasks are failing.



@click.command("init")
@click.option("--template", default="simple-example", help="cookiecutter template to be used")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@click.option("--template", default="simple-example", help="cookiecutter template to be used")
@click.option("--template", default="simple-example", help="cookiecutter template folder name to be used in the repo - https://github.com/flyteorg/flytekit-python-template.git")

Also eventually I think the repo itself should be parameterizable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this idea of exposing the repo is interesting. So your expectation is that people will have their own private (or public forks) of the repo? In any case, if the need arises we can definitely go that way, but not in this PR, ok?

@kumare3
Copy link
Contributor

kumare3 commented Nov 5, 2021

Small comment then +1

kumare3
kumare3 previously approved these changes Nov 8, 2021
requirements.in Outdated Show resolved Hide resolved
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario eapolinario merged commit d846ed5 into master Nov 9, 2021
reverson pushed a commit to reverson/flytekit that referenced this pull request May 27, 2022
* Add cookiecutter to requirements and pyflyte init

Signed-off-by: Eduardo Apolinario <[email protected]>

* Invoke cookiecutter with the overridden configuration.

Signed-off-by: Eduardo Apolinario <[email protected]>

* Comment the use of directory in the call to cookiecutter.

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use the original repo and do not clobber existing files+directories

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add example sub-command

Signed-off-by: Eduardo Apolinario <[email protected]>

* make requirements

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix test_dataclass_transformer test

Signed-off-by: Eduardo Apolinario <[email protected]>

* Constrain mashmallow-jsonschema temporarily

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add --template flag and project-name argument to command

Signed-off-by: Eduardo Apolinario <[email protected]>

* Revert changes to dataclass transformer tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Constrain marshmallow-jsonschema in dev-requirements

Signed-off-by: Eduardo Apolinario <[email protected]>

* Revert "Constrain marshmallow-jsonschema in dev-requirements"

This reverts commit bf7f22d.

* Fix typo in Makefile

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix regeneration of spark2 requirements file

Signed-off-by: Eduardo Apolinario <[email protected]>

* Use click for prompts

Signed-off-by: Eduardo Apolinario <[email protected]>

* Changes default workflow name to `my_wf`

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove the two inputs from simple-example

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add more descriptive help for the template flag

Signed-off-by: Eduardo Apolinario <[email protected]>

* Set cookiecutter in setup.py

Signed-off-by: Eduardo Apolinario <[email protected]>

* Default top level directory to flyte (as opposed to myapp)

Signed-off-by: Eduardo Apolinario <[email protected]>

Co-authored-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Robert Everson <[email protected]>
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.

3 participants