search

iterative / setup-cml

GitHub Action for CML setup
25 stars 12 forks source link

Old vega version - causes conflict with certain plot ecodings #75

Closed tibor-mach closed 2 years ago

tibor-mach commented 2 years ago

https://github.com/iterative/setup-cml/blob/7eecee79650503f8624b7012b89d27bef4f7de60/src/utils.js#L51

vega-lite version should be bumped to 5 (in fact 5.2.0 is necessary). DVC has recently updated plots to use this vega version and some types of plots cannot be rendered correctly otherwise.

For more details for why this causes problems see this comment in a related issue

dacbd commented 2 years ago

Related:

dacbd commented 2 years ago

@0x2b3bfa0 am I reading the history correctly that v5 was not used because of the ?? operator requiring node >14 ? I don't believe this is a concern anymore?

0x2b3bfa0 commented 2 years ago

I don't believe this is a concern anymore?

0x2b3bfa0 commented 2 years ago

Enough of a concern for actions/checkout to bump major version when requiring Node.js 16+

dacbd commented 2 years ago

Enough of a concern for actions/checkout to bump major version when requiring Node.js 16+

Not the only change but sure, also does the actions runner not package these run times? so with an action.yml using: node12 and using: node16 are the same as running a cmd or script that uses the machines node runtime.

I don't believe this is a concern anymore?

and we have since pinned/specified the engine information?

Part of my question with all of the above links is: Is there an environment that we can reasonably expect to encounter that is using node12 over node16? From what I can tell the answer is no.

tibor-mach commented 2 years ago

I am not sure what the cause of this is, but clearly vega4 (or a version lower than 5.2.0) is used when CML is set up with this action.

With CML (on a standard GitHub runner) I get the following as the output of dvc plots diff for the specific bar plot

image

And locally (importantly without CML) the result looks like this (which is desired). image

This is what older vega versions do since they do not have the encoding option available which shifts the bars. It used to be an issue with DVC as well but has since been fixed. I am not sure why the CML setup action enforces the older vega version, but somehow it does.

For reproducibility this is the workflow that I run (default GitHub runner):

name: Pull request
on: [pull_request]
env:
  REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
  AZURE_STORAGE_CONNECTION_STRING: ${{ secrets.AZURE_STORAGE_CONNECTION_STRING }}
  AZURE_STORAGE_CONTAINER_NAME: ${{ secrets.AZURE_STORAGE_CONTAINER_NAME }}
jobs:
  reproduce-and-report:
    runs-on: ubuntu-20.04
    steps:
      - uses: actions/checkout@v3
        with:
          fetch-depth: 0
      - uses: iterative/setup-cml@v1
      - uses: actions/setup-python@v4
        with:
          python-version: '3.8'
          cache: 'pip'
      - run: pip install -r requirements.txt
      - name: DVC repro and CML report
        run: |
          dvc pull --run-cache
          dvc repro -P
          source .github/workflows/cml_report.sh

and the only relevant part of requirements.txt is dvc[azure]==2.12.0.

The shell script is basically this (there is also a loop over multiple targets etc. but that is not important):

            dvc plots diff $GITHUB_BASE_REF $GITHUB_HEAD_REF \
                --target mytargetfile.csv \
                --show-vega > vega.json
            vl2svg vega.json > plot.svg
            cml publish --md plot.svg >> report.md

cml send-github-check report.md

The template for the plot is bar_horizontal_sorted merged here

Hope this helps.

dacbd commented 2 years ago

@tibor-mach would you try - uses: iterative/setup-cml@update-vega-lite and let me know if your plot is generated correctly?

tibor-mach commented 2 years ago

@dacbd Yep, that seems to have done the trick!

dacbd commented 2 years ago

@tibor-mach you can continue to use that branch in your workflow, when the PR gets merged that branch will be deleted but I can keep it around so we have minimal chances of breaking your workflows. I'll keep you updated here.

tibor-mach commented 2 years ago

@dacbd thanks!

dacbd commented 2 years ago

@tibor-mach updated, give me a 👍 when you have your workflows updated back to v1 and are working so I can delete the merged branch.

tibor-mach commented 2 years ago

@dacbd :+1: You can delete the branch.