cml pr: is racy, generates a PR for different submissions

[skshetry]

Sorry for the title. I don't have a good way to express what went wrong here.

This happened twice in ldb-hackathon for me where it generated a PR, where the contents were quite different from the result that I was expecting. In fact, the same contents were already generated before my workflow even ran. I am not quite sure here why cml is creating a PR with content that is already in the repo (even if the git pull is done before cml pr to try to keep the repo in sync).

1. 13:53 GMT: This PR #94 was published.
2. 13:55 GMT: Triggered workflow.
3. 13:57 GMT: Started "evaluate" job/train step.
4. 16:47:50 GMT: Finished training
5. 16:47:58 GMT: Created a PR #106 with identical contents to #94.

Run cml pr --rebase 'submissions/*.json'
  cml pr --rebase 'submissions/*.json'
  shell: sh -e {0}
  env:
    AWS_DEFAULT_REGION: us-east-1
    AWS_REGION: us-east-1
    AWS_ACCESS_KEY_ID: ***
    AWS_SECRET_ACCESS_KEY: ***
    AWS_SESSION_TOKEN: ***
    REPO_TOKEN: ***
{"level":"warn","message":"Failed to enable auto-merge: Enable the feature in your repository settings: https://github.com/iterative/ldb-hackathon/settings#merge_types_auto_merge. Trying to merge immediately..."}
https://github.com/iterative/ldb-hackathon/pull/106(https://github.com/iterative/ldb-hackathon/runs/7807546288?check_suite_focus=true#step:15:6)

Also I am a bit surprised to see cml pr using same branch name for submissions, which may likely be the issue here.
The commits are same and is from earlier time.

[dacbd]

cml pr by default bases the branch name off of the forking commit. so if multiple cml pr . invocations occur with the same parent the subsequent runs will fail.

This is/was part of the original design of cml pr for our particular use case with the hackathon it does feel like a bug. There is a --branch option that would have prevented this for example cml pr --branch "cml-$(date +%s)" --rebase "submissions/*.json"

[skshetry]

@dacbd, thank you for the reply. I am have one more question, why does it create a new PR with the same commit instead of overwriting with a new commit (or, getting push-rejected)? Is this also part of the design decision?

I would have at least expected it to overwrite the old commit, or add on top of it.

[dacbd]

That logic is programmed in; if that branch exists it will try the print the URL for the PR you can see here:

cml/src/cml.js

Lines 518 to 532 in 7973d0e

	const branchExists = (
	await exec(
	`git ls-remote $(git config --get remote.${remote}.url) ${source}`
	)
	).includes(source);
	if (branchExists) {
	const prs = await driver.prs();
	const { url } =
	prs.find(
	(pr) => source.endsWith(pr.source) && target.endsWith(pr.target)
	) || {};
	if (url) return renderPr(url);
	} else {

For the originally reasoning I'm not 100% sure. but I feel like it airs on the side of creating less PR spam if you are say debugging a training pipeline.

[0x2b3bfa0]

Note for the future

• Introduce subcommands in a backwards-compatible way #1073 (comment)

[casperdcl]

Would be great to have docs on this... and also potentially a new command (cml pr update or similar?)

See also SO#70123668