Callback stage not exectued if params defined in dvc.yaml

[jhoecker]

Bug Report

Callback stages (defined by not existing dependencies as well as by dvc run --always-changed option) are not executed if parameters were defined in the dvc.yaml but restored from the run-cache and skipped.

Minimal example:

$ git init
$ dvc init
$ dvc run -n foo -o a.txt "echo 'a' > a.txt"
Running callback stage 'foo' with command:
     echo 'a' > a.txt
$ dvc repro
Running callback stage 'foo' with command:
     echo 'a' > a.txt
$ # Create params.yaml and append params section to dvc.yaml
$ echo -e "foo:\n bar: 2" > params.yaml
$ echo -e "    params:\n    - foo.bar" >> dvc.yaml

$ # Try to reproduce build with callback stage but stage is not a callback stage anymore
$ dvc repro
Running stage 'foo' with command:
    echo 'a' > a.txt
$ dvc repro
Stage 'foo' didn't change, skipping.

$ # Try to force execution by adding always_changed
$ echo '    always_changed: true' >> dvc.yaml
$ dvc repro
Restored stage 'foo' from run-cache
Skipping run, checking out outputs.

This leads from my point of view to quite confusing and inconsistent behavior. If this is the desired behavior then it should be mentioned in the documentation.
I would like to define the names of data folders and files in the params.yaml which are updated from the www every time I kick of the pipeline. Thus I would like to use the params in combination with a callback stage.

Please provide information about your setup

Tested on Win10 as well as on Raspbian.
Output of dvc version:

$ dvc version
DVC version 1.3.1 (pip)
-----------------------------
Platform: Python 3.7.3. on Linux
Cache types: hardlink, symlink
Repo: dvc, git

[pared]

Reproduction script with always_changed:

#!/bin/bash

rm -rf repo
mkdir repo

pushd repo

set -ex

git init --quiet
dvc init --quiet

echo -e "foo:\n bar: 2" > params.yaml

dvc run -p params.yaml:foo.bar -n cp -o a.txt --always-changed "echo 'a' > a.txt"

# not reproduced
dvc repro cp

So I guess the question here is whether we want run cache to have priority over always_changed or the other way around.

@jhoecker Workaround, for now, would be to dvc repro -f

[skshetry]

@pared, regarding these, we have to ask ourselves two questions:

1. Should we restore "always_changed" stages?
2. Should we save run-cache for "always_changed" stages?

As per docs, "always_changed" stages should be always executed, so 1's answer should probably be no.
For 2, I am not quite sure, but probably we should not save it. Consider this scenario:

1. Runs "always_changed" stages.
2. Removes "always_changed".
3. Runs repro.

What should happen? Should it be re-executed? Or, should it be reproduced from the run-cache?

[jhoecker]

@pared Yes, I tried that yesterday. But that reproduces the full pipeline even though there are no changes in the downloaded data making subsequent stages unnecessary. Thinking about this a bit longer it is probably the right way to drop the params section in the respective pipeline stage which is superfluous since this only indicates DVC to look for changes but the stage should be carried out anyway. Nevertheless I still think from the user and documentation perspective like @pared that the explicit option always_changed: true should force an execution and overwrite the quite implicit change of the stages callback "character".

However: Thank you for your fast responses and the nice piece of software 👍

[pared]

@jhoecker that makes sense. So that seems like a bug.

@skshetry

1. I agree
2. It makes sense to me that in the described use case result should be obtained from the run cache (since its not always_changed anymore) But I also think that we should not keep always_changed run execution results in run cache, just for this particular use case. If the stage is truly always_changed we will produce a lot of garbage in run cache that will never be used. In the case of always-changed, you have to explicitly state it, so you probably will not want to roll back from that decision.

[efiop]

Update: currently (as of 1.8.2) we don't restore always_changed stages, but we still save them to the run-cache.

[efiop]

Saving is an interesting topic, potentially it could be useful for things that are importing stuff in some way. E.g. an always_changed stage that tries to dump a remote database (or download some dataset from an external source) and succeeds once, but you don't commit .lock file and so if that external resource becomes unavailable in the future, you won't be able to get anything, unless you have run-cache to have your back. But at the same time I agree that in many cases it might be just wasteful to save such runs.

Afterall, seems like we indeed shouldn't save always_changed to run-cache by default. There might be a potential for some explicit option to force us to do so, but we'll need someone to ask for it first. I'll send a patch to stop saving run-cache for always_changed soon.

Thank you guys for a great discussion! 🙏