comment --pr=true default

[casperdcl]

https://github.blog/changelog/2022-08-04-commit-comments-no-longer-appear-in-the-pull-request-timeline/ meaning the --pr flag is now mandatory for CML reports to show up in PRs. Kinda a UX breaking change potentially.

Options (pick just one):

1. update docs & examples, adding --pr everywhere (breaking change, examples: add --pr cml.dev#331)

also auto-detect this/print warnings?

$ cml comment ...
(ok)
$ cml pr ...
(ok)
$ cml comment ...
WARNING: did you forget `--pr` flag?

2. cml comment ... [--pr=true] by default (UX friendly)
   • iff not in a PR, print a warning and fallback to --pr=false (commit-comment)
   • users need explicit --pr=false/--no-pr to use commit-comments in PRs (edge case)
3. cml comment ... [--pr=auto] by default (better fail-not-warn error handling)
   • try to PR-comment, fallback to commit-comment
   • explicit --pr={true,false} throws error on failure (no fallback)
   • however, is this implementation possible? does yargs allow --flag=foo == "foo" && --flag == true (latter needed for backwards compat)?

[0x2b3bfa0]

Historical context

Traditionally, cml send-comment has operated on individual commits rather than pull requests.

We promoted a workflow where users opened a single pull request per “experimentation session” and then pushed [potentially] several commits, each one with a different “experiment” like modifying an hyperparameter. Reports for every of these changes would be available as a commit comment¹ as soon as the training process finished, and were guaranteed to match the code from a specific commit.

Then, there was a request for creating pull request comments rather than commit comments (#735) to avoid losing comments when rebasing a pull request.

Footnotes

1. Only if supported by the forge. Bitbucket used a pull request comment with the commit hash on it as a workaround. ↩

[0x2b3bfa0]

Suggested solution

Commit comments are unequivocally tied to a commit, but aren't supported on every forge.¹ Wouldn't it be better if we used --pr everywhere and, perhaps, added a footer showing the commit hash?

Footnotes

1. Bitbucket, as usual, does not support commit comments. ↩

[casperdcl]

blocked on #1172 ?

[tasdomas]

One problem with requiring

      - uses: actions/checkout@v3
        with:
          ref: ${{ github.event.pull_request.head.sha }}

in the workflow definition for cml comment create --pr to work is that we're not including any master changes in the context of the action.

I think we should augment cml drivers with forge-specific logic for determining the pull request (if there is one) to add comments to.

[dacbd]

we're not including any master changes in the context of the action.

I'm not following, can you elaborate?

[casperdcl]

I think @tasdomas means checking $GITHUB_HEAD_REF instead of requiring explicit checkout.with.ref: ${{ github.event.pull_request.head.sha }} 301 Moved Permanently to #1204

[casperdcl]

updated the title & description a bit, clearer now?

[tasdomas]

1. Bitbucket, as usual, does not support commit comments. [leftwards_arrow_with_hook](#user-content-fnref-1-bc519f06fb4bd021a0f1bade91b47e07)

What about this ?

[0x2b3bfa0]

What about this?

Probably a layer-8 error on my side 🤔

[casperdcl]

opened #1223

[dacbd]

@casperdcl, with the improved options and logic with the introduction of cml comment create --target=x, I think we can close this?

[casperdcl]

Yes closed by #1228