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

Promoting version to the same version twice #199

Closed
aguschin opened this issue Jun 28, 2022 · 6 comments
Closed

Promoting version to the same version twice #199

aguschin opened this issue Jun 28, 2022 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@aguschin
Copy link
Contributor

☝️ While investigating, I found two edge cases that are introduced by the way GTO currently works.

1. A version can only be promoted to production once in the same commit. If after this promotion a different version of the same artifact is promoted to the same stage, you can promote the first version again.

For CommitA (sha: 'commit-a) & CommitB (sha: 'commit-b) this script:

$ gto register model commit-a --version v0.0.1
$ gto register model commit-b --version v0.0.2

$ gto promote model production commit-a
$ gto promote model production commit-b
$ gto promote model production commit-a

Will yield the following git tags:

Commit A: model@v0.0.1 model#production#1 model#production#3
Commit B: model@v0.0.2 model#production#2

Proposed solution:
We could either show all promotions for that commit, or filter ‘duplicate’ promotions. For Commit A

  1. All promotions would show; v0.0.1 - production & v0.0.1 - production (twice)
  2. Only one would be shown v0.0.1 - production

2. Promoting a model without registering a new version

For CommitC (sha: 'commit-c'), this script:

$ gto promote model production commit-c --skip-registration

Will yield only a git tag model#production#4, and no version registration tag.

Proposed solution
We should still show this promotion if the model column is set to Promotion display setting. We should also allow this commit to be a candidate for a version registration if the model column display setting is switched to Version registrations.

Originally posted by @jellebouwman in https://github.com/iterative/studio/issues/3606#issuecomment-1168249521

@aguschin
Copy link
Contributor Author

Thank you @jellebouwman!

  1. It's a bug, I'll fix this.
  2. This is to support simple use case when user doesn't want to register SemVers, and care only about promotions. It kinda aligned with "simple intuitive actions should be possible" idea from @dmpetrov: like promotion with --simple flag that creates rf#prod instead of rf#prod#1. Agree with "Proposed solution". In CLI it will look like this:

Screen Shot 2022-06-28 at 18 15 38

QQ about "We should also allow this commit to be a candidate for a version registration" - do you mean the History part?
image

If yes, we may skip showing it since in GTO counterpart there is nothing about it. Then it will be aligned. E.g.:
Screen Shot 2022-06-28 at 18 18 36

The reason for this is that it's not actual registration. That's why I assumed there is no need in showing registration action, but show only promotion action there.

cc @omesser

@aguschin aguschin changed the title ☝️ While investigating, I found two edge cases that are introduced by the way GTO currently works. Promoting version to the same version twice. Q about promoting a model without registering a new version Jun 28, 2022
@aguschin aguschin added the bug Something isn't working label Jun 28, 2022
@aguschin aguschin changed the title Promoting version to the same version twice. Q about promoting a model without registering a new version Promoting version to the same version twice Jun 28, 2022
@jellebouwman
Copy link
Contributor

jellebouwman commented Jun 28, 2022

This is to support simple use case when user doesn't want to register SemVers, and care only about promotions. It kinda aligned with "simple intuitive actions should be possible" idea from @dmpetrov: like promotion with --simple flag that creates rf#prod instead of rf#prod#1. Agree with "Proposed solution".

Alright, thanks for confirming this is a valid case. We will probably not support doing this in Studio at this stage. We will have to be able to show it in the View table however. Good to know!

QQ about "We should also allow this commit to be a candidate for a version registration" - do you mean the History part?

As I understand it, with gto promote's --skip-registration option you specify a commit sha, or HEAD, and specifically say that you do not want to register a version at that commit.

If, at a later stage, you would like to register that commit as a version, a registration action could be done, right?

@aguschin
Copy link
Contributor Author

Alright, thanks for confirming this is a valid case. We will probably not support doing this in Studio at this stage. We will have to be able to show it in the View table however. Good to know!

Yes, it's definitely doesn't need to be supported in Studio now. The default flow (with registration) is cleaner and should be advised.

If, at a later stage, you would like to register that commit as a version, a registration action could be done, right?

Yes!

@aguschin aguschin self-assigned this Jun 28, 2022
@tapadipti
Copy link
Contributor

It's a bug, I'll fix this.

@aguschin So if the user does promote an older version to production again, then what are the expected tags? In Jelle's example above, they are model#production#1 and model#production#3. Will it just be model#production#1 instead?

@aguschin
Copy link
Contributor Author

aguschin commented Jul 4, 2022

@tapadipti
No, it will be model#production#3. But to promote this, you need to add --force flag (or force=True arg in API). In the first iteration in Studio we can, either:

  1. Always use force=True and create tag
  2. Don't use force=True and error out in this case, saying that the model version is already promoted to selected stage (I suggest this option, since this is an extra case to promote what already was promoted)
  3. Add a flag to allow enable force=True - but this will require development on both BE and FE side.

@aguschin
Copy link
Contributor Author

aguschin commented Jul 4, 2022

Seems like this was fixed by #201
Added a regression test for this in #202

@aguschin aguschin closed this as completed Jul 5, 2022
@aguschin aguschin moved this to Done in MLEM + GTO Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants