dvc.yaml: add matrix-do

[skshetry]

Introduces matrix:do, fixes #5172.

stages:
  stage1:
    matrix:
      os: [windows, linux, macos]
      pyv: ["3.9", "3.10", "3.11"]
      dict:
      - arg1: 1
      - arg2: 2
      lst:
      - [1, 2]
      - [3, 4]
    cmd: echo ${item.os} ${item.pyv} ${item.dict} ${item.lst.0} ${item.lst.1}

DVC will build the name for the generated stages based on their variables.
If the variables are numbers, strings, booleans, etc, they will use their string representation, and for others, dvc will use the variable name and index.

For example, for above, os and pyv will use their respective values, whereas dict/lst will use their variable name plus index. For example, one of the instance will look like follows;

stage1@windows-3.9-dict0-lst0

This name will be it's address (i.e. how you invoke it in dvc repro and other places), and it's identity. Like foreach:do, you can invoke the whole group with just dvc repro stage1 as well.

Example output of above

dvc stage list --all
stage1@windows-3.9-dict0-lst0   No outputs or dependencies
stage1@windows-3.9-dict0-lst1   No outputs or dependencies
stage1@windows-3.9-dict1-lst0   No outputs or dependencies
stage1@windows-3.9-dict1-lst1   No outputs or dependencies
stage1@windows-3.10-dict0-lst0  No outputs or dependencies
stage1@windows-3.10-dict0-lst1  No outputs or dependencies
stage1@windows-3.10-dict1-lst0  No outputs or dependencies
stage1@windows-3.10-dict1-lst1  No outputs or dependencies
stage1@windows-3.11-dict0-lst0  No outputs or dependencies
stage1@windows-3.11-dict0-lst1  No outputs or dependencies
stage1@windows-3.11-dict1-lst0  No outputs or dependencies
stage1@windows-3.11-dict1-lst1  No outputs or dependencies
stage1@linux-3.9-dict0-lst0     No outputs or dependencies
stage1@linux-3.9-dict0-lst1     No outputs or dependencies
stage1@linux-3.9-dict1-lst0     No outputs or dependencies
stage1@linux-3.9-dict1-lst1     No outputs or dependencies
stage1@linux-3.10-dict0-lst0    No outputs or dependencies
stage1@linux-3.10-dict0-lst1    No outputs or dependencies
stage1@linux-3.10-dict1-lst0    No outputs or dependencies
stage1@linux-3.10-dict1-lst1    No outputs or dependencies
stage1@linux-3.11-dict0-lst0    No outputs or dependencies
stage1@linux-3.11-dict0-lst1    No outputs or dependencies
stage1@linux-3.11-dict1-lst0    No outputs or dependencies
stage1@linux-3.11-dict1-lst1    No outputs or dependencies
stage1@macos-3.9-dict0-lst0     No outputs or dependencies
stage1@macos-3.9-dict0-lst1     No outputs or dependencies
stage1@macos-3.9-dict1-lst0     No outputs or dependencies
stage1@macos-3.9-dict1-lst1     No outputs or dependencies
stage1@macos-3.10-dict0-lst0    No outputs or dependencies
stage1@macos-3.10-dict0-lst1    No outputs or dependencies
stage1@macos-3.10-dict1-lst0    No outputs or dependencies
stage1@macos-3.10-dict1-lst1    No outputs or dependencies
stage1@macos-3.11-dict0-lst0    No outputs or dependencies
stage1@macos-3.11-dict0-lst1    No outputs or dependencies
stage1@macos-3.11-dict1-lst0    No outputs or dependencies
stage1@macos-3.11-dict1-lst1    No outputs or dependencies

[codecov]

Codecov Report

Patch coverage: 91.57% and project coverage change: -0.01% ⚠️

Comparison is base (bc86fa0) 90.45% compared to head (bf6b0c2) 90.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9725      +/-   ##
==========================================
- Coverage   90.45%   90.45%   -0.01%     
==========================================
  Files         482      483       +1     
  Lines       36719    36797      +78     
  Branches     5299     5309      +10     
==========================================
+ Hits        33213    33283      +70     
- Misses       2900     2907       +7     
- Partials      606      607       +1     

Files Changed	Coverage Δ
dvc/parsing/__init__.py	96.72% <90.00%> (-2.44%)	⬇️
dvc/repo/stage.py	94.44% <100.00%> (ø)
dvc/schema.py	100.00% <100.00%> (ø)
dvc/stage/loader.py	97.56% <100.00%> (ø)
tests/func/parsing/test_matrix.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dberenbaum]

Do you think it makes sense to keep foreach or should this eventually replace it?

[skshetry]

Do you think it makes sense to keep foreach or should this eventually replace it?

We did not implement matrix initially, because we thought foreach..do would be more intuitive to users, and simple compared to matrix or even nested foreach..do.

I am not sure if a lot has changed in the past 2 years. Maybe GitHub Actions has standardized the syntax/concepts now? So I am not sure about foreach in the short term. But in the future, if the users don't have difficulty picking it up, we can think of deprecating foreach.

[dberenbaum]

Thanks @skshetry! Looks really nice, and appreciate the example. It might be good to also include an example of accessing subitems like model below (just a thought for docs):

stages:
  stage1:
    matrix:
      os: [windows, linux, macos]
      pyv: ["3.9", "3.10", "3.11"]
      dict:
      - arg1: 1
      - arg2: 2
      lst:
      - [1, 2]
      - [3, 4]
      model:
      - epochs: 3
        thresh: 10
      - epochs: 10
        thresh: 15
    do:
      cmd: echo ${item.os} ${item.pyv} ${item.dict} ${item.lst.0} ${item.lst.1} --model epochs=${item.model.epochs},thresh=${item.model.thresh}

[dberenbaum]

We did not implement matrix initially, because we thought foreach..do would be more intuitive to users, and simple compared to matrix or even nested foreach..do.

My feeling is that having both will be more complex and confusing than asking users to learn matrix. The simple case of foreach that accepts a list instead means two different ways to use it, and I would rather just have a single way of doing things (matrix that expects a dict).

[daavoo]

We did not implement matrix initially, because we thought foreach..do would be more intuitive to users, and simple compared to matrix or even nested foreach..do.

My feeling is that having both will be more complex and confusing than asking users to learn matrix. The simple case of foreach that accepts a list instead means two different ways to use it, and I would rather just have a single way of doing things (matrix that expects a dict).

I would personally prefer to only document matrix in the docs once this is merged

[skshetry]

I would prefer waiting a couple of months before we do that. If we do want to promote matrix, and deprecate foreach soon, let's scrutinize the syntax a bit more, as we don't really need to do things the way foreach does?

1. Should matrix inject item variable? Or, should it be named matrix? Or, should variables be directly addressed? i.e. ${os}, etc.
2. We did not know anything better than foreach .. do (we started with foreach .. in which I still prefer which binds variables in-side a block). Is there a better structure than this that we can do for matrix?

Maybe the current way is fine, but I do want to touch on these issues and discuss explicitly.

[dberenbaum]

Great points, thanks for raising them.

1. Or, should variables be directly addressed? i.e. ${os}, etc.

Since matrix requires a dict, I like the simplicity of this, although we would need to handle conflicts with other vars.

${matrix.os} follows GitHub but GitLab directly addresses the variables, and I don't think GitHub Actions syntax is so pervasive that we need to follow it if there's a simpler option available.

2. We did not know anything better than foreach .. do (we started with foreach .. in which I still prefer which binds variables in-side a block). Is there a better structure than this that we can do for matrix?

The simplest solution would be to get rid of the do/in block altogether like:

stages:
  stage1:
    matrix:
      os: [windows, linux, macos]
      pyv: ["3.9", "3.10", "3.11"]
      dict:
      - arg1: 1
      - arg2: 2
      lst:
      - [1, 2]
      - [3, 4]
    cmd: echo ${item.os} ${item.pyv} ${item.dict} ${item.lst.0} ${item.lst.1}

[sjawhar]

Wanted to chime in with a random thought that came to me just now. Imagine something like this:

# params.yaml
foo:
  bar: [a,b,c,d,e]
  baz: [1,2,3]

# dvc.yaml
stages:
  foo_bar:
    matrix:
      first: ${foo.bar}
      second: [x, y, z]
    <<: &stage_foo
      cmd: ...
      ...

  foo_baz:
    matrix:
      first: ${foo.baz}
      second: [x, y, z]
    <<: *stage_foo

Could there would be a way to specify such a "ragged matrix" as a single foo stage, where different slices have different numbers of elements? i.e. the full list of stages would be

• foo@firsta-secondx
• foo@firsta-secondy
• foo@firsta-secondz
• foo@firstb-secondx
• foo@firstb-secondy
• ...
• foo@firste-secondz
• foo@first1-secondx
• foo@first1-secondy
• foo@first1-secondz
• foo@first2-secondx
• ...
• foo@first3-secondz

[dberenbaum]

Hey @sjawhar, I think it's not possible even with this PR because you want to treat foo.bar and foo.baz like a single concatenated list, right? It's outside the scope of this PR, but it is something that's been discussed in #7151 and #9148.

[sjawhar]

Good point that this is essentially just concatenating the two lists. I wonder if it could be accomplished using hydra.

[daavoo]

Q) Is dict unpacking expected/supposed to work here (i.e. echo ${item])?

[skshetry]

yes

[daavoo]

not a blocker, I think would be good to add a test for that

[dberenbaum]

@skshetry Are you okay with the suggestions in #9725 (comment)? Once we agree that, it's ready from a product perspective.

[skshetry]

On both variable addressing, and matrix..do block, one advantage that I see with what you proposed, is that it makes it easy to convert a templated stage to a matrix.

Since matrix requires a dict, I like the simplicity of this, although we would need to handle conflicts with other vars.

item or matrix variable logically groups them, and avoids conflicts. Not only that, I think this logical grouping may come useful in the future if we ever implement env. and secrets. like namespaces. They are not so that complicated, either. If we implement directly addressing variables, we'd probably spend more time explaining "shadowing of variables".

I think the arguments are weak on both sides, but the grouping under item is a bit safer, so I am leaning towards it unless I hear more (strong) arguments for or against this.

The simplest solution would be to get rid of the do/in block altogether like:

matrix..do makes it a bit clear on what is a data source and what is being templated.
Does that structure help? I don't have a strong opinion here, though. Syntactically, we are also enforcing order, as otherwise, the flat map in yaml can be defined in any order.

On both of these issues, I don't have a strong opinion. But I do want to hear pros and cons of those changes before deciding on this.

[dberenbaum]

Thanks @skshetry, agreed neither are true blockers, so I'm approving.

On both variable addressing, and matrix..do block, one advantage that I see with what you proposed, is that it makes it easy to convert a templated stage to a matrix.

Yes, I think it's enough reason to at least drop do since it saves you from having to make significant modifications to even a non-templated stage.

[daavoo]

I think the arguments are weak on both sides, but the grouping under item is a bit safer, so I am leaning towards it unless I hear more (strong) arguments for or against this.

Maybe is just a plus for me, but on the side of grouping I like the possibility of using dict unpacking on item. Allows me to write less verbose cmd and to update the matrix entry without having to update cmd

[dberenbaum]

@daavoo I assume you would be fine with either item or matrix for that purpose?

[daavoo]

@daavoo I assume you would be fine with either item or matrix for that purpose?

yes

[skshetry]

One argument in favour of matrix is symmetry.

matrix:
  os: [windows, linux, macos]
  pyv: ["3.9", "3.10", "3.11"]

You access os variables with matrix.os.

Is that enough of a reason to move away from item and make it confusing with foreach? That's something I am not quite sure of.

[daavoo]

Let's agree on the name/syntax (no strong opinion from me) and remember to update https://github.com/iterative/dvcyaml-schema

cc @iterative/vs-code for awareness of something new worth adding to snippets

[dberenbaum]

Personally I always found item hard to remember since it's completely unrelated to foreach.

[dberenbaum]

Dropping do also significantly simplifies the schema. Snippets and suggestions will be easier to follow.

[mattseddon]

I think the simplest thing to do in VS Code is to drop the dvc-pipeline-foreach-stage snippet and not add a dvc-pipeline-matrix-stage one. This is because the new matrix syntax will only be supported for the most recent versions of DVC.

I think we can do this because foreach/matrix is a more advanced usage which people can find through the docs.

We can add the new snippet once some more time has passed. LMK what you think.

[skshetry]

I'll leave this up for comments for a few days, and will merge by the end of this week.

[dberenbaum]

Thanks @skshetry!