QA: params/vars and loops in dvc.yaml (2.0) round 2

[jorgeorpinel]

Continues #4854

Bugs?

• dvc repro executes stage groups alphabetically e,g, if my foreach list is foo, bar, baz (in that order) still the stages gets reproed (and saved to dvc.locl) in this order: stg@bar, stg@baz, stg@foo.
  Generalized and extracted to repro: fallback order of execution for independent stages #5181

UI/UX

• Foreach loops: since do: always contains all the regular stage fields, what about avoiding do and go straight to cmd, etc.? Like this:

  stages:
    something:
      foreach: ...
      cmd: python script.py --thresh ${item...}
      deps: ...

  This way the only syntax change is to add the foreach field, which makes the whole entry into a stage group.

• If a directory path in vars, repro prints a generic ERROR: unexpected error - [Errno 21] Is a directory: '{path}'. which while informative enough, seems like it could be better handled. (Fix on parametrization: fix error when the file in vars is a directory #5171)

Feature design/ Side-effects

• There seems to be no way to indicate you don't want any params file to be loaded (even if it exists for other purposes). Earlier on you could use: none.
• Similarly, there's no way to ONLY use an alternative params file e.g. params.json and avoid the default (params.yaml). I feel like the default behavior should be to skip params.yaml if any params files are specified.
• vars cannot overwrite any values in params files, but params files accept repeated objects IF they can be merged without conflict. E.g.: params.json {"grp": {"a": 1}} params2.json {"grp": {"b": 2}} can both be included even when grp is overloaded, but you can't define grp at all in vars.

[skshetry]

dvc repro executes stage groups alphabetically e,g, if my foreach list is foo, bar, baz (in that order) still the stages gets reproed (and saved to dvc.locl) in this order: stg@bar, stg@baz, stg@foo

Not a bug. The ordering is based on the DAG. Other than that, it might be difficult to promise to order (and, not that it matters). Lock file has its own way of ordering things wherever it matters, it does not have to work the way you expect.

Foreach loops: since do: always contains all the regular stage fields, what about avoiding do and go straight to cmd, etc.?

The reasoning for this is that we have two levels of vars: local and global. Three if you count the foreach's key and value, but they come from global vars most of the time.

do helps them separate. Not in a clean way that I'd like, as wdir depends on global variables rather than local variables as mentioned in treeverse/dvc.org#2052 (comment).

... the whole entry into a stage group.

I don't like this term at all. They are not a group in any way. If you are confusing it with dvc repro <foreach_name>, then that's purely a convenience function.

There seems to be no way to indicate you don't want any params file to be loaded (even if it exists for other purposes). (Earlier on you could use: none.)

I brought that in the discussion with @dmpetrov. The main reasoning was that most people will use params.yaml by default, and if they do use other files, that either means they use separate files like params.json and don't use params.yaml or they do use params.yaml for common configs but use other files for specialized params.

And, if you don't want any parameterization, don't use it at all. If you only want to declare variables in vars section only, then name it a bit differently so they don't collide.

vars cannot overwrite any values in params files, but params files accept repeated objects

vars has the same behaviour whether you use files or declare it in vars itself. It never allows you to overwrite, you can only recursively load them. So, the following should work in vars:

vars:
  grp:
    b: 3

This is useful in the top-level vars, but rarely useful in the stage-level vars (where, key: value is a better choice).

We have decided that if someone wants to have an "overwrite" method, we'd introduce a new keyword as follows in the future:

overwrite_vars: true
vars:
  grp: 3

[jorgeorpinel]

The ordering is based on the DAG

So if there are no DAG connections in a stage group, why use any other order than what is specified in dvc.yaml? We even assign 0...n indices to some lists given to foreach. That seems much more intuitive IMO but @efiop @shcheklein can you please weight in?

it does not have to work the way you expect.

DVC should not work in a predictable/intuitive way? I probably didn't understand what you meant with that @skshetry.

they come from global vars most of the time

I think we are assuming there. OR do we know that users actually prefer global vs. local vars?

do: helps them separate

I don't have a strong opinion on this since it's already implemented that way but I think it looks cleaner without do:, and the change vs. regular stages is minimal: you just add the foreach: field to the spec (it could even be called something else like set:). Again, something for others to weight in, hopefully.

They are not a group in any way

https://www.merriam-webster.com/dictionary/group

I see you fixed the "repro prints a generic ERROR" bug, great! 👍

no way to indicate you don't want any params file

most people will use params.yaml... if you don't want any parameterization, don't use it at all... If you only want to declare variables in vars section only, then name it a bit differently

No strong opinion either but those are workarounds. What if I want parameterization with vars with the same names as what is in params.yaml? Since it doesn't let me overwrite them, use: none was useful for that (I'm not saying that exact syntax should be brought back). Up to you guys, I'm just leaving all the "QA" results here for consideation.

no way to ONLY use an alternative params file e.g. params.json... (Maybe) the default behavior should be to skip params.yaml if any params files are specified?

Is there a reasoning to keep that default behavior? Thanks

vars has the same behaviour whether you use files or declare it in vars itself

You're right! I confirmed this does work consistently 👍 (except that grp in your example should be a - list item).

[shcheklein]

why use any other order than what is specified in dvc.yaml?

any additional guarantee reduces flexibility (let's say we decide to run them in parallel at some point and people already expect certain order). I would wait for someone to ask this, for a specific use case to start considering this at all.

but I think it looks cleaner without do:

I think @skshetry mentioned that do is needed. Not sure what do you suggest in terms of avoiding it?

They are not a group in any way

so, according to that screenshot I tend to agree with @skshetry - since they do not represent a unit in any way

[jorgeorpinel]

any additional guarantee reduces flexibility (let's say we decide to run them in parallel

@shcheklein Let's use a simple example to make sure we're talking about the same thing:

stages:
  notagroup:
    foreach:
    - 1
    - 2
    - 3
    cmd: echo ${item}

(BTW notice I also used my suggested syntax without do: above (previously mentioned in treeverse/dvc.org#2052 (comment)). I don't see why it's needed but again, up to you guys. In fact I've already documented it with do.)

So you're saying maybe we'll introduce some new field signaling to run them in parallel? OK that's fine, in that case you would be specifying an order too. My point is that if there's no way to build a DAG nor an order gets specified, the intuitive behavior would be to use the explicit order in which dvc.yaml is written.

Why does that reduce flexibility? It's just a predictable outcome. At least I'd like to know what that order is because it's not random. In fact I've noticed that changing the command or other parts of regular stages (not linked by DAGs) can result in a different execution order — so this seems to be an existing/general situation BTW: we can def. extract it from here if needed.

[shcheklein]

@jorgeorpinel

I don't see why it's needed

hmm ... what about the explanation @skshetry provided, with global/local vars?

So you're saying maybe we'll introduce some new field signaling to run them in parallel?

no. We might run them in parallel by default, nothing prevents us from doing this at the moment.

My point is that if there's no way to build a DAG nor an order gets specified ...

I'm not sure I understand this to be honest. Could you clarify please?

Why does that reduce flexibility?

you kinda set specific thing (execution order in this case). This is by definition reduces flexibility - we have to maintain that specification. E.g. it makes harder to run stuff in parallel by default.

In fact I've noticed that changing the command or other parts of regular stages

Yes. We don't guarantee any order.