Skip to content
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

[WIP] gs: fix import-url for directories #7655

Closed
wants to merge 1 commit into from

Conversation

dtrifiro
Copy link
Contributor

fixes #7638

@dtrifiro dtrifiro requested a review from a team as a code owner April 28, 2022 10:28
@dtrifiro dtrifiro requested review from efiop and skshetry April 28, 2022 10:28
@dtrifiro dtrifiro changed the title fix import-url for directories gs: fix import-url for directories Apr 28, 2022
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be catching this external data stuff in the cloud tests (https://github.com/iterative/dvc-gs).
Seems that we are not really testing external data at all?
Similar thing happened in #7563

@efiop
Copy link
Contributor

efiop commented Apr 28, 2022

@daavoo Indeed, we specifically skip import_dir test in https://github.com/iterative/dvc-gs/blob/564592e28be38c8d45f7c04f0b07d0065a65f1b2/dvc_gs/tests/test_dvc.py#L39 I think this is for legacy reasons from way back when we used etags for dir hashes. We now use regular md5s, so might be able to make it work for gs.

@efiop
Copy link
Contributor

efiop commented Apr 28, 2022

@dtrifiro The testing for gs is a bit complicated, because we are in the middle of creating plugins, so the tests run in https://github.com/iterative/dvc-gs (they have the creds and stuff there). If you don't happen to have set it up before, you could create a test PR in dvc-gs and make it install dvc from your fork and run the tests there, just to confirm that it works before merging this.

@dtrifiro dtrifiro marked this pull request as draft April 28, 2022 11:22
@dtrifiro
Copy link
Contributor Author

dtrifiro commented Apr 28, 2022

Seems there are some failures. Will mark this PR as draft and investigate later

@dtrifiro dtrifiro changed the title gs: fix import-url for directories [WIP] gs: fix import-url for directories Apr 28, 2022
@dberenbaum
Copy link
Collaborator

We should be catching this external data stuff in the cloud tests (https://github.com/iterative/dvc-gs).
Seems that we are not really testing external data at all?
Similar thing happened in #7563

Should we create a separate issue for this?

@daavoo
Copy link
Contributor

daavoo commented Jun 1, 2022

This needs to be moved to https://github.com/iterative/dvc-objects

dvc/fs/__init__.py Outdated Show resolved Hide resolved
@dtrifiro
Copy link
Contributor Author

dtrifiro commented Jun 8, 2022

Closing this as this was merged in iterative/dvc-objects#41. Just need to wait for dvc-data to be bumped

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

Successfully merging this pull request may close these issues.

import-url: importing directory from GCS bucket downloads to parent directory
4 participants