import: intercept and rephrase OutputNotFound message #2777

pared · 2019-11-11T15:17:01Z

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addresses. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Related to #2602

pared · 2019-11-11T15:20:43Z

Example:

#!/bin/bash

rm -rf repo
mkdir repo

pushd repo
dvc init --no-scm

dvc import git@github.com:iterative/dataset-registry.git invalid/path

was:
https://asciinema.org/a/280576

after change:
https://asciinema.org/a/280577

Suor · 2019-11-11T19:27:59Z

The thing with this it affects not only import, but also get as well as dvc.api.open()/read() calls. The idea expressed here for remotes missing for external repos.

It says basically wrap the yield in external_repo().

pared · 2019-11-12T10:19:54Z

Thanks @Suor for pointing that out. Ill fix that.

efiop · 2019-11-12T11:47:34Z

dvc/repo/__init__.py

@@ -418,10 +418,15 @@ def stages(self):
        return get_stages(self.graph)

    def find_outs_by_path(self, path, outs=None, recursive=False):
+        abs_path = (


So why do we need this if we have find_outs_by_relpath already?

Ah, I see you've removed it.

efiop · 2019-11-12T11:48:48Z

dvc/dependency/repo.py

@@ -49,10 +49,10 @@ def _make_repo(self, **overrides):

    def status(self):
        with self._make_repo() as repo:
-            current = repo.find_out_by_relpath(self.def_path).info
+            current = repo.find_out_by_path(self.def_path).info


Not sure why this was required?

Technically its not required, though considering change in find_outs_by_path, now find_out_by_relpath will work both with abs and relpath, thats why I decided to rename it.

@pared yeah, but why do we need to make find_out_by_path accept relpaths? Just wondering.

To pass original path to OutputNotFoundError. It makes sense because it passes originally inputed path to error, which is easier to understand, since its user input. Calculating relpaths/abspaths was the original reason for ambiguous paths flooding stderr when output could not be found.

Suor · 2019-11-12T14:27:29Z

dvc/repo/__init__.py

+        abs_path = (
+            os.path.join(self.root_dir, path)
+            if not os.path.isabs(path)
+            else path
+        )


It used to support paths relative to curdir, it won't after this change. Now it understands non absolute paths as relative to root dir. So this is a change in semantics. Will it break something? Why do we need it?

Oh, you are right, should make abspath out of it.
I moved abspath calculation here so that I could pass path to OutputNotFoundError. In case of relative path passed by user, OutputNotFoundError can use the input for error message, which is more meaningful than printing full path or path relative to curdir, as it used to be.
https://asciinema.org/a/280576
vs
https://asciinema.org/a/280577

But it should stay relative to curdir for outs within current repo. Like it or not, but it is what we are doing everywhere now.

Suor · 2019-11-12T14:30:51Z

dvc/external_repo.py

+    try:
+        yield repo
+    except OutputNotFoundError as cause:
+        raise NoOutputInExternalRepoError(url, cause.failed_output)


I know I suggested it, but it is fragile. We don't know whether that OutputNotFoundError we catch here comes from the yielded repo. To fix this we will probably need to pass repo to OutputNotFoundError and check it here:

if cause.repo is repo: # wrap else: raise

I also wouldn't call it cause but simply exc, it only becomes a cause when we wrap it not when we catch it.

dvc/exceptions.py

efiop · 2019-11-19T13:37:37Z

@Suor @jorgeorpinel Please review.

EDIT: sorry, nevermind, this is a WIP, seems like

pared · 2019-11-19T14:01:34Z

@Suor, @jorgeorpinel I think it should be ready now.

efiop · 2019-11-20T04:27:55Z

For the record: unrelated tests failed, that are already fixed on master.

pared requested a review from jorgeorpinel November 11, 2019 15:20

restyled-io bot mentioned this pull request Nov 12, 2019

Restyle import: intercept and rephrase OutputNotFound message #2779

Closed

pared force-pushed the 2602 branch from d692db3 to e05f14f Compare November 12, 2019 11:21

efiop reviewed Nov 12, 2019

View reviewed changes

pared force-pushed the 2602 branch 3 times, most recently from 27db275 to 5c59d74 Compare November 12, 2019 12:29

Suor suggested changes Nov 12, 2019

View reviewed changes

jorgeorpinel reviewed Nov 12, 2019

View reviewed changes

dvc/exceptions.py Outdated Show resolved Hide resolved

jorgeorpinel mentioned this pull request Nov 12, 2019

import: cryptic, confusing error output #2602

Closed

4 tasks

pared force-pushed the 2602 branch 2 times, most recently from efd50fc to a55292b Compare November 13, 2019 17:22

pared requested a review from Suor November 13, 2019 17:29

pared force-pushed the 2602 branch 4 times, most recently from 291dfc3 to 822f5c1 Compare November 15, 2019 15:52

pared changed the title ~~import: intercept and rephrase OutputNotFound message~~ [WIP] import: intercept and rephrase OutputNotFound message Nov 15, 2019

weekly-digest bot mentioned this pull request Nov 17, 2019

Weekly Digest (10 November, 2019 - 17 November, 2019) #2805

Closed

efiop requested a review from jorgeorpinel November 19, 2019 13:37

pared added 4 commits November 19, 2019 14:42

import: intercept and rephrase OutputNotFound message

f575bc7

import/get: make extarnal repo handle OutputNotFound

e5aaf98

import: just inform that output not found

5a92c46

external repo: make NoOutputInExternalRepo path-aware

d1094c4

pared force-pushed the 2602 branch from d162ac0 to d1094c4 Compare November 19, 2019 13:59

pared changed the title ~~[WIP] import: intercept and rephrase OutputNotFound message~~ import: intercept and rephrase OutputNotFound message Nov 19, 2019

jorgeorpinel approved these changes Nov 19, 2019

View reviewed changes

Suor approved these changes Nov 19, 2019

View reviewed changes

efiop merged commit 1683511 into iterative:master Nov 20, 2019

weekly-digest bot mentioned this pull request Nov 24, 2019

Weekly Digest (17 November, 2019 - 24 November, 2019) #2841

Closed

pared mentioned this pull request Nov 26, 2019

RemoteNotSpecifiedInExternalRepoError: dont pass cause of exception #2854

Merged

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import: intercept and rephrase OutputNotFound message #2777

import: intercept and rephrase OutputNotFound message #2777

pared commented Nov 11, 2019 •

edited

Loading

pared commented Nov 11, 2019

Suor commented Nov 11, 2019

pared commented Nov 12, 2019

efiop Nov 12, 2019

efiop Nov 12, 2019

efiop Nov 12, 2019

pared Nov 12, 2019

efiop Nov 12, 2019 •

edited

Loading

pared Nov 12, 2019

Suor Nov 12, 2019

pared Nov 12, 2019

Suor Nov 12, 2019

Suor Nov 12, 2019

efiop commented Nov 19, 2019 •

edited

Loading

pared commented Nov 19, 2019

efiop commented Nov 20, 2019

import: intercept and rephrase OutputNotFound message #2777

import: intercept and rephrase OutputNotFound message #2777

Conversation

pared commented Nov 11, 2019 • edited Loading

pared commented Nov 11, 2019

Suor commented Nov 11, 2019

pared commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop commented Nov 19, 2019 • edited Loading

pared commented Nov 19, 2019

efiop commented Nov 20, 2019

pared commented Nov 11, 2019 •

edited

Loading

efiop Nov 12, 2019 •

edited

Loading

efiop commented Nov 19, 2019 •

edited

Loading