Skip to content

Conversation

@skshetry
Copy link
Collaborator

parsing.Context now only expects the path to always be relative path. To clarify, it's relative if it's a LocalFileSystem and relatively-absolute (relative to the root of the repo) in GitFileSystem, as opposed to being absolutely-absolute paths.

This expectation for Context to always get a relative path is managed in DataResolver for now, as changing anything in higher level may not be trivial.

This change, that it uses the relative path in LocalFileSystem and relatively-absolute paths in GitFileSystem does affect dvc.params.diff(). Before we were using same relative/absolute paths, which worked for both filesystems, but now we will receive the data in something like following format:

{
    "workspace": {
        "data": {
            "../test_params.yaml": {  # relative path from LocalFS
                "data": {
                    "vars.model1.epoch": 20,
                    "vars.model2.epoch": 35
                }
            }
        }
    },
    "HEAD": {
        "data": {
            "test_params.yaml": {  # From GitFS as it is relative from the root of the repo
                "data": {
                    "vars.model1.epoch": 15,
                    "vars.model2.epoch": 35
                }
            }
        }
    }
}

So, as a result of this, this had to be transformed such that they return same paths, so that we can compare them easily.

Fix #7307.

@skshetry skshetry requested a review from a team as a code owner April 16, 2022 16:40
@skshetry skshetry requested a review from pared April 16, 2022 16:40
@skshetry skshetry self-assigned this Apr 16, 2022
@efiop efiop requested review from efiop and removed request for pared April 17, 2022 19:30
@efiop
Copy link
Contributor

efiop commented Apr 18, 2022

@skshetry Any thoughts on how this would behave itself with moving gitfs to root paths? Asking in case you've looked into that 🙂

@skshetry
Copy link
Collaborator Author

@skshetry Any thoughts on how this would behave itself with moving gitfs to root paths? Asking in case you've looked into that 🙂

It's hard to say for all stage loading helpers, but this is how it already works with root paths of gitfs. The following code is trying to transform filesystem-absolute paths into root paths in the case of GitFileSystem:
https://github.com/iterative/dvc/blob/5d4d8585744bbe8163a32c9f78c9bb681b4eba56/dvc/parsing/__init__.py#L142-L147

@efiop
Copy link
Contributor

efiop commented Apr 18, 2022

@skshetry Great! 🔥 With this and repofs, we can now try to use treeverse/scmrepo#56

@efiop efiop merged commit 5eb16ab into treeverse:main Apr 18, 2022
@skshetry skshetry deleted the fix-7307 branch April 18, 2022 11:20
@skshetry skshetry added the bugfix fixes bug label Apr 18, 2022
@skshetry skshetry restored the fix-7307 branch April 27, 2022 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

import: does not work with repositories using params

2 participants