Integrate `cml publish` with `cml send-comment` #1026

0x2b3bfa0 · 2022-05-27T02:31:06Z

Use npm install --global github:iterative/cml#7bd9257 to use this feature.

Usage

tee report.md <<END
# Report
![cat](cat.jpg)
END

npx github:iterative/cml#7bd9257 send-comment --publish --watch report.md &

while sleep 30; do
  curl --location https://thecatapi.com/api/images/get?format=src > cat.jpg
done

When using locally (as opposed to running from CI/CD), provide also --repo https://github.com/user/repository, --token ghp_personal_access_token and --commit-sha a1b2c3d pointing to a commit on that repository, preferably part of an open pull request.

Behavior

--publish — uploads and replaces all the local paths on report.md (e.g. links & images)
- e.g. ![description](outputs/plot.png) becomes ![description](https://assets.cml.dev/...)
--watch — watches report.md and all the local paths it contains
- i.e. when any of them changes, updates the comment in the forge

Experimental

--trigger-file — specify a trigger file with the same behavior as DVC checkpoint file-based API¹
- only effective along with --watch

Using a trigger file

cml send-comment --publish --watch --trigger-file=example report.md &

while true; do
  date > report.md # modify the report
  touch example # trigger an update
  while test -f example; do sleep 1; done # wait for the update to finish
done

Pending

Figure out the consequences of having no synchronization mechanism¹
- Added a primitive file-based trigger, functionally equivalent to the DVC checkpointing file-based API
Account for cases where report.md doesn't yet exist when starting the watcher
Use a proper file watcher on all the paths instead of polling
Setting awaitWriteFinish to true triggers infinite change events paulmillr/chokidar#1224
- 4c21ff5 & ab28d8d

Reverted

Segmentation fault (core dumped) on Github Actions running Node.js 12.x jestjs/jest#10662 (comment)
- e9465da
Migrate to ECMAScript modules #1051
- Not a blocker, just an eyesore

Questions

is there any use case for calling cml publish directly after we implement this?
- maybe (e.g. publish for use outside markdown reports?), though probably fine to keep for backward-compat? — @casperdcl dixit
- fine to keep hidden, perhaps, as any other deprecated command? — @0x2b3bfa0 respondebat
- p3-whatever — @casperdcl 🙃
How does this work outside CI? Are we asking users to run this before using DVCLive?
- yes, but
- technically this PR is (currently) standalone without anything to do with integrations: CML dvclive#91. Moving meta discussion to epic: CML <> DVCLive #1036.

As the DVC team may know, a file-based API needs synchronization to avoid all sorts of pitfalls; e.g. like lost events, rate limits, corrupted files... et cetera. This makes me question whether we should follow this approach or not. ↩ ↩²

casperdcl · 2022-05-27T07:29:01Z

btw shouldn't cml publish --update be a prerequisite (i.e. overwriting existing files on assets.cml.dev)?

0x2b3bfa0 · 2022-05-27T12:11:00Z

@casperdcl, cml publish --update is not necessary, because storage is content-addressed.

$ echo one > file
$ cml publish file
https://host/4355a46b19d348dc2f57c046f8ef63d4538ebb936000f3c9ee954a27460dd865
$ echo two > file
$ cml publish file
https://host/53c234e5e8472b6ac51c1ae1cab3fe06fad053beb8ebfd8977b010655bfdd3c3
$ echo two > file
$ cml publish file # same CONTENT, thus same URL
https://host/53c234e5e8472b6ac51c1ae1cab3fe06fad053beb8ebfd8977b010655bfdd3c3

Location-addressed storage is probably what you’re looking for with cml publish --update

$ echo one > file
$ cml publish file
https://host/file
$ echo two > file
$ cml publish file
https://host/file
$ echo three > file
$ cml publish --update file # same PATH, thus same URL
https://host/file

However, paths generated this second way are:

Insecure, i.e. easy to IDOR without authentication
Prone to undesirable collisions across users, repositories and workflow runs
Unable to bypass GitHub user content cache; it rewrites all the image source URLs to camo.githubusercontent.com¹

If dynamic status badges work despite this, it may be a non-issue (?) ↩

0x2b3bfa0 · 2022-05-27T13:10:32Z

An intermediate solution to avoid clutter and preserve desirable properties of storage is:

Generate a UUIDv4 when the daemon starts
Use it as a prefix for location-based storage

casperdcl · 2022-05-27T13:18:49Z

I mean this:

$ echo one > file
$ ONE_URL=$(cml publish file)
$ curl -I $ONE_URL | head -n1
HTTP/1.1 200 OK
$ echo two > file
$ TWO_URL=$(cml publish file)
warn: CML detected subsequent publish of same filename in same session. Deleting old file.
$ curl -I $TWO_URL | head -n1
HTTP/1.1 200 OK
$ curl -I $ONE_URL | head -n1
HTTP/1.1 404 Not Found

To avoid us hosting 1M images per training run.

0x2b3bfa0 · 2022-05-27T13:23:56Z

CML detected subsequent publish of same filename in same session

Sounds like #1026 (comment)?

0x2b3bfa0 · 2022-05-27T20:53:39Z

Related to iterative/dvclive#91 (comment)

src/cml.js

0x2b3bfa0 · 2022-05-30T08:07:31Z

After #1026 (comment) and some other limitations¹ of the file event interface, I wonder if this is the right approach to integrate CML with other tools. 🤔

Namely, the inability of synchronizing events with rate limits, and the issues with corrupted files during write. ↩

dacbd

Code LGTM, functionality I'll defer to others. 😁 🥼

0x2b3bfa0 · 2022-06-14T21:38:18Z

@iterative/cml, shall we merge this?

bin/cml/send-comment.js

casperdcl · 2022-06-16T06:34:51Z

bin/cml/send-comment.js

+      },
+      triggerFile: {
+        type: 'string',
+        description: 'File used to trigger the watcher',


Trigger how?

Suggested change

description: 'File used to trigger the watcher',

description: 'If specified, --watch will trigger only when the lockfile is present, and will delete the lockfile',

Exactly as stated in the “using a trigger file” section on #1026 (comment)

It's hard to describe in a help text. 😅

Takes a path to a regular file that doesn't exist yet

Creating a regular file in that path triggers an --update

Once the --update finishes, the file gets automatically deleted

Suggested change

description: 'File used to trigger the watcher',

description: 'Path to the watcher trigger; create a file on that path to triger an update, then wait until the file disappears',

Need to do #762 before rewording this again :)

update: If you'd prefer to do #762 immediately after this PR please do feel free to leave this unresolved here @0x2b3bfa0

If you're happy to see #1073 merged immediately after this pull request, fine by me. I would push a new commit to #1073 with unified periods and (perhaps) some better descriptions.

Given that this is a hidden option and we don't know if DVCLive is going to use it, I'd rather not care too much about rewording (?)

resolving unresolved

bin/cml/send-comment.js

src/cml.js

casperdcl

only had a super quick glance, just checking - is this "solution to avoid clutter and preserve desirable properties of storage" or equivalent implemented?

0x2b3bfa0 · 2022-06-16T11:00:11Z

only had a super quick glance, just checking - is this "solution to avoid clutter and preserve desirable properties of storage" or equivalent implemented?

_yes, yes, YES

Resolved

Integrate cml publish with cml send-comment

bcaaaeb

0x2b3bfa0 temporarily deployed to internal May 27, 2022 02:31 Inactive

Update snapshot tests

36b6c6b

0x2b3bfa0 temporarily deployed to internal May 27, 2022 03:00 Inactive

daavoo mentioned this pull request May 27, 2022

live: Add report option. iterative/dvclive#215

Closed

0x2b3bfa0 mentioned this pull request May 27, 2022

integrations: CML iterative/dvclive#91

Closed

daavoo reviewed May 28, 2022

View reviewed changes

src/cml.js Outdated Show resolved Hide resolved

Merge branch 'master' into comment-publish-report

afe8be5

0x2b3bfa0 temporarily deployed to internal May 29, 2022 23:32 Inactive

0x2b3bfa0 temporarily deployed to internal May 29, 2022 23:35 Inactive

0x2b3bfa0 force-pushed the comment-publish-report branch from 30d35c0 to eaf33bc Compare May 30, 2022 00:00

0x2b3bfa0 temporarily deployed to internal May 30, 2022 00:00 Inactive

0x2b3bfa0 force-pushed the comment-publish-report branch from eaf33bc to 85374e4 Compare May 30, 2022 00:19

0x2b3bfa0 temporarily deployed to internal May 30, 2022 00:19 Inactive

0x2b3bfa0 force-pushed the comment-publish-report branch from 85374e4 to 88d15df Compare May 30, 2022 01:46

0x2b3bfa0 temporarily deployed to internal May 30, 2022 01:46 Inactive

Add watcher

869ad75

0x2b3bfa0 force-pushed the comment-publish-report branch from 88d15df to 869ad75 Compare May 30, 2022 02:06

0x2b3bfa0 temporarily deployed to internal May 30, 2022 02:06 Inactive

casperdcl mentioned this pull request May 30, 2022

epic: CML <> DVCLive #1036

Closed

7 tasks

casperdcl assigned 0x2b3bfa0 May 30, 2022

casperdcl added cml-publish Subcommand cml-comment Subcommand labels May 30, 2022

0x2b3bfa0 temporarily deployed to internal June 12, 2022 20:01 Inactive

dacbd previously approved these changes Jun 12, 2022

View reviewed changes

0x2b3bfa0 linked an issue Jun 13, 2022 that may be closed by this pull request

epic: CML <> DVCLive #1036

Closed

7 tasks

0x2b3bfa0 dismissed dacbd’s stale review via 763b4ff June 14, 2022 00:46

0x2b3bfa0 temporarily deployed to internal June 14, 2022 00:46 Inactive

Use older versions of packages to avoid ESM

484143d

0x2b3bfa0 force-pushed the comment-publish-report branch from 763b4ff to 484143d Compare June 14, 2022 00:50

0x2b3bfa0 temporarily deployed to internal June 14, 2022 00:50 Inactive

Merge branch 'master' into comment-publish-report

ca2621a

0x2b3bfa0 temporarily deployed to internal June 14, 2022 21:38 Inactive

Merge branch 'master' into comment-publish-report

303fe2a

DavidGOrtega temporarily deployed to internal June 15, 2022 18:21 Inactive

0x2b3bfa0 mentioned this pull request Jun 15, 2022

Command naming consistency #762

Closed

1 task

casperdcl reviewed Jun 16, 2022

View reviewed changes

bin/cml/send-comment.js Show resolved Hide resolved

casperdcl reviewed Jun 16, 2022

View reviewed changes

bin/cml/send-comment.js Show resolved Hide resolved

casperdcl reviewed Jun 16, 2022

View reviewed changes

bin/cml/send-comment.js Show resolved Hide resolved

casperdcl reviewed Jun 16, 2022

View reviewed changes

src/cml.js Show resolved Hide resolved

casperdcl suggested changes Jun 16, 2022

View reviewed changes

Merge branch 'master' into comment-publish-report

c0730c6

0x2b3bfa0 temporarily deployed to internal June 23, 2022 14:03 Inactive

casperdcl approved these changes Jun 23, 2022

View reviewed changes

casperdcl mentioned this pull request Jun 23, 2022

Introduce subcommands in a backwards-compatible way #1073

Merged

9 tasks

casperdcl merged commit 7bd9257 into master Jun 23, 2022

casperdcl deleted the comment-publish-report branch June 23, 2022 17:43

0x2b3bfa0 mentioned this pull request Jun 23, 2022

Improve the comment watermark format #1076

Closed

0x2b3bfa0 mentioned this pull request Sep 7, 2022

Document new command-line interface structure iterative/cml.dev#316

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate `cml publish` with `cml send-comment` #1026

Integrate `cml publish` with `cml send-comment` #1026

0x2b3bfa0 commented May 27, 2022 •

edited

Loading

casperdcl commented May 27, 2022

0x2b3bfa0 commented May 27, 2022 •

edited

Loading

0x2b3bfa0 commented May 27, 2022

casperdcl commented May 27, 2022 •

edited

Loading

0x2b3bfa0 commented May 27, 2022

0x2b3bfa0 commented May 27, 2022

0x2b3bfa0 commented May 30, 2022 •

edited

Loading

dacbd left a comment •

edited

Loading

0x2b3bfa0 commented Jun 14, 2022

casperdcl Jun 16, 2022

0x2b3bfa0 Jun 16, 2022

0x2b3bfa0 Jun 16, 2022

0x2b3bfa0 Jun 16, 2022

casperdcl Jun 20, 2022

casperdcl Jun 23, 2022 •

edited

Loading

0x2b3bfa0 Jun 23, 2022

0x2b3bfa0 Jun 23, 2022

casperdcl Jun 23, 2022

casperdcl left a comment

0x2b3bfa0 commented Jun 16, 2022

	description: 'File used to trigger the watcher',
	description: 'If specified, --watch will trigger only when the lockfile is present, and will delete the lockfile',

	description: 'File used to trigger the watcher',
	description: 'Path to the watcher trigger; create a file on that path to triger an update, then wait until the file disappears',

Integrate cml publish with cml send-comment #1026

Integrate cml publish with cml send-comment #1026

Conversation

0x2b3bfa0 commented May 27, 2022 • edited Loading

Usage

Behavior

Experimental

Using a trigger file

Pending

Reverted

Questions

Footnotes

casperdcl commented May 27, 2022

0x2b3bfa0 commented May 27, 2022 • edited Loading

Footnotes

0x2b3bfa0 commented May 27, 2022

casperdcl commented May 27, 2022 • edited Loading

0x2b3bfa0 commented May 27, 2022

0x2b3bfa0 commented May 27, 2022

0x2b3bfa0 commented May 30, 2022 • edited Loading

Footnotes

dacbd left a comment • edited Loading

Choose a reason for hiding this comment

0x2b3bfa0 commented Jun 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperdcl left a comment

Choose a reason for hiding this comment

0x2b3bfa0 commented Jun 16, 2022

Integrate `cml publish` with `cml send-comment` #1026

Integrate `cml publish` with `cml send-comment` #1026

0x2b3bfa0 commented May 27, 2022 •

edited

Loading

0x2b3bfa0 commented May 27, 2022 •

edited

Loading

casperdcl commented May 27, 2022 •

edited

Loading

0x2b3bfa0 commented May 30, 2022 •

edited

Loading

dacbd left a comment •

edited

Loading

casperdcl Jun 23, 2022 •

edited

Loading