api: open scm controlled files #2566

pared · 2019-10-03T07:22:58Z

Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? If yes, have you created a PR for
dvc.org documenting it or at
least opened an issue for it? If so, please add a link to it.

dvc/repo/__init__.py

tests/func/test_api.py

Suor

Several things:

we should test for local (git handled) file first, because that one is faster. Plus this will allow us to not swallow OutputNotFoundError as we do here.
I am not a big fan of double catch like here, I will prefer .exists() test
~~not sure we should use self.tree here, why not simply use open()?~~ see EDIT
we lost symmetry between Repo.open() and Repo._open(), ._open() only open outputs now while .open() does everything, so rename ._open() -> ._open_out(). Still starts with underscore since it raises unwrapped exception.

EDIT: It looks like we use Repo.tree everywhere in Repo/Stage for now, so that should be kept. I still don't like this abstraction, but that one is a separate topic.

jorgeorpinel · 2019-10-03T20:26:26Z

Hi. Will this PR implement #2515 as suggested in #2515 (comment)?

pared · 2019-10-04T07:03:09Z

@jorgeorpinel
It does not seem so. Problem with this task lies inside dvc/repo/Repo.open, while as I look in the code, #2515 requires changes in dvc/repo/Repo.get at dvc file check and out file search
It should probably be handled by separate PR.

Suor

One point from my first review still stands:

we should test for local (git handled) file first, because that one is faster. Plus this will allow us to not swallow OutputNotFoundError as we do here.

OutputNotFoundError is used as cause now, which doesn't make much sense to me, I would let it show itself, e.g. not catch it, after the change above.

efiop · 2019-10-07T11:44:05Z

tests/func/test_api.py


    # Remove cache to make foo missing
    shutil.rmtree(dvc_repo.cache.local.cache_dir)
+    os.remove(repo_dir.FOO)


Why is this one needed? Because it will trigger https://github.com/iterative/dvc/pull/2566/files#diff-16d20341660af559adf7d9e7306c760dR458 ?

efiop · 2019-10-07T11:45:35Z

tests/func/test_api.py

+    dvc_repo.run(
+        metrics_no_cache=[metric_file], cmd="python {}".format(code_file)
+    )
+    os.remove(metric_file)


Is this one needed for https://github.com/iterative/dvc/pull/2566/files#diff-16d20341660af559adf7d9e7306c760dR458 again? This seems weird.

Yes, but I don't think there is another way to test it. We want to test whether dvc will inform us that some output cannot be checked out. If the file is in a repo, it seems natural that it will be used, instead of checking whether it is in the cache, don't you think?

efiop · 2019-10-07T11:46:58Z

dvc/repo/__init__.py

    @contextmanager
    def open(self, path, remote=None, mode="r", encoding=None):
        """Opens a specified resource as a file descriptor"""
+        if not remote and self.tree.exists(path):


tree.exists() will be True if someone done checkout and might not be what we need if dvc file then changed (e.g. by git checkout). Shouldn't we first look in cached outputs and then fallback to tree check?

Hmm, at the beginning I agreed with @Suor s comment here
but now that you mention checkout, it probably better to check cache first.

@pared Good point. Or maybe we can improve this current approach so that it doesn't suffer from that issue?

I guess we could solve that either by checking a checksum, or prompting the user?
Checking the checksum would result in losing the advantage suggested by @Suor.

So we should more strictly go over all the possibilities here:

get out at the start

if we have out and it's cached then we go to ._open_cached()

if we don't have out or have out, which is not cached, we go .tree.open(), no need to check for exists then.

There is never such thing as retrying tree open after cached open or vice versa.

That makes sense. I should have started with separating finding out from opening it. @efiop do you have any concerns about this approach?

@pared sounds good to me.

Suor · 2019-10-07T12:04:15Z

dvc/exceptions.py

+class NotCachedOutputFileMissingError(DvcException):
+    def __init__(self, path_info):
+        super(NotCachedOutputFileMissingError, self).__init__(
+            "Can't find '{}'. This file was not cached.".format(path_info)
        )


Both class name and message blur more than they explain. And, BTW, what's the value of distinguishing this from OutputFileMissing?

I say we should stop inventing more exception classes. This complicates both our code and API.

Suor · 2019-10-07T12:18:33Z

tests/func/test_api.py

+    os.remove(metric_file)
+
+    with pytest.raises(NotCachedOutputFileMissingError):
+        api.read(metric_file)


These two tests are almost the same. I would say simply move:

os.remove(metric_file) with pytest.raises(NotCachedOutputFileMissingError): api.read(metric_file)

to the end of the first one and remove the second test.

Also, why do you create new code file, while we already have code.py?

New code is required due to issue regarding original code described in #2574

Suor · 2019-10-07T12:27:08Z

dvc/repo/__init__.py

+    def _open_out(self, path, remote=None, mode="r", encoding=None):
        out, = self.find_outs_by_path(path)
+
+        if not out.use_cache and not out.exists:


This condition is wrong - if out is not using cache, but is missing we continue to try open it from cache, which doesn't make sense.

I would say we should check if out is cached before ._open_out() then, rename ._open_out() to ._open_cached() and pass out there instead of path.

Suor · 2019-10-07T12:31:45Z

dvc/repo/__init__.py

    @contextmanager
    def open(self, path, remote=None, mode="r", encoding=None):
        """Opens a specified resource as a file descriptor"""
+        if not remote and self.tree.exists(path):


So we should more strictly go over all the possibilities here:

get out at the start

if we have out and it's cached then we go to ._open_cached()

if we don't have out or have out, which is not cached, we go .tree.open(), no need to check for exists then.

There is never such thing as retrying tree open after cached open or vice versa.

efiop

LGTM

Suor

Looks good.

Suor · 2019-10-08T11:15:07Z

tests/func/test_api.py

using

..., cmd="python -c '{}'".format(metric_code)

and not creating any file should be easier. This not that important though.

Sure, I will introduce it

efiop · 2019-10-08T12:24:30Z

@pared ok, one more question. So does this make dvc get work with scm-controlled files as well?

pared · 2019-10-08T12:29:22Z

@efiop no,
here is the reason

shcheklein · 2019-10-08T17:41:51Z

@efiop no, here is the reason

@pared @efiop let's create a ticket then? We need to get to a parity here.

efiop · 2019-10-08T19:24:12Z

@shcheklein We do have the ticket #2515 . I was just double-checking.

efiop requested a review from Suor October 3, 2019 07:33

efiop reviewed Oct 3, 2019

View reviewed changes

dvc/repo/__init__.py Outdated Show resolved Hide resolved

efiop reviewed Oct 3, 2019

View reviewed changes

tests/func/test_api.py Outdated Show resolved Hide resolved

Suor suggested changes Oct 3, 2019

View reviewed changes

efiop requested a review from Suor October 4, 2019 09:13

Suor suggested changes Oct 4, 2019

View reviewed changes

pared force-pushed the 2507 branch from fb45271 to cacdc2c Compare October 4, 2019 10:48

pared added 7 commits October 4, 2019 15:22

tree: make open method use mode and encoding

bbcfce9

api: try to open using self.tree if output not found

a3b1c32

repo: rename OutputMissingException

6525553

test: api: test adding not cached output

9d1f245

api: open: exists test instead of try

f6730da

repo: open: try opening from tree first

4f59e6e

repo: open: handle missing and not cached files

2f64620

pared force-pushed the 2507 branch from 31a38c9 to 2f64620 Compare October 4, 2019 13:24

pared requested review from Suor and efiop October 4, 2019 13:26

jorgeorpinel mentioned this pull request Oct 4, 2019

get: allow downloading regular files/dirs tracked by Git #2515

Closed

efiop reviewed Oct 7, 2019

View reviewed changes

Suor suggested changes Oct 7, 2019

View reviewed changes

pared force-pushed the 2507 branch from 742b8eb to 4a81096 Compare October 8, 2019 07:50

pared requested review from Suor and efiop October 8, 2019 07:51

efiop approved these changes Oct 8, 2019

View reviewed changes

Suor approved these changes Oct 8, 2019

View reviewed changes

pared force-pushed the 2507 branch from 4a81096 to 4ee2a7a Compare October 8, 2019 11:34

repo: open: separate find out logic from opening

958fcfa

pared force-pushed the 2507 branch from 4ee2a7a to 958fcfa Compare October 8, 2019 11:39

efiop merged commit d2f8cb0 into iterative:master Oct 8, 2019

pared deleted the 2507 branch December 17, 2019 13:11

api: open scm controlled files #2566

api: open scm controlled files #2566

Uh oh!

Conversation

pared commented Oct 3, 2019 • edited by efiop Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Suor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pared commented Oct 4, 2019

Uh oh!

Suor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Suor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop commented Oct 8, 2019

Uh oh!

pared commented Oct 8, 2019

Uh oh!

shcheklein commented Oct 8, 2019

Uh oh!

efiop commented Oct 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pared commented Oct 3, 2019 •

edited by efiop

Loading

Suor left a comment •

edited

Loading

jorgeorpinel commented Oct 3, 2019 •

edited

Loading