get/import: improve error message on no default remote

[shcheklein]

Since we allow importing/getting files that are not cached this error does not make much sense:

failed to import 'model.pkl' from '../test-import/'. - config file error: no remote specified. Setup default remote with

in a lot of cases.

[Suor]

So is this just a wrong error message?

[shcheklein]

I would say it's in a wrong place. We need to raise this only when we try to import a cached object and no default remote is specified.

[alvaroabascar]

@shcheklein, @Suor I found out I could not run dvc import if the source repo has no default remote set. Also, the error I got (see below) confused me, because it made me think I had to run dvc config core.remote in the working repository.

ERROR: failed to import 'models/spacy_model/' from 'git@gitlab.com:iomed/datascience/spacy-model-es-cat'. - config file error: no remote specified. Setup default remote with
    dvc config core.remote <name>
or use:
    dvc pull -r <name>

As a newbie dvc user who might be missing something, I'd say it would be useful to:

• Skip the error message shown by @shcheklein
• If no default remotes are set:
  • in the source repository, have a more explicit error message indicating that the problem is in the source repository.
  • Have a parameter to indicate the remote to use, maybe?

[Suor]

+1 for better more specific message.
-1 for extra param, why that could not be fixed by adding default remote to source repo? What's the use case?

[efiop]

Let's start with catching that error and re-raising something better.

[Suor]

The place to wrap an exception in external_repo() around the yield.

[shcheklein]

@Suor @efiop if we allow non-cached artifacts to be pulled from an external repo, why would we require any remotes at all? The case can (and we had something like this in the past) - use it to collect metrics and use dvc metrics show exclusively. And other cases, of course.

+1 for improving messages.

[shcheklein]

Btw, it's the second time someone is asking us about adding a parameter to specify remote that should be used for an external repo. Just for the record.

[shcheklein]

Specifically this one is related: #2466

[Suor]

Let's discuss remote function param or cli in #2466. And leave this issue for message only.

[shcheklein]

And leave this issue for message only.

@Suor there are three parts to it:

• remove the requirement of a remote defined for non-cached artifacts (this is actually the main purpose of the ticket I had in mind when I created it)
• improve the message when remote is needed after all (cached outputs) - and I think we are all on the page with this
• being able to specify an additional parameter - agreed, let's move it to the get/import/list/etc command should accept a remote as argument #2466 and keep discussion going there.

[efiop]

@shcheklein

remove the requirement of a remote defined for non-cached artifacts (this is actually the main purpose of the ticket I had in mind when I created it)

This should probably be done as a part of implementing get/import for non-cached files. Out of scope for this ticket.

[shcheklein]

@efiop it affects API as well, so can be considered independent. That's at least the scope I had in mind creating the ticket :) Anyway, I don't have a strong opinion. If you feel it's better to take care of it as part of implementing get/import for non-cached files I'm totally fine with that - just put a note in the corresponding ticket description, or checkbox to keep track of it.

[efiop]

@shcheklein That's a good point! Thinking about it, that issue is only relevant for erepo.pull(and friends). When you are accessing regular non-cached files, that one is not called. So it would be best to keep that in mind when fixing this. Will do.

[pared]

If main point of this task is to remodel the message for get/import we should probably rename the issue, as it indicates that main point is to lift the remote requirement from them. @efiop can I do that?

[efiop]

@pared Sorry for the delay. Sure, feel free!

[pared]

@efiop renamed the issue, created new ticket for lifting the remote requirement, so that we do not lost track of it.

[pared]

@efiop In the PR that fixed this issue, we are passing the ConfigError as cause, which results in
even longer error message:

https://asciinema.org/a/Kxo0jgnTtLrK2ZPFWhI4VyzDU

I think for the sake of UI we should not pass the cause, as it obstructs the readability of this exception.

[efiop]

@pared Makes sense, please feel free to submit a PR for that. Reopen this issue if needed.