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

cml pr trigger CI pipelines on GitLab #1023

Closed
francesco086 opened this issue May 25, 2022 · 16 comments
Closed

cml pr trigger CI pipelines on GitLab #1023

francesco086 opened this issue May 25, 2022 · 16 comments
Assignees
Labels
bug Something isn't working ci-gitlab cml-pr Subcommand external-request You asked, we did

Comments

@francesco086
Copy link
Contributor

Following this message on Discord: https://discord.com/channels/485586884165107732/728693131557732403/978973993338044456

When the command cml pr --merge dvc.lock is executed as part of a GitLab CI pipeline, a new MR is created (as expected) and CI pipeline for that commit is triggered, which creates a new MR, etc. (loop of CI pipelines)
If you look at the pipelines you will see several consecutive automatically triggered pipelines. Same if you look in the MRs (closed by me).

@francesco086
Copy link
Contributor Author

Forgot to mention, this is the branch where I am trying to make use of cml pr: https://gitlab.com/francesco-calcavecchia/titanic-demo-dvc-as-model-registry/-/tree/rely_solely_on_ci_for_dvc_repro

@casperdcl casperdcl added bug Something isn't working cml-pr Subcommand ci-gitlab labels May 25, 2022
@DavidGOrtega
Copy link
Contributor

If we are adding [ci skip] gitlab might be not honouring it (again)?

@francesco086
Copy link
Contributor Author

francesco086 commented May 27, 2022

Reading from here: "Do not create a CI pipeline for the latest push. Only skips branch pipelines and not merge request pipelines."

In my gitlab-ci I did not configure merge request pipelines, so it shouldn't be a problem. However, it could become an issue in some other cases. If this is really the case it would be worth to mention it in the documentation...?

@francesco086
Copy link
Contributor Author

I am observing that the CI pipelines are skipped if I omit the --merge option for cml. Curious about it, I checked in the code, and here found something interesting. Why do you append skip ci to the commit message only if not merge (or squash or rebase)? Maybe that is the issue?

@casperdcl
Copy link
Contributor

casperdcl commented May 27, 2022

That is a bug 🙈 :)

I think I know the original thinking (don't skip-ci post-merge!) but said thinking is a bit flawed.

I think we should also try to put [skip ci] into the PR/MR title & body (if CI honour this?) instead of commit body if possible.

@francesco086
Copy link
Contributor Author

Ok, happy to see that I got it right :)

While waiting for your reply I was already checking into your further suggestion. In fact currently, even if the new branch does not activate the ci, once you merge it will! And that will create a new merge request etc.
So I was looking for a way to avoid that, and indeed if you set the title of the MR to include [skip ci] you should solve the problem.

@francesco086
Copy link
Contributor Author

It doesn’t seem very difficult to make these couple of changes, shall I create a MR for you? :)

@casperdcl
Copy link
Contributor

casperdcl commented May 28, 2022

once you merge it will!

That's typically intended behaviour (always test main). The workflow should be written to only create PRs from non-main branches (to avoid infinite PRs on cml pr --merge). I guess you have a different problem?

  1. Push feature branch containing experiment hyperparams
  2. CI opens PR (cml pr --merge) containing experiment results
  3. PR gets merged into feature branch
  4. repeat from step (2). Recursion ad infinitum

Potential work-arounds:

  1. Make runs deterministic so rerunning with same hyperparams produces identical results (empty diff so no PR)
  2. Have cml pr --merge use default github-actions token (to not trigger another CI run). Not sure if there's a GitLab equivalent.
    • feature request: option to add [skip ci] to the PR title (afaik the PR title usually becomes the commit title upon merge)

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented May 29, 2022

It doesn’t seem very difficult to make these couple of changes, shall I create a MR for you? :)

That would be awesome! 🙏

@francesco086
Copy link
Contributor Author

Here you go: #1035

PS: I would have preferred to first add tests and then make the changes, but I have never worked with js, and I don't have the time to figure out how to set up the development environment.

@francesco086
Copy link
Contributor Author

francesco086 commented May 30, 2022

Potential work-arounds:

1. Make runs deterministic so rerunning with same hyperparams produces identical results (empty diff so no PR)

2. Have `cml pr --merge` use default github-actions token (to not trigger another CI run). Not sure if there's a GitLab equivalent.
   
   * feature request: option to add `[skip ci]` to the PR title (afaik the PR title usually becomes the commit title upon merge)

Deterministic runs are of course desirable and should be the aim, but sometimes things go unexpectedly, so I would suggest no to rely on it, if it is not strictly necessary.

Regarding 2: I don't know about github-actions token, sorry. But I am quite sure that the MR title would solve the issue in GitLab. As it does not break anything, I don't see why not going that away, rather than having to set up extra stuff.

@francesco086
Copy link
Contributor Author

I take back my previous comment on MR title fixing the issue for GitLab :(

@francesco086
Copy link
Contributor Author

Checking again in the cml code and the GitLab API reference. I believe that one could avoid the problem mentioned above by adding [skip ci] to the commit message when merging the merge request, see here.

In practice, one could add the commit message with [skip ci] here: https://github.com/francesco086/cml/blob/33a54a5e8e4581122b76113f7b2c3dcddd4ecd55/src/drivers/gitlab.js#L267
or, probably better, add the skipci option once again, and in case append it to the commit message in the automerge...?

@DavidGOrtega
Copy link
Contributor

@francesco086 thanks for the contribution!!! ❤️
We have already merged it and will be available in the next release within a few hours!

@DavidGOrtega
Copy link
Contributor

@francesco086
Copy link
Contributor Author

francesco086 commented Jun 2, 2022

@francesco086 thanks for the contribution!!! ❤️

It was my pleasure :) thank you for the guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci-gitlab cml-pr Subcommand external-request You asked, we did
Projects
None yet
Development

No branches or pull requests

4 participants