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(cli): add renku rollback command #2426

Merged
merged 10 commits into from Nov 26, 2021
Merged

feat(cli): add renku rollback command #2426

merged 10 commits into from Nov 26, 2021

Conversation

Panaetius
Copy link
Member

@Panaetius Panaetius commented Oct 20, 2021

closes #2351
closes #440

adds a renku rollback command. The user gets presented with a (paginated) history of renku actions and can pick a point in time to return to, they are then shown all entities and files that would be changed by this action and, if they confirm, the repo is reset to that state.

Also removes the need for most renku commands to create multiple commits (with the exception of migrations).

Can be tested in https://renkulab.io/projects/ralf.grubenmann/test-rollback

Command to run: renku rollback

@Panaetius Panaetius force-pushed the 2351-renku-rollback branch 8 times, most recently from a9b879c to 97d7bcb Compare October 21, 2021 06:18
@Panaetius Panaetius marked this pull request as ready for review October 21, 2021 06:58
@Panaetius Panaetius requested a review from a team as a code owner October 21, 2021 06:58
vigsterkr
vigsterkr previously approved these changes Oct 22, 2021
Copy link
Contributor

@vigsterkr vigsterkr left a comment

Choose a reason for hiding this comment

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

🚀

@rokroskar
Copy link
Member

+1 on the PR description 😻

@rokroskar
Copy link
Member

This looks great @Panaetius - just a few quick observations:

  1. why no commit sha in the listing?
  2. what happens if the number of commits/renku commands is very long?
  3. a wording suggestion - since you say at the top "The following changes would be done" you can omit "that would be" from the lines that follow
  4. this needs colors and emojis 😆

@Panaetius
Copy link
Member Author

Panaetius commented Oct 25, 2021

This looks great @Panaetius - just a few quick observations:

1. why no commit sha in the listing?

Well I thought if we ever allow running renku commands without commit (i.e. you have to explicitly commit once you're happy) we wouldn't have one. But at the moment we can add it, might be useful.

2. what happens if the number of commits/renku commands is very long?

it starts paging after 50 entries (you press 'm' to show the next 50), in a lazy way (so we don't have to parse the whole commit history every time the command is run).

3. a wording suggestion - since you say at the top "The following changes would be done" you can omit "that would be" from the lines that follow

👍

4. this needs colors and emojis laughing
Metadata:
    Modified ♻️:
            Dataset: e
    Removed 🔥🗑️:
            Plan: cp-blabla-asdasf-0b535
            Plan: test
            Run: /activities/cc3ab70952fc499e93e7e4075a076bf5 (Plan name: cp-blabla-asdasf-0b535)
            Run: /activities/48b89b22567d4282abe8a016fa91878f (Plan name: cp-blabla-asdasf-0b535)
Files:
    Restored ⊞:
            blabla
    Removed 🔥🗑️:
            asdasf

?

@rokroskar
Copy link
Member

Hmm good point regarding the sha. How would this work if commands were not tied to commits? I guess we would make a new commit that would undo whatever had been done? Or we would revert to a commit before the command in question? In the latter case, having the commit sha would still be useful since it would allow the user to quickly place the command in the broader context. The emoji suggestion was mostly a joke, but I like 🔥 😄

@gavin-k-lee
Copy link

Hi @Panaetius I'm currently testing the repo out - I've run into a bug - I can't seem to fetch datasets
image
and when I try to add datasets from the UI:
image
Is there any way I can remedy this in this repo?

@Panaetius
Copy link
Member Author

Panaetius commented Oct 26, 2021

@gavin-k-lee You can't use that project on renkulab, it uses renku-python version 1.0.0 and is not supported by the current version of renkulab.

You should be able to start an interactive session and work there (the Dockerfile is set up to use this branch) or clone it locally, install this branch and work there.

@gavin-k-lee
Copy link

Looking good so far! It might be the browser but the bin emoji doesn't show for some reason
Chrome:
image
Firefox:
image

@Panaetius
Copy link
Member Author

@gavin-k-lee The garbage bin? I didn't want to spam emojis and just added the flame

@gavin-k-lee
Copy link

Oh right ok.

renku/cli/rollback.py Outdated Show resolved Hide resolved
@rokroskar
Copy link
Member

This looks good (apologies for taking a while to re-review)!

rokroskar
rokroskar previously approved these changes Nov 3, 2021
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

🔥 ♻️

@Panaetius Panaetius enabled auto-merge (squash) November 4, 2021 13:37
@rokroskar
Copy link
Member

This actually also (at least partially) closes #440 and #338

from renku.core.models.workflow.plan import AbstractPlan
from renku.core.utils import communication

CHECKPOINTS_PER_PAGE = 50
Copy link
Contributor

Choose a reason for hiding this comment

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

why define twice this static var? see in renku/cli/rollback.py above CHECKPOINTS_PER_PAGE = 10

Copy link
Contributor

@vigsterkr vigsterkr left a comment

Choose a reason for hiding this comment

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

:shipit:

@Panaetius Panaetius merged commit 83fb842 into master Nov 26, 2021
@Panaetius Panaetius deleted the 2351-renku-rollback branch November 26, 2021 08:15
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 a renku rollback command renku undo command
4 participants