index: fix storage for imports

[efiop]

Fixes #10194

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (987b1c4) 90.50% compared to head (bee5aec) 90.50%.

Files	Patch %	Lines
dvc/repo/index.py	84.61%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10213   +/-   ##
=======================================
  Coverage   90.50%   90.50%           
=======================================
  Files         499      499           
  Lines       37970    37982   +12     
  Branches     5517     5518    +1     
=======================================
+ Hits        34363    34375   +12     
  Misses       2965     2965           
  Partials      642      642           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dberenbaum]

Thanks @efiop! Just checking that this doesn't break some other edge cases for import-url:

1. dvc import-url --version-aware should still pull from source (and should not be pushed to remote).
2. dvc import-url --no-download && dvc pull should be able to download from source even if the data was never pushed (see the discussion around --no-download here).
3. Both of those should fail if the source data is now missing or modified.

The general logic behind import-url should be to try to pull from dvc remote storage first but be able to fall back to the source storage if the same content still exists there.

[efiop]

@dberenbaum Thank you for this, looks like I indeed completely forgot about 1 (confusing how we have 3 different behaviours for import-url and import together). Will adjust.

Second one also needs some attention with cloud versioning context, but it is clear that it is partially broken at the very least. Also taking a look (if I won't send a PR soon I'll create an issue first).

[efiop]

@dberenbaum Double checked, we have test for those in

dvc/dvc/testing/workspace_tests.py

Line 89 in bf5fe95

class TestImportURLVersionAware:

, so we should be good for now with all of those points.

dvc import-url --version-aware should still pull from source...

Always using that source fs as a remote storage with version-aware imports ✅

... (and should not be pushed to remote).

and that remote is always marked as read-only ✅

dvc import-url --no-download && dvc pull should be able to download from source even if the data was never pushed (see the discussion around --no-download #8024).

We have a specific test for that (see no_download one in the first link). ✅

Both of those should fail if the source data is now missing or modified.

We request a particular version_id in both, so missing (particular version_id) will get you a FileNotFoundError and modified shouldn't affect it (as that's a different version_id).

[dberenbaum]

Double checked, we have test for those

That's great (and how it should be obviously).

We have a specific test for that (see no_download one in the first link).

Non-version-aware remotes should also be able to pull from source after no-download. That's tested here:

dvc/tests/func/test_data_cloud.py

Lines 268 to 277 in bf5fe95

	def test_pull_partial_import(tmp_dir, dvc, local_workspace):
	local_workspace.gen("file", "file content")
	dst = tmp_dir / "file"
	stage = dvc.imp_url("remote://workspace/file", os.fspath(dst), no_download=True)
	result = dvc.pull("file")
	assert result["fetched"] == 0
	assert dst.exists()
	assert stage.outs[0].get_hash().value == "d10b4c3ff123b26dc068d43a8bef2d23"

We request a particular version_id in both, so missing (particular version_id) will get you a FileNotFoundError and modified shouldn't affect it (as that's a different version_id).

For non-version-aware remotes, we should also test that it fails if the original source is missing or modified. See this comment. I added tests for those in https://github.com/iterative/dvc/tree/test-pull-partial-import if you want to merge those. test_pull_partial_import_missing passes but test_pull_partial_import_modified doesn't. Not sure when that broke.

[dberenbaum]

For non-version-aware remotes, we should also test that it fails if the original source is missing or modified. See this comment. I added tests for those in https://github.com/iterative/dvc/tree/test-pull-partial-import if you want to merge those. test_pull_partial_import_missing passes but test_pull_partial_import_modified doesn't. Not sure when that broke.

Should we open an issue for this and move the discussion there?

[efiop]

@dberenbaum That makes sense, I don't think we ever supported that in the past, but now we totally can through the same mechanism that we use for cloud versioning. I'll introduce metadata valiadation for non cloud versioned file storage in dvc-data and will update what's necessary in #10220 on top of your tests. Thank you!

[dberenbaum]

@efiop Are you sure it was never supported? Maybe it was specific to storages that have etags? This is what it says in #8024 (comment):

Running dvc pull downloads the partially-imported data, raising an error if the etag changed and also updates the .dvc file with outs and stage metadata.

[efiop]

@dberenbaum I'll be sure to double check.