Micro #67

DavidGOrtega · 2020-04-23T13:10:56Z

Introduces the new architecture.

Still to do before merge:

Remove previous unneeded code (can be done in a separate PR)
Improve docs (can be done in a separate PR)

I have uploaded an image with this work to be tested:
dvcorg/dvc-cml:micro

with this workflow:

name: CML

on: [push, pull_request]

jobs:
  cml:
    runs-on: [ubuntu-latest]
    container:
      image: docker://dvcorg/dvc-cml:micro

    steps:
      - uses: actions/checkout@v2

      - name: dvc_action_run
        shell: bash
        env:
          AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
          AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

        run: |
          cml-setup

          apt-get update && apt-get install -y python-pip && pip install --upgrade pip
          pip install tensorflow wget

          dvc pull -f
          dvc repro train.dvc
          dvc push

          echo "# CML report" >> report.md
          dvc metrics diff --show-json origin/master HEAD | cml-metrics >> report.md
          dvc diff --show-json origin/master HEAD | cml-files >> report.md

          cml-send-report -p report.md

DavidGOrtega · 2020-04-23T13:12:24Z

@dmpetrov please consider what to be done with the code removal and or docs.
I think that what is missing in docs is an explanation of every cml command.

dmpetrov

Great job @DavidGOrtega! This design is much better than the monolith one.
Comments are inline.

dmpetrov · 2020-04-24T08:18:01Z

README.md

-        test -f requirements.txt && pip3 install -r requirements.txt
-        # Run report:
-        dvc_cml_run
+        cml-setup


We agreed to extract this code to bash level.

dmpetrov · 2020-04-24T08:30:30Z

README.md

-        # Run report:
-        dvc_cml_run
+        cml-setup
+


please replace on: [push, pull_request] to one event. push is the best - we discussed this on slack.

dmpetrov · 2020-04-24T08:33:09Z

README.md

+        cml-setup
+
+        apt-get update && apt-get install -y python-pip && pip install --upgrade pip
+        pip install tensorflow wget


This should go through the requirements file
test -f requirements.txt && pip3 install -r requirements.txt

yeah, this is only my project

dmpetrov · 2020-04-24T08:33:55Z

README.md

-        dvc_cml_run
+        cml-setup
+
+        apt-get update && apt-get install -y python-pip && pip install --upgrade pip


Why not python3? py2 was deprecated already. We should force the best practices :)

its just my project, I have to update wiki with the new approach

dmpetrov · 2020-04-24T08:41:33Z

README.md

+        pip install tensorflow wget
+
+        dvc pull -f
+        dvc repro train.dvc


In the docs we should use the most general case. So, dvc repro is enough.

Not sure how, in the example it should be two steps train and eval. In fact in my example its wrong should be eval to run the whole pipeline. So the two steps pipeline should be train.dvc and Dvcfile?

dmpetrov · 2020-04-24T09:11:47Z

src/settings.js

 const DVC_TAG_PREFIX = '';
 const CI_SKIP_MESSAGE = '[ci skip]';
-const METRICS_FORMAT = '0[.][0000000]';
 const BASELINE = 'origin/master';
 const REPRO_TARGETS = ['Dvcfile'];


Not needed anymore

dmpetrov · 2020-04-24T09:11:58Z

src/settings.js

 const DVC_TAG_PREFIX = '';
 const CI_SKIP_MESSAGE = '[ci skip]';
-const METRICS_FORMAT = '0[.][0000000]';
 const BASELINE = 'origin/master';


To the script level

dmpetrov · 2020-04-24T09:12:17Z

src/settings.js

 const DVC_TAG_PREFIX = '';
 const CI_SKIP_MESSAGE = '[ci skip]';
-const METRICS_FORMAT = '0[.][0000000]';


CI_SKIP_MESSAGE and METRICS_FORMAT - to the command option

we are not dooing skip anymore right?

dmpetrov · 2020-04-24T09:12:38Z

src/settings.js

 const DVC_TAG_PREFIX = '';
 const CI_SKIP_MESSAGE = '[ci skip]';
-const METRICS_FORMAT = '0[.][0000000]';
 const BASELINE = 'origin/master';
 const REPRO_TARGETS = ['Dvcfile'];
 const A_REV = BASELINE;
 const B_REV = 'HEAD';
 const INPUT_SKIP = '-';


A_REV, B_REV, INPUT_SKIP - all on the script level

dmpetrov · 2020-04-24T09:12:53Z

src/settings.js

 const DVC_TAG_PREFIX = '';
 const CI_SKIP_MESSAGE = '[ci skip]';
-const METRICS_FORMAT = '0[.][0000000]';
 const BASELINE = 'origin/master';
 const REPRO_TARGETS = ['Dvcfile'];
 const A_REV = BASELINE;
 const B_REV = 'HEAD';
 const INPUT_SKIP = '-';

 exports.DVC_TITLE = DVC_TITLE;


Script level

DavidGOrtega · 2020-04-24T09:57:20Z

Settings is actually not used by the new architecture but the old one

dmpetrov · 2020-04-24T10:56:21Z

Settings is actually not used by the new architecture but the old one

Right. This is why we need to remove it.

DavidGOrtega · 2020-04-26T12:42:45Z

@dmpetrov Im going to remove the dvc-cml code in another PR if not this PR would be huge.

I have included the send report as comment in gitlab since tags were broken. Not broken but you needed to do lots of stuff manually that differs a lot with GH.

The work can be tested with the container at:
dvcorg/dvc-cml:micro

dmpetrov

Good improvements on the comment and cleaning code. But the major issues still need to be addressed.

dmpetrov · 2020-04-26T12:55:30Z

src/cml.js

@@ -1,17 +1,17 @@
 const print = console.log;
-// console.log = console.error;


Commented code? Please remove all the commented code without a good reason.

This is outdated... The code that I committed don't have it, maybe an issue in GH UI?

dmpetrov · 2020-04-26T16:13:11Z

README.md

-        dvc_cml_run
+
+        # needed to be able to do dvc metrics and dvc diff
+        git fetch --prune --unshallow


Can we make it a part of actions/checkout@v2? I hope it supports all these options.

We could do as stated by them:

- uses: actions/checkout@v2 - run: | git fetch --prune --unshallow

but I think is easier to loose the track. What do you think? should I move it?

I found it in their docs. yep, it looks good, let's keep it.

I have understood to keep it in our side and not put it in the checkout action. Also one point to remember is the gitlab similarity. GL does not have that action

dmpetrov · 2020-04-26T16:14:13Z

README.md

+        dvc push
+
+        echo "# CML report" >> report.md
+        dvc metrics diff --show-json origin/master HEAD | cml-metrics >> report.md


Is there a reason to not to extract BASELINE as a variable?

dmpetrov · 2020-04-27T05:52:08Z

bin/cml-files.js

+
+const { metrics_args, diff_run, error_handler } = require('../src/cml');
+const opts = metrics_args();
+diff_run(opts).catch(e => error_handler(e));


All the code needs to be extracted from scm/cml (except common libs).
Each command is supposed to be as independent as possible. It should simplify command replacement for users.

dmpetrov · 2020-04-27T05:52:53Z

bin/cml-metrics.js

+
+const { metrics_args, metrics_diff_run, error_handler } = require('../src/cml');
+const opts = metrics_args();
+metrics_diff_run(opts).catch(e => error_handler(e));


Same. Please move all then needed code from src/cml to here.

dmpetrov · 2020-04-27T05:53:02Z

bin/cml-send-report.js

+} = require('../src/cml');
+
+const opts = send_report_args();
+send_report_run(opts).catch(e => error_handler(e));


dmpetrov · 2020-04-27T05:53:14Z

bin/cml-setup-env-remote.js

@@ -0,0 +1,5 @@
+#!/usr/bin/env node
+
+const { setup_env_remote, error_handler } = require('../src/cml');


dmpetrov · 2020-04-27T05:55:54Z

src/pipe-args.js

@@ -0,0 +1,36 @@
+const fs = require('fs');


I still don't understand why do we need this module. We should just read stdin.

read the buffer, stdin buffer

why do you need a buffer? you can read everything at once. if it fails - totally fine since our limit is way below any machine memory restriction.

I assume there is a 1-3 line of JS code that can do this.

I assume there is a 1-3 line of JS code that can do this.

No, I dont think so, code was much simpler before but with DVC output breaks. I can try a library like https://www.npmjs.com/package/get-stdin
but the code is prettty similar to mine using buffer

ok. let's keep it for now

DavidGOrtega · 2020-04-27T09:14:00Z

Is there a reason to not to extract BASELINE as a variable?

Whats the benefit? You are setting it on there or in the variable all that its in the yaml file. I dont see the difference and its worse since we handle code that its not needed. Maybe Im losing the point

dmpetrov · 2020-04-27T09:29:24Z

Whats the benefit?

The point:

Many commands use the same baseline - avoid code duplication.
This additional line of code will serve as documentation.

DavidGOrtega · 2020-04-27T10:00:02Z

OK,

in the bash or in the ENV?

BASELINE="origin/master"
dvc metrics diff --show-json "$BASELINE" HEAD

dmpetrov

Great change @DavidGOrtega ✨
One question inline - please answer

dmpetrov · 2020-04-27T10:24:13Z

README.md

-        dvc metrics diff --show-json origin/master HEAD | cml-metrics >> report.md
-        dvc diff --show-json origin/master HEAD | cml-files >> report.md
+        dvc metrics diff --show-json "$BASELINE" HEAD | cml-metrics >> report.md
+        dvc diff --show-json "$BASELINE" HEAD | cml-files >> report.md


Do you need to compare it to HEAD? It means the results need to be committed.
Based on my understanding it should look like dvc diff --show-json "$BASELINE" | ....
Could you please make sure which approach is correct.

I will check

you are right

DavidGOrtega added 7 commits April 23, 2020 14:07

code and partial docs

b06573a

fix test

c84a86a

fix test

d5923c1

no double check

02e4912

Token

3230afe

Token

b409b63

-ci

7be6340

DavidGOrtega requested a review from dmpetrov April 23, 2020 13:12

DavidGOrtega added 3 commits April 23, 2020 18:20

no metrics

73b5ac2

revert entrypoint

50e2946

rm test files

a248a9f

dmpetrov requested changes Apr 24, 2020

View reviewed changes

DavidGOrtega force-pushed the micro branch from 5db35c3 to dc3f9f1 Compare April 26, 2020 10:50

Gitlab report send comment

bc99604

DavidGOrtega force-pushed the micro branch from dc3f9f1 to bc99604 Compare April 26, 2020 10:51

DavidGOrtega added 2 commits April 26, 2020 14:19

PR amends

d42b519

fix test

9e17a1f

DavidGOrtega added 2 commits April 26, 2020 16:14

remove dvc-cml

00442a1

no entrypoint

7bea263

dmpetrov requested changes Apr 27, 2020

View reviewed changes

decoupled cml

f32a5d6

dmpetrov approved these changes Apr 27, 2020

View reviewed changes

HEAD rev

f3d54bf

DavidGOrtega merged commit a184690 into master Apr 27, 2020

DavidGOrtega deleted the micro branch April 27, 2020 12:30

0x2b3bfa0 mentioned this pull request Sep 22, 2021

Retry pipe EAGAIN on macOS #732

Merged

0x2b3bfa0 mentioned this pull request Apr 21, 2022

Remove standard input detection from cml publish #983

Merged

casperdcl mentioned this pull request May 11, 2022

fix cml publish stdin pipe #1000

Closed

		@@ -1,17 +1,17 @@
		const print = console.log;
		// console.log = console.error;

		@@ -0,0 +1,5 @@
		#!/usr/bin/env node

		const { setup_env_remote, error_handler } = require('../src/cml');

Micro #67

Micro #67

Conversation

DavidGOrtega commented Apr 23, 2020

DavidGOrtega commented Apr 23, 2020

dmpetrov left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGOrtega commented Apr 24, 2020

dmpetrov commented Apr 24, 2020

DavidGOrtega commented Apr 26, 2020

dmpetrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGOrtega Apr 27, 2020 • 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

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

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

Choose a reason for hiding this comment

DavidGOrtega commented Apr 27, 2020 • edited Loading

dmpetrov commented Apr 27, 2020

DavidGOrtega commented Apr 27, 2020

dmpetrov left a 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

DavidGOrtega Apr 27, 2020 •

edited

Loading

DavidGOrtega commented Apr 27, 2020 •

edited

Loading