Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot plot with windows paths syntax #8689

Closed
SamuelAnsel opened this issue Dec 13, 2022 · 3 comments · Fixed by #9072
Closed

Cannot plot with windows paths syntax #8689

SamuelAnsel opened this issue Dec 13, 2022 · 3 comments · Fixed by #9072
Assignees
Labels
A: plots Related to the plots bug Did we break something? P: windows Related to the Platform: Windows

Comments

@SamuelAnsel
Copy link

Bug Report

Windows path syntax

I work on windows and tried to display some plots, here is my dvc.yaml file :

  train:
    cmd: python .\src\train.py
    deps:
    - .\data\train.csv
    params:
    - train.batch_size
    - train.seed
    outs:
    - .\train_out
    metrics:
    - .\dvclive\metrics.json:
        cache: false
    plots:
    - .\dvclive\plots\metrics\train_loss.tsv:
        cache: false
        x: step
        y: train_loss
    - .\dvclive\plots\metrics\train_acc.tsv:
        cache: false
        x: step
        y: train_acc

Notice that my paths have a Windows syntax here.

If I run dvc plots show -v --json it outputs :

{
  ".\\dvclive\\plots\\metrics\\train_loss.tsv": [],
  ".\\dvclive\\plots\\metrics\\train_acc.tsv": []
}
2022-12-13 13:48:49,678 DEBUG: Analytics is enabled.
2022-12-13 13:48:49,681 DEBUG: Trying to spawn '['daemon', '-q', 'analytics', 'C:\\Users\\sansel\\AppData\\Local\\Temp\\tmpleidc5tk']'
2022-12-13 13:48:49,684 DEBUG: Spawned '['daemon', '-q', 'analytics', 'C:\\Users\\sansel\\AppData\\Local\\Temp\\tmpleidc5tk']'

Then if I replace \ with / in my paths like so :

stages:
  train:
    cmd: python ./src/train.py
    deps:
    - ./data/train.csv
    params:
    - train.batch_size
    - train.seed
    outs:
    - train_out
    metrics:
    - ./dvclive/metrics.json:
        cache: false
    plots:
    - ./dvclive/plots/metrics/train_loss.tsv:
        cache: false
        x: step
        y: train_loss
    - ./dvclive/plots/metrics/train_acc.tsv:
        cache: false
        x: step
        y: train_acc

Now DVC is able to correctly display plots.

Expected

You would expect Windows path syntax to work on Windows

Environment information

Output of dvc doctor:

$ dvc doctor
DVC version: 2.34.0 (exe)
---------------------------------
Platform: Python 3.10.8 on Windows-10-10.0.19044-SP0
Subprojects:

Supports:
        azure (adlfs = 2022.10.0, knack = 0.10.0, azure-identity = 1.12.0),
        gdrive (pydrive2 = 1.14.0),
        gs (gcsfs = 2022.11.0),
        hdfs (fsspec = 2022.11.0, pyarrow = 10.0.0),
        http (aiohttp = 3.8.3, aiohttp-retry = 2.8.3),
        https (aiohttp = 3.8.3, aiohttp-retry = 2.8.3),
        oss (ossfs = 2021.8.0),
        s3 (s3fs = 2022.11.0, boto3 = 1.24.59),
        ssh (sshfs = 2022.6.0),
        webdav (webdav4 = 0.9.8),
        webdavs (webdav4 = 0.9.8),
        webhdfs (fsspec = 2022.11.0)
Cache types: hardlink
Cache directory: NTFS on C:\
Caches: local
Remotes: None
Workspace directory: NTFS on C:\
Repo: dvc, git

You are using dvc version 2.34.0; however, version 2.37.0 is available.
To upgrade, uninstall dvc and reinstall from https://dvc.org.
@daavoo daavoo added A: plots Related to the plots P: windows Related to the Platform: Windows labels Dec 13, 2022
@dberenbaum dberenbaum added p1-important Important, aka current backlog of things to do bug Did we break something? labels Dec 13, 2022
daavoo added a commit to iterative/dvclive that referenced this issue Dec 14, 2022
daavoo added a commit to iterative/dvclive that referenced this issue Dec 14, 2022
@daavoo daavoo self-assigned this Dec 15, 2022
@daavoo daavoo linked a pull request Dec 15, 2022 that will close this issue
2 tasks
@blakeNaccarato
Copy link
Contributor

See my original comment where I provided a repro for a related issue. I have since renamed that repro to reflect its relationship to the current issue number 8689. Here's that repro again. It breaks a plot when there's a \ in the path. The plot path\fail.png doesn't show up in rendered HTML of dvc plots show, while path/pass.png does. The same goes for the plot rendering in the DVC VSCode extension. In fix-offending-plot the \ is swapped for /, then in remove-offending-plot the offending plot is commented out of dvc.yaml. Each branch has the output of dvc plots show committed as well.

It is not an exhaustive reproduction for this now broader issue, but represents one way in which Windows paths can mess things up.

Workaround

Here's an updated, generalized workaround since I last commented over in the other issue. Supply all paths in params.yaml under a key of your choosing (e.g. dirs), then inject them into dvc.yaml using DVC's built-in templating support (e.g. ${dirs.<...>}). Now you can watch your params.yaml for changes and always run a script like the repl_path_in_params.py below to normalize any \ to / before those parameters get injected into dvc.yaml. It's a bit of a bandaid solution, but it makes sure your paths are acceptable by DVC.

See below for an example normalizing script, parameters file, and dvc.yaml file.

repl_path_in_params.py
"""Replace Windows path separators with POSIX paths in configuration file."""

from pathlib import Path

from ruamel.yaml import YAML

PARAMS_FILE = "params.yaml"
DIRS_KEY = "dirs"

yaml = YAML()
yaml.indent(offset=2)


def main():
    params = yaml.load(PARAMS_FILE)
    params[DIRS_KEY] = repl_path(params[DIRS_KEY])
    yaml.dump(params, PARAMS_FILE)


def repl_path(dirs_dict: dict[str, Path]):
    """Replace Windows path separator with POSIX separator."""
    return {k: str(v).replace("\\", "/") for k, v in dirs_dict.items()}


if __name__ == "__main__":
    main()
params.yaml
dirs:
  plot_new_fit_0: data/metrics/plots/new_fit_0.png
  plot_new_fit_1: data/metrics/plots/new_fit_1.png
  plot_new_fit_2: data/metrics/plots/new_fit_2.png
  file_pipeline_metrics: data/metrics/tables/pipeline_metrics.json
other_params:
  ...
dvc.yaml
  ...
  metrics:
    cmd: "<metrics_cmd>"
    deps:
      - ...
    metrics:
      - "${dirs.file_pipeline_metrics}"
    plots:
      - "${dirs.plot_new_fit_0}"
      - "${dirs.plot_new_fit_1}"
      - "${dirs.plot_new_fit_2}"
    params:
      - ...
  ...

@daavoo
Copy link
Contributor

daavoo commented Dec 20, 2022

Thanks for the workaround @blakeNaccarato 🙏
I will try to look into a proper fix when I find the time

@daavoo daavoo added bug Did we break something? and removed bug Did we break something? p1-important Important, aka current backlog of things to do labels Feb 18, 2023
@anitagraser
Copy link

The most tricky thing about this bug is that dvc stage add happily adds stages with backslashes in the path and dvc repro and dvc exp run run the stages without errors but then, suddenly, there are no plots.

daavoo added a commit that referenced this issue Feb 23, 2023
Was causing `parse` to silently file when trying to collect the sources.

Fixes #8689

- Add `test_show_sources_with_native_os_path`.
- Remove unnecessary OrderedDict usage in `load_sv`.
daavoo added a commit that referenced this issue Feb 23, 2023
Was causing `parse` to silently file when trying to collect the sources.

Fixes #8689

- Add `test_show_sources_with_native_os_path`.
- Remove unnecessary OrderedDict usage in `load_sv`.
daavoo added a commit that referenced this issue Feb 23, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 23, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 23, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 23, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 23, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Feb 24, 2023
Fixes #8689

- Add test_show_sources_with_native_os_path.
- Remove unnecessary OrderedDict usage in load_sv.
daavoo added a commit that referenced this issue Mar 1, 2023
`plots.show` returned `definitions` in native `nt` format but sources in `posixpath` format so the matching was failing.

Closes #8689

- Add `test_show_plots_defined_with_native_os_path`.
daavoo added a commit that referenced this issue Mar 6, 2023
`plots.show` returned `definitions` in native `nt` format but sources in `posixpath` format so the matching was failing.

Closes #8689

- Add `test_show_plots_defined_with_native_os_path`.
daavoo added a commit that referenced this issue Mar 6, 2023
`plots.show` returned `definitions` in native `nt` format but sources in `posixpath` format so the matching was failing.

Closes #8689

- Add `test_show_plots_defined_with_native_os_path`.
daavoo added a commit that referenced this issue Mar 6, 2023
`plots.show` returned `definitions` in native `nt` format but sources in `posixpath` format so the matching was failing.

Closes #8689

- Add `test_show_plots_defined_with_native_os_path`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: plots Related to the plots bug Did we break something? P: windows Related to the Platform: Windows
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants