repo: use is_repo_import to check for imports in outs_graph

[pmrowla]

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

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

Fixes #9912

[pmrowla]

This is no longer valid since repo deps now have a dvcfs fs_path

[pmrowla]

I think this check could actually be if stage.is_import here since outs can't overlap with external dep paths now?

(But I left the check as repo imports only for now to match the original behavior, and since import-url deps won't cause the erepo git cloning issue)

[codecov]

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed	Coverage
dvc/repo/graph.py	100.00%

📢 Thoughts on this report? Let us know!.

[skshetry]

Thanks @pmrowla.

[shcheklein]

Thanks @pmrowla for the quick fix! Curious what is your take on adding tests on issues like this (and probably for bug fixes in general?). (I had a conversation with @efiop and our opinions differ)- I wanted to calibrate with you and probably with the whole team as well :) (clearly it goes case by case, and there are very valid exception from any policy).

[pmrowla]

I'm not really sure that a test for this would be particularly useful (i.e. having a func test for "does dvc add or outs_graph trigger an erepo clone").

This issue was really an outs_graph performance regression due to the new behavior that didn't exist when outs_graph was first implemented. The unexpected erepo clone behavior was introduced because the underlying support for repo imports has been improved (so they can now be treated like a regular dependency that has a path). But at the end of the day, both dvc add and the outs_graph worked correctly both before and after this patch.

This is something that I would expect to be caught via higher level QA and/or benchmarks. It would be better for us to have more useful real-world test cases in dvc-bench, but IMO this isn't something that needs to be tested at the DVC func or unit test level

[skshetry]

I see this as a design issue with the current fs.path thing. It should not be this easy to have unwanted side effects (especially on a DVCS and a build system where you need controlled side-effects).