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

feat(core): add template command #2590

Merged
merged 18 commits into from Feb 21, 2022
Merged

feat(core): add template command #2590

merged 18 commits into from Feb 21, 2022

Conversation

m-alisafaee
Copy link
Contributor

@m-alisafaee m-alisafaee commented Jan 19, 2022

Description

renku template command to list, set, or update projects' template. It allows forced updates and provides an interactive flag to ask users what to do with each file that exist in the project.
Project.automated_update is deprecated now and templates can be always updated if they don't have a fixed reference.

Testing

This can be tested on any project. I've used this one: https://dev.renku.ch/gitlab/mohammad.alisafaee/lego-sets.git

Example commands:

  • renku template update
  • renku template update --dry-run
  • renku template set -f python-minimal -s https://dev.renku.ch/gitlab/renku-python-integration-tests/core-it-template-variable-test-project
  • renku template set -i R-minimal
  • renku template ls
  • renku template show -s https://dev.renku.ch/gitlab/renku-python-integration-tests/core-it-template-variable-test-project
  • renku template show

Fixes #2460
Fixes #1482

@m-alisafaee m-alisafaee force-pushed the 2460-template-update branch 9 times, most recently from e90770b to fd6b01b Compare January 25, 2022 21:49
@m-alisafaee m-alisafaee marked this pull request as ready for review January 26, 2022 09:19
@m-alisafaee m-alisafaee requested a review from a team as a code owner January 26, 2022 09:19
@rokroskar
Copy link
Member

does renku template ls show me the templates that are loaded in renku-python or does it clone the default template repo? In either case, it would be nice to know e.g. the tag I'm looking at and the location it's taken from. Also, I get a warning when I run template ls where I think it isn't needed (I'm just inspecting templates, not doing a project action - I'm actually not even in a project at all):

renku template ls                                                                                                                                                                                                                                                                                                                           
Warning: Run CLI commands only from project's root directory.

@rokroskar
Copy link
Member

I guess this answers my previous question:

renku template set -i -f -r R-minimal                                                                                                                                                                                                                                                              
Error: Invalid parameter value - Templates included in renku don't support specifying a template_ref

@rokroskar
Copy link
Member

I really like the addition of the set command! Maybe we could only ask about overwriting files that are actually different? e.g. I know for a fact .gitkeep is the same :) In fact, running set -i -f -t python-minimal -s https://dev.renku.ch/gitlab/renku-python-integration-tests/core-it-template-variable-test-project only changed a small fraction of the files - it would greatly simplify the flow (the user doesn't really know what .renkulfsignore is, for example, so they won't know if they want it overwritten)

git show --name-only                                                                                                                                                                                                                                                                             
commit eb1be2a207033f9e624be222e5cdb4fe8c0b7a48 (HEAD -> master)
Author: Rok Roškar <rok.roskar@sdsc.ethz.ch>
Date:   Fri Jan 28 09:29:26 2022 +0100

    renku template set -i -f -t python-minimal -s
            https://dev.renku.ch/gitlab/renku-python-integration-tests/core-it-template-variable-test-project

    renku-transaction: 1b5ee25c879b4400a3e7e73ac5e8c726

.renku/metadata/project
.renku/template_checksums.json
Dockerfile
README.md

@rokroskar
Copy link
Member

This functionality is really close, but it's missing what I really want it to be able to do --> update my template when a new version has been tagged/released. Maybe we are missing a concept here since we are using ref for everything (branches, tags, commits). Should we think about a notion of "version"? And when I do renku template update I get a list of newer versions I can update to? Yes this complicates things with branches since a tag can presumably transcend a branch designation, but I expect that to be really a 0.01% edge case. We would need to decide on a tag regex that defines versions, but a normal semver regex (e.g. [0-9]+\.[0-9]+\.[0-9]+) seems ok. I guess an extension could be to make that configurable (via the template manifest.yaml file?) but I'm not even sure it's needed atm.

@gavin-k-lee
Copy link

Hi @mohammad-sdsc, thanks for putting this together. I wasn't able to run renku template from your project here: https://dev.renku.ch/gitlab/mohammad.alisafaee/lego-sets.git so I built it against this branch on a fork here: https://dev.renku.ch/gitlab/lee.gavin.k/lego-sets and so I have:

 base ▶ ~ ▶ work ❯ lego-sets ▶ master ▶ $ ▶ renku --version
1.0.1.dev45+gfd6b01b7

When I ran renku template it didn't find a manifest file:

renku template ls
Error: There is no manifest file 'manifest.yaml'
...
FileNotFoundError: [Errno 2] No such file or directory: '/home/jovyan/.renku/venv/lib/python3.9/site-packages/renku/templates/manifest.yaml'

so I don't think this is the right version of the Renku I have in my project. Are you able to point me to the right renku version? Thanks

@Panaetius
Copy link
Member

@gavin-k-lee You might need to do make download-templates before doing pip install .[all] on this branch (This happens automatically for proper releases. it's a long story, but we can hopefully fix this at some point).

Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, it is a really big change and adding a ^renku template` command group was sorely needed!

Sorry for the ton of comments, but when looking over your and my old code I just realized that we could structure all this in a more clean way 🙈

renku/cli/__init__.py Outdated Show resolved Hide resolved
renku/cli/__init__.py Outdated Show resolved Hide resolved
renku/cli/template.py Show resolved Hide resolved
renku/cli/template.py Outdated Show resolved Hide resolved
renku/cli/template.py Outdated Show resolved Hide resolved
renku/core/utils/templates.py Outdated Show resolved Hide resolved
renku/core/utils/templates.py Outdated Show resolved Hide resolved
renku/core/utils/templates.py Outdated Show resolved Hide resolved
renku/core/utils/templates.py Outdated Show resolved Hide resolved
renku/core/utils/templates.py Outdated Show resolved Hide resolved
@m-alisafaee
Copy link
Contributor Author

@rokroskar Thanks for the review!

This functionality is really close, but it's missing what I really want it to be able to do --> update my template when a new version has been tagged/released. Maybe we are missing a concept here since we are using ref for everything (branches, tags, commits). Should we think about a notion of "version"? And when I do renku template update I get a list of newer versions I can update to? Yes this complicates things with branches since a tag can presumably transcend a branch designation, but I expect that to be really a 0.01% edge case.

You can use template update --force to force an update which ignores the ref and tries to update from the repo's HEAD. Isn't this enough for this use-case?

We use ref as a way to pin template to a fixed version (when ref is a tag for example). If it's not set then update works without the force flag. An issue is that projects created via UI always have a ref set for them even when they use Renku template. I believe this is not required and it should be changed to allow updates. We can also come up with another way to pin a template version and always update templates with ref.

@gavin-k-lee
Copy link

Looks great! It's certainly a powerful feature and I liked the interactive write/overwrite capability. I second what Rok mentioned. I think having support to update/pull to a particular ref is quite important, especially if a project-template repo has changes often (e.g. with a class where a teacher updates materials frequently).

@rokroskar
Copy link
Member

@mohammad-sdsc I don't doubt that pinning the template version is useful - but it's not a nice UX imho to expect the user to go and look up a newer tag themselves. We should be able to present a list of tags to the user and offer to update to another one. Of course not any random tag, but ones that match some pre-defined regex. We can't just initialize projects with the tip of the master branch because a new template might be incompatible with what is on renkulab.

@m-alisafaee m-alisafaee marked this pull request as draft February 1, 2022 12:49
@m-alisafaee m-alisafaee force-pushed the 2460-template-update branch 6 times, most recently from 8c28ce8 to c3f22c3 Compare February 16, 2022 00:48
@m-alisafaee m-alisafaee force-pushed the 2460-template-update branch 2 times, most recently from 5fba1c6 to cc19aa1 Compare February 16, 2022 16:43
@m-alisafaee m-alisafaee marked this pull request as ready for review February 17, 2022 07:51
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Thanks you, this looks so much cleaner now!

Comment on lines +241 to +242
print_name = functools.partial(click.style, bold=True, fg="magenta")
print_value = functools.partial(click.style, bold=True)
Copy link
Member

Choose a reason for hiding this comment

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

Oh I like this, we should move it to some file and combine it with the color changes in https://github.com/SwissDataScienceCenter/renku-python/pull/2633/files#diff-7d96f8caf55d6ff0f188af796d7ce5af7963c75621a4399e0197aceaa2f3d86bR118 and make it so all the print functions use it.

renku/core/management/template/template.py Outdated Show resolved Hide resolved
renku/core/management/template/template.py Outdated Show resolved Hide resolved
renku/core/management/template/template.py Outdated Show resolved Hide resolved
renku/core/management/template/template.py Outdated Show resolved Hide resolved
renku/core/management/template/template.py Outdated Show resolved Hide resolved
renku/core/models/template.py Outdated Show resolved Hide resolved
if not self.path.exists():
raise errors.InvalidTemplateError(f"Template directory for '{self.id}' does not exists")

# TODO: What are required files
Copy link
Member

Choose a reason for hiding this comment

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

We should check this, like environment.yml might be required, too

renku/core/models/template.py Outdated Show resolved Hide resolved
Panaetius
Panaetius previously approved these changes Feb 18, 2022
@m-alisafaee m-alisafaee merged commit 4ff9c4f into develop Feb 21, 2022
@m-alisafaee m-alisafaee deleted the 2460-template-update branch February 21, 2022 15:36
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.

improve the logic of template updates Creating a project via the CLI makes all files without final newlines
4 participants