import: intercept and rephrase OutputNotFound message

[pared]

• ❗ 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]

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]

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]

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

[efiop]

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

[efiop]

Ah, I see you've removed it.

[efiop]

Not sure why this was required?

[pared]

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.

[efiop]

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

[pared]

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]

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?

[pared]

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

[Suor]

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]

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.

[efiop]

@Suor @jorgeorpinel Please review.

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

[pared]

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

[efiop]

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