params/metrics: cache remote files

[skshetry]

See iterative/dvc.org#4851 (comment).

[codecov]

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed	Coverage
dvc/dependency/param.py	100.00%
dvc/repo/metrics/show.py	100.00%
dvc/repo/params/show.py	100.00%
dvc/utils/serialize/__init__.py	100.00%
dvc/utils/serialize/_common.py	100.00%
dvc/utils/serialize/_json.py	100.00%
dvc/utils/serialize/_py.py	100.00%
dvc/utils/serialize/_toml.py	100.00%
dvc/utils/serialize/_yaml.py	100.00%

📢 Thoughts on this report? Let us know!.

[shcheklein]

@skshetry hey, could you please summarize high level so that I know what to expect - what does exactly it solve (caching? just an ability to use DVC-tracked metrics / params?).

btw, do we need a tests for this as well?

[skshetry]

This only saves the metric/params files from the remote to the cache and reused on successive invocation, similar to what we did in the plots.

Using dvc tracked metrics/params was already supported when we introduced top-level metrics/params but it was broken in certain scenarios. That should be fixed by #9909.

This PR is a temporary solution so consider this as an optimization rather than a feature. So, no test is required. It uses DVCFileSystem which supports caching, so testing it here is not required.

A better solution is to use dvc.fetch() API itself inside params/metrics command, but I need to still experiment and see what performance it has. So for now, I am sticking to the tested and lazy approach that DVCFileSystem provides here.

[shcheklein]

This only saves the metric/params files from the remote to the cache and reused on successive invocation, similar to what we did in the plots.

perfect, thanks. Does it all mean I can also remove dvc pull in the workflow file?

This PR is a temporary solution so consider this as an optimization rather than a feature. So, no test is required. It uses DVCFileSystem which supports caching, so testing it here is not required.

yep, my take overall that we should be testing expected behavior (e.g. not to break if during refactoring) as well, not only bug fixes. Benchmarks is a good option I guess, but it's even harder to maintain and add that amount of them. It's interesting question on what is the best way to put all these behavioral assumptions into tests.

test `dvc metrics show`
  assert "it can use cache"
  assert "files are cached in between invocations"
  assert ...

in case like these I feel func tests are enough probably ...

[skshetry]

perfect, thanks. Does it all mean I can also remove dvc pull in the workflow file?

yes, you should not need dvc pull.

overall that we should be testing expected behavior

I think the push for keeping cli and api symmetrical has pushed us into the wrong directions. Instead of making things composable with well-defined set of components that you can rely on, we have some magical, heavy, black-boxed interfaces like DVCFileSystem and end up with a heavy API that does everything, instead of just reading metrics or params file. It does make it easier to write tests, but it's debatable whether they make it easier to maintain. They end up being a liability, as is the case at the moment.

I'd much rather test dependencies and those components with a well-defined responsibilities and boundaries than at the high-level. Also, adding tests for high-level is not possible at all, given there are 6 dependents here just for metrics show here:
exp show/exp diff/metrics show/metrics diff/dvc.api.exp_show/dvc.api.metrics_show, etc. And that's for just metrics show, there are plots show and params show. So you'll end up with 10s of tests that just do the same thing, hit the same code, and will be of zero use for finding regressions or bugs.

We have similar issues in import/import-url/ls tests, where they all tests the same thing. In addition to that, we have tests for DVCFileSystem that also tests the same thing.
And all those commands are just proxies to the DVCFileSystem.

These kinds of tests only give a false sense of security, and lead us to a CI wait time for >30 minutes.

[shcheklein]

yes, you should not need dvc pull.

👍

I'd much rather test dependencies and those components with a well-defined responsibilities and boundaries than at the high-level. Also, adding tests for high-level is not possible at all, given there are 6 dependents here just for metrics show here:

to make it clear, I was not suggesting a very specific approach, it was from the top of my head. Please consider this as a retro-like discussion.

I'm more concerned here "high" level. If let's say something breaks the next time we are refactoring this - it'll be a p0 for us. How can we prevent this from happening? My intuition that there should be a safeguard of some form to make the refactoring safe. Can it be a parametrized set of tests - may be, can it be a benchmark - probably, does it mean we need to split things - I don't know. I don't have enough context for this. But the fact that we do not have "asserts" (in a broad sense) on the behavior that is important for us does bother me.

These kinds of tests only give a false sense of security

if we have tests like this, we should be reconsidering them of course

and lead us to a CI wait time for >30 minutes.

I would personally always prioritize less p0 / p1 for end users vs CI time