dvc import ignores cache and downloads from remote

[johnyaku]

Bug Report

dvc import ignores cache and downloads from remote

Description

I'm using dvc import to import some files that I know are already in an external shared cache (which is also the cache directory for the current project).

dvc downloads the imported files rather than simply sym-linking to the cache

Reproduce

1. create external cache directory
2. create project that uses this external directory as its cache directory
3. git push the config
4. dvc import some data from a registry but don't git push the resulting .dvc files
5. delete the project directory but not the external cache
6. recreate the project directory by recloning a new instances (this will include the config from 3 above)
7. dvc import the same data again

Expected

Expected dvc to create symlinks to the external cache when the files were already there.

Environment information

Observed this bug with dvc 2.53 and 2.55.
dvc 2.39 works as expected

[dberenbaum]

Related to #8808. Thanks for pointing out that it worked in 2.39. I had missed that this was a regression, so going to bump the priority of this one.

[efiop]

Definitely my fault and caused by #9246 I think the issue is that dvcfs that we use in imports doesn't set local cache and thus always streams from remote (oneliner fix if confirmed). I'll take a closer look.

[shcheklein]

I was looking a bit into this yesterday. I can confirm that it's first got broken with 2.53.0 (and yes, it's related to the #9246 most likely):

DVC version: 2.53.0
-------------------
Platform: Python 3.9.16 on macOS-13.3.1-arm64-arm-64bit
Subprojects:
	dvc_data = 0.47.3
	dvc_objects = 0.21.2
	dvc_render = 0.3.1
	dvc_task = 0.2.1
	scmrepo = 0.2.1
Supports:
	http (aiohttp = 3.8.4, aiohttp-retry = 2.8.3),
	https (aiohttp = 3.8.4, aiohttp-retry = 2.8.3),
	s3 (s3fs = 2023.4.0, boto3 = 1.26.76)
Cache types: reflink, hardlink, symlink
Cache directory: apfs on /dev/disk3s1s1
Caches: local
Remotes: s3, s3
Workspace directory: apfs on /dev/disk3s1s1
Repo: dvc, git
Repo.site_cache_dir: /Library/Caches/dvc/repo/f2f169a5407da9f721ec62974a7ca573

I hit this with a simple example like this, from the get started:

dvc import https://github.com/iterative/dataset-registry.git get-started/data.xml -v

It ignores cache and downloads it again.

Haven't had enough time, but brief look was not enough to see the way to pass cache into the DVCFileSystem's get_file (which goes into DataFileSystem.get_file()). Do we have any examples? Is it managed outside before we even go into download?

---

It there is not single line fix, should we revert it (seems severe enough since affects multiple commands) for now, until we fix this?

Also, a bit unrelated minor question, while looking into this, I saw that circular imports detection got deleted. Out of curiosity, is it handled by something else (e.g. regular graph circular deps check?). Or did we discuss that it's not critical anymore? (it would be great to have this kind of context in the PR description).

[efiop]

Let me take a look today and I'll get back with the actions. Sure, reverting is also an option, as always.

Regarding circular imports, the new mechanism should handle that better than before so there wasn't a need a for that check anymore. There wasn't a written discussion about it, but we did discuss it privately with @pmrowla before and agreed that it makes sense to drop that for now.

[shcheklein]

the new mechanism should handle that better than before so there wasn't a need a for that check anymore. There wasn't a written discussion about it, but we did discuss it privately with @pmrowla before and agreed that it makes sense to drop that for now.

yep, that's fine (would be great still to have it documented). What I guess brought my attention that we also removed two tests- do we have a scenario like this somewhere tested, etc - thoughts that were crossing my mind.

[efiop]

@shcheklein

Haven't had enough time, but brief look was not enough to see the way to pass cache into the DVCFileSystem's get_file (which goes into DataFileSystem.get_file()). Do we have any examples? Is it managed outside before we even go into download?

You can pass config to the DVCFileSystem, which will pass it to Repo. No documented examples, unfortunately.

yep, that's fine (would be great still to have it documented)

I don't think this was documented before. The scenario is overall complex and I wouldn't document it without first researching it properly. It will require a significant amount of effort to document every edge case and tbh I don't think the effort is worth it. Happy to create an issue if you want to, but realistically I don't think it will be actionable or worthwhile.

What I guess brought my attention that we also removed two tests- do we have a scenario like this somewhere tested, etc - thoughts that were crossing my mind.

Yup, that PR is not my proudest "verbose" PR in regards to comments and descriptions 😅 First test just doesn't apply anymore, and the second one tests that we raise a nonexistent exception in a normal scenario. I don't think we need a new test there at that point, as it is a normal scenario now and should work by design. Happy to discuss it further in that PR (just to avoid polluting this unrelated issue).