Stop duplicating dvc.yaml entries for nested dirs on Windows - #907
Open
Muhtasim-Munif-Fahim wants to merge 1 commit into
Open
Stop duplicating dvc.yaml entries for nested dirs on Windows#907Muhtasim-Munif-Fahim wants to merge 1 commit into
Muhtasim-Munif-Fahim wants to merge 1 commit into
Conversation
`update_dvcyaml` recognizes the entries it owns by testing whether a path
starts with the dvclive directory, but it built that prefix with
`os.path.relpath`, which uses the native separator. Entries themselves are
written through `rel_path`, which normalizes to POSIX.
On Windows a nested `dir` therefore produced the prefix `dvclive\subdir/`
while the stored entry read `dvclive/subdir/metrics.json`. No existing entry
matched, so none were treated as stale and every run appended a fresh copy:
metrics:
- dvclive/subdir/metrics.json
- dvclive/subdir/metrics.json
A single-level `dir` was unaffected, because the only separator involved is
the "/" that was already hardcoded.
Normalizing the prefix with `Path(...).as_posix()` makes it match what is
actually written. Adds a regression test that reruns a nested-`dir` `Live`
and asserts the entries are replaced rather than appended; it fails on the
Windows CI leg without this change.
Fixes treeverse#848
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #848.
The bug
update_dvcyamlrecognizes the entries it owns by testing whether a stored path starts with the dvclive directory:but the entries themselves are written through
rel_path, which normalizes to POSIX:On Windows those two disagree as soon as
dirhas more than one level. The prefix comes out asdvclive\subdir/while the stored entry readsdvclive/subdir/metrics.json, so_drop_stale_dvclive_entriesmatches nothing, treats every existing entry as foreign, and keeps it. The new entry is then appended alongside.A single-level
diris unaffected, because the only separator involved is the"/"that was already hardcoded — which is why #848 reports this specifically for multi-level paths.Reproduction
Running the snippet from the issue twice on Windows:
Before:
After, it matches the expected output in the issue — one entry each, replaced rather than appended.
The fix
Normalize the prefix the same way the entries are normalized:
Tests
Adds
test_make_dvcyaml_nested_dir_not_duplicated, which reruns a nested-dirLiveand asserts the entries are replaced. It fails on thewindows-latestleg of your matrix without this change and is a no-op on Linux and macOS, whereos.path.relpathalready returns POSIX separators.Verified on Windows: the test fails before the change and passes after;
tests/excludingtests/frameworksis 232 passed.🤖 Generated with Claude Code