iterative / dvc

🦉 Data Versioning and ML Experiments
https://dvc.org
Apache License 2.0
13.95k stars 1.19k forks source link

`dvc metrics show` reads the wrong file #9954

Open shcheklein opened 1 year ago

shcheklein commented 1 year ago

After the recent changes to read cached metrics (and using now DVCFS?).

Since in the dvc_objects.Path (name is confusing, should be PathUtils or something like that, as well as in objects like any of the variety of FSs should have fs.path_utils, but at least not fs.path) we resolve path like ../out/metrics.cvs into the same out/metrics.cvs. We are just ignoring the .. part. It leads to us potentially reading the wrong file in a repo that has an outdated external output (which is also not clear if we support or not - e.g. we allow still creating those stages even via CLI, via API, etc).

(.venv) √ Projects/test-dvc-out-metrics % tree .
.
├── dvc.yaml
└── out
    └── metric.csv

1 directory, 2 files
(.venv) √ Projects/test-dvc-out-metrics % cat out/metric.csv
a,res
1,2
(.venv) √ Projects/test-dvc-out-metrics % cat dvc.yaml
stages:
  metric:
    cmd: cmd
    metrics:
    - ../out/metric.csv:
        cache: false
(.venv) √ Projects/test-dvc-out-metrics % dvc metrics show
Path
../out/metric.csv  a,res 1,2

DVC Version

DVC version: 3.21.1 (pip)
-------------------------
Platform: Python 3.11.4 on macOS-13.3.1-arm64-arm-64bit
Subprojects:
    dvc_data = 2.16.1
    dvc_objects = 1.0.1
    dvc_render = 0.6.0
    dvc_task = 0.3.0
    scmrepo = 1.3.1
Supports:
    azure (adlfs = 2023.8.0, knack = 0.11.0, azure-identity = 1.14.0),
    gdrive (pydrive2 = 1.17.0),
    gs (gcsfs = 2023.6.0),
    hdfs (fsspec = 2023.6.0, pyarrow = 13.0.0),
    http (aiohttp = 3.8.5, aiohttp-retry = 2.8.3),
    https (aiohttp = 3.8.5, aiohttp-retry = 2.8.3),
    oss (ossfs = 2023.8.0),
    s3 (s3fs = 2023.6.0, boto3 = 1.28.17),
    ssh (sshfs = 2023.7.0),
    webdav (webdav4 = 0.9.8),
    webdavs (webdav4 = 0.9.8),
    webhdfs (fsspec = 2023.6.0)
Config:
    Global: /Users/ivan/Library/Application Support/dvc
    System: /Library/Application Support/dvc
Cache types: <https://error.dvc.org/no-dvc-cache>
Caches: local
Remotes: None
Workspace directory: apfs on /dev/disk3s1s1
Repo: dvc, git
Repo.site_cache_dir: /Library/Caches/dvc/repo/9a90ec76c34eb30e03f6c3851d26b1c0
shcheklein commented 1 year ago

cc @skshetry

skshetry commented 1 year ago

External outputs aren’t supported in show commands. We have to drop external outputs before we pass them to the dvcfs.

shcheklein commented 1 year ago

External outputs aren’t supported in show commands. We have to drop external outputs before we pass them to the dvcfs.

It's fine if they are not supported (though it's not 100% for me what is the level of support and if we handle this gracefully). The bug here is that we read the wrong existing internal file instead.

skshetry commented 1 year ago

Yes, that’s a bug. As I have said, we have to filter them out before passing to dvcfs, as it does not (and cannot) support external files.

dberenbaum commented 1 year ago

@shcheklein Is it about passing studio tests? I wonder if anyone is using external metrics files.

shcheklein commented 1 year ago

@shcheklein Is it about passing studio tests? I wonder if anyone is using external metrics files.

We had an issue with some external outs in general, but I think @efiop has solved it and now it's running fine.

We don't expect on the Studio side this to work at all (I mean external metrics, or any external outs).

This bug is not related to Studio tests. If someone has a repo with a similar structure that I described - we might show some wrong numbers. But I doubt it exists atm.

I would check though (not high priority either) if that applies to other things - like plots. That might be a bit more common story. Most likely for users it would look like a bad error message in Studio.