cloud versioning: ```dvc pull``` fails after version aware import-url for non version aware s3 remote

[jolyons123]

Bug Report

Description

We have a use-case where we need to capture "metadata" of remote s3 storage and are not able to do dvc push because this would essentially end up in duplicating our storage. Old files do not change and the dataset only changes as new files come in.
Thus, I was hoping to make use of DVC to capture those metadata i.e. to manage a file in git which holds a list of files in our remote s3 bucket. Later on it should be possible to do a clean git clone and do dvc pull to fetch the data from the original (not dvc pushed) s3 bucket. (See Discord thread for more info)
This almost works with dvc import-url --version-aware s3://uri.... The generated .dvc file of the root folder contains all the s3 paths of the files inside the root folder.
However, upon cleanly checking out the git repo which is holding the dvc metadata and doing dvc pull there is an assertion error. (Note that the original s3 bucket is not version aware! I am only using the --version-aware option so that DVC saves all the metadata of all files inside the .dvc file instead of the .dvc/cache folder (which is not committed to git!))

Reproduce

1. git init
2. dvc init
3. dvc import-url --version-aware s3://s3-bucket-which-is-not-version-aware
4. git add .
5. git commit -m 'initial commit'
6. git clean -dfX
7. dvc pull -v

Expected

To download all the files from the original s3.

Workaround

Comment out line https://github.com/iterative/dvc-data/blob/main/src/dvc_data/index/save.py#L24

Environment information/Command output

�[32m2023-01-23 16:17:41,940�[39m �[34mDEBUG�[39m: failed to pull cache for 'XYZ'
�[32m2023-01-23 16:17:44,009�[39m �[34mDEBUG�[39m: built tree 'object b7b631ef79755d98f49ffc96bb740dea.dir'
dvc : �[32m2023-01-23 16:17:45,804�[39m �[31mERROR�[39m: unexpected error
At line:1 char:1
+ dvc pull -v 2>&1 > out.txt
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (�[32m2023-01-23...nexpected error:String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
�[31m------------------------------------------------------------�[39m
Traceback (most recent call last):
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\cli\__init__.py", line 184, in main
    ret = cmd.do_run()
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\cli\command.py", line 26, in do_run
    return self.run()
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\commands\data_sync.py", line 31, in run
    stats = self.repo.pull(
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\repo\__init__.py", line 66, in wrapper
    return f(repo, *args, **kwargs)
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\repo\pull.py", line 34, in pull
    processed_files_count = self.fetch(
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\repo\__init__.py", line 66, in wrapper
    return f(repo, *args, **kwargs)
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\repo\fetch.py", line 111, in fetch
    imported = save_imports(
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc\repo\imports.py", line 120, in save_imports
    md5(data_view)
  File "C:\ProgramData\Miniconda3\envs\dvc\lib\site-packages\dvc_data\index\save.py", line 24, in md5
    assert entry.fs
AssertionError
�[31m------------------------------------------------------------�[39m
�[32m2023-01-23 16:17:45,906�[39m �[34mDEBUG�[39m: link type reflink is not available ([Errno 129] no more link types left to try out)
�[32m2023-01-23 16:17:45,906�[39m �[34mDEBUG�[39m: Removing 'C:\Users\johnl\Arbeit2\.FAGs8ZeCNpMeFrGRKfP7S5.tmp'
�[32m2023-01-23 16:17:45,906�[39m �[34mDEBUG�[39m: Removing 'C:\Users\johnl\Arbeit2\.FAGs8ZeCNpMeFrGRKfP7S5.tmp'
�[32m2023-01-23 16:17:45,908�[39m �[34mDEBUG�[39m: Removing 'C:\Users\johnl\Arbeit2\.FAGs8ZeCNpMeFrGRKfP7S5.tmp'
�[32m2023-01-23 16:17:45,908�[39m �[34mDEBUG�[39m: Removing 'C:\Users\johnl\Arbeit2\DVC3\.dvc\cache\.VsKTYWSQigtDM6pyQ4F5cw.tmp'
�[32m2023-01-23 16:17:45,911�[39m �[34mDEBUG�[39m: Version info for developers:
DVC version: 2.43.1 
---------------------------------
Platform: Python 3.9.16 on Windows-10-10.0.22621-SP0
Subprojects:
	dvc_data = 0.35.1
	dvc_objects = 0.19.0
	dvc_render = 0.0.17
	dvc_task = 0.1.11
	dvclive = 1.3.2
	scmrepo = 0.1.6
Supports:
	http (aiohttp = 3.8.3, aiohttp-retry = 2.8.3),
	https (aiohttp = 3.8.3, aiohttp-retry = 2.8.3),
	s3 (s3fs = 2023.1.0, boto3 = 1.24.59)
Cache types: hardlink, symlink
Cache directory: NTFS on C:\
Caches: local
Remotes: None
Workspace directory: NTFS on C:\
Repo: dvc, git

�[33mHaving any troubles?�[39m Hit us up at �[34mhttps://dvc.org/support�[39m, we are always happy to help!
�[32m2023-01-23 16:17:45,913�[39m �[34mDEBUG�[39m: Analytics is enabled.
�[32m2023-01-23 16:17:46,070�[39m �[34mDEBUG�[39m: Trying to spawn '['daemon', '-q', 'analytics', 'C:\\Users\\johnl\\AppData\\Local\\Temp\\tmpu8q07j2a']'
�[32m2023-01-23 16:17:46,073�[39m �[34mDEBUG�[39m: Spawned '['daemon', '-q', 'analytics', 'C:\\Users\\johnl\\AppData\\Local\\Temp\\tmpu8q07j2a']'

[dberenbaum]

@pmrowla It seems like #8249 didn't address pulling imported directories from source since we would at least need the .dir file in that case. Maybe when importing a directory we should be doing the same as in #8849 and skipping the .dir file? cc @daavoo

[daavoo]

Maybe when importing a directory we should be doing the same as in #8849 and skipping the .dir file?

Not sure I follow. Is this a feature request to support import --version-aware behavior (save as files) for buckets without cloud versioning?

[dberenbaum]

Yes, that's basically what I understood the discord discussion and this description:

We have a use-case where we need to capture "metadata" of remote s3 storage and are not able to do dvc push because this would essentially end up in duplicating our storage.

If they don't want to dvc push, we need some way to still be able to pull that dataset later from a freshly cloned repo. It could also be solved with something like pushing only the .dir file (although that assumes they have some remote to push to), but I don't see how it's possible now.

[jolyons123]

Not sure I follow. Is this a feature request to support import --version-aware behavior (save as files) for buckets without cloud versioning?

Sorry for the misleading title. But yes, this is rather a feature request than a bug.

Yes, that's basically what I understood the discord discussion and this description:

We have a use-case where we need to capture "metadata" of remote s3 storage and are not able to do dvc push because this would essentially end up in duplicating our storage.

If they don't want to dvc push, we need some way to still be able to pull that dataset later from a freshly cloned repo. It could also be solved with something like pushing only the .dir file (although that assumes they have some remote to push to), but I don't see how it's possible now.

We are able to do dvc push but only lightweight files and not the copy of the original files.

Maybe there is an easy solution for that? Because when skipping the assert the download actually works.

[pmrowla]

It seems like #8249 didn't address pulling imported directories from source since we would at least need the .dir file in that case.

The .dir file isn't supposed to be relevant at all for imported directories with import-url --version-aware (since we already generate the files listing in the deps section for the import). Nothing in the traceback indicates the problem is related to a .dir file, the error occurs when we try to compute the directory hash for the newly downloaded files.

Maybe there is an easy solution for that? Because when skipping the assert the download actually works.

Skipping the assert isn't safe, that field is supposed to be filled and other parts of dvc-data will break if it is not filled by that function call (even if removing the assert happens to work in your case). Mixing --version-aware behavior with non-versioned filesystems (buckets) is probably also causing issues, but the bug here is whatever is generating an entry without a valid fs field.

[pmrowla]

The error here is a bug, but I'm not sure this is a use case we should be trying to support (using --version-aware flags with non-versioned buckets).

@dberenbaum if we want to consider generating files for all --import-url cases (regardless of whether or not the user uses --version-aware) we can do that, but suggesting --version-aware in non-versioned scenarios is a hack and if it works at all in specific scenarios it is unintentional, and is not guaranteed to continue working in the future (and will likely break as soon as some implementation detail in dvc-data gets updated for any unrelated bugfix).

There are other things in DVC that will not work as expected here, for one thing the checks for whether or not a data source has changed probably won't work properly. Normally we check etags for an import-url dependency to see if anything has changed, but in version-aware contexts we use the version ID instead since it is faster in a lot of cases to just check whether or not the versioned URL for an object exists instead of actually getting an etag from S3 or azure. (Versioned objects are immutable, so we can use the versioned URLs in place of etags)

However, in this case mixing non-versioned filesystems will probably break this behavior (since we have None version IDs, and will end up just checking whether or not the unversioned file URL exists, which will always be True regardless of whether or not that file has changed.

[dberenbaum]

@dberenbaum if we want to consider generating files for all --import-url cases (regardless of whether or not the user uses --version-aware) we can do that

Yes, this is what I meant. What I suggested in https://discord.com/channels/485586884165107732/1067063386296291328/1067080951131295827 was that he shouldn't need --version-aware. However, I realized later why he did so: the scenario in #8274 to pull imports from source only works for files, not directories.

[dberenbaum]

@jolyons123 Can you confirm that this works for you in the latest release?

The error here is a bug, but I'm not sure this is a use case we should be trying to support (using --version-aware flags with non-versioned buckets).

Long term, I agree with @pmrowla that we should enforce that the bucket is version-aware and fail here like in #8949, but until we have an alternative that works for this scenario (like #8884), it might be better to leave it as is.