Skip to content

dvcfs: add basic fsid #9923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 7, 2023
Merged

dvcfs: add basic fsid #9923

merged 2 commits into from
Sep 7, 2023

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Sep 7, 2023

Implements fsspec's fsid fsspec/filesystem_spec#1122

Note that this is not rigid and we can change it up as we go.

Depends on iterative/dvc-data#436
Fixes #9904

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
dvc/fs/dvc.py 100.00%

📢 Thoughts on this report? Let us know!.

@efiop efiop added bugfix fixes bug feature is a feature and removed bugfix fixes bug labels Sep 7, 2023
@efiop efiop merged commit 784f5a5 into iterative:main Sep 7, 2023
@shcheklein
Copy link
Member

Getting this here from Slack for visibility and to not mix conversations.

Curious if this can and should be tested.

@efiop
Copy link
Contributor Author

efiop commented Sep 8, 2023

@shcheklein If you are talking about the linked issue then no, this is not a dvc-level thing, this is a dvc-data/fs level logic fault, there is no simple good test for that (... that I can see on the spot).

@shcheklein
Copy link
Member

@shcheklein If you are talking about the linked issue then no, this is not a dvc-level thing, this is a dvc-data/fs level logic fault, there is no simple good test for that.

A unit test on the dvc-data/fs side? That we identify / dedup FS instances by specific fields? (id vs protocol + something)?

@efiop
Copy link
Contributor Author

efiop commented Sep 8, 2023

@shcheklein I don't think it is necessary right now.

@efiop efiop mentioned this pull request Sep 11, 2023
@efiop
Copy link
Contributor Author

efiop commented Sep 11, 2023

Added tests in #9934 and caught noscm bug 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dvc import: imports the same file twice from 2 different repos
2 participants