api: fallback to external_repo if not a DVC repository

[skshetry]

Fixes #3221. Now, it will just fallback to the external_repo if Repo() could not be instantiated.
Another idea would have been to check for the existence of .dvc directory, but, I went with this, as it feels correct and robust.

• ❗ 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 addressed. 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. 🙏

[skshetry]

open_by_relpath on the Repo also takes more arguments such as remote, etc which does not make sense here.

[Suor]

If it works it should stay as is. Let's not add ignored arguments, this will lead to silent errors.

[skshetry]

It does not as it also gets unexpected arguments from Repo.open_by_relpath:

dvc/dvc/repo/__init__.py

Line 430 in 4903996

def open_by_relpath(self, path, remote=None, mode="r", encoding=None):

[skshetry]

We could though add remote, or not even send one, but, I prefer this.

[Suor]

I don't understand. This line is never called from Repo.open_by_relpath().

[skshetry]

It's called by dvc.api.open() here with following args:

dvc/dvc/api.py

Lines 70 to 80 in 4903996

	def open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
	"""Context manager to open a file artifact as a file object."""
	args = (path,)
	kwargs = {
	"repo": repo,
	"remote": remote,
	"rev": rev,
	"mode": mode,
	"encoding": encoding,
	}
	return _OpenContextManager(_open, args, kwargs)

And, successively, _open calls this line:

dvc/dvc/api.py

Lines 94 to 100 in 4903996

	def _open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
	with _make_repo(repo, rev=rev) as _repo:
	with _repo.open_by_relpath(
	path, remote=remote, mode=mode, encoding=encoding
	) as fd:
	yield fd

[Suor]

Ok, then we should add only remote param not **kwargs and assert remote is None. This way things won't be silently ignored.

[efiop]

@Suor assert is not needed, as the point of that method is to provide the same interface for git and dvc repos, so it should be silently ignored there. Or am I missing something?

[skshetry]

Third argument is buffering.

[efiop]

@skshetry Please check the tests, they've failed.

[codecov]

Codecov Report

Merging #3222 into master will decrease coverage by 0.33%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3222      +/-   ##
==========================================
- Coverage   91.67%   91.33%   -0.34%     
==========================================
  Files         139      139              
  Lines        8681     8681              
==========================================
- Hits         7958     7929      -29     
- Misses        723      752      +29

Impacted Files	Coverage Δ
dvc/scm/git/__init__.py	88.39% <ø> (ø)	⬆️
dvc/system.py	70.17% <0%> (-17.55%)	⬇️
dvc/analytics.py	93.15% <0%> (-2.74%)	⬇️
dvc/daemon.py	54.23% <0%> (-1.7%)	⬇️
dvc/external_repo.py	88.61% <0%> (-1.63%)	⬇️
dvc/utils/fs.py	96.15% <0%> (-0.77%)	⬇️
dvc/utils/__init__.py	92.59% <0%> (-0.62%)	⬇️
dvc/stage.py	96.27% <0%> (-0.38%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da6399d...2e0b143. Read the comment docs.

[Suor]

If it works it should stay as is. Let's not add ignored arguments, this will lead to silent errors.