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

gs: add {_strip/unstrip}_protocol #41

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Jun 6, 2022

fixes resolution of relative paths (iterative/dvc#7638)

fixes #38

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2022

Codecov Report

Merging #41 (9c92c56) into main (8b44ac4) will increase coverage by 0.77%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   47.00%   47.77%   +0.77%     
==========================================
  Files          46       46              
  Lines        2517     2545      +28     
  Branches      307      307              
==========================================
+ Hits         1183     1216      +33     
+ Misses       1311     1306       -5     
  Partials       23       23              
Impacted Files Coverage Δ
src/dvc_objects/fs/implementations/gs.py 60.86% <50.00%> (-3.84%) ⬇️
tests/fs/test_system.py 100.00% <0.00%> (ø)
src/dvc_objects/fs/system.py 83.82% <0.00%> (+11.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b44ac4...9c92c56. Read the comment docs.

@dtrifiro dtrifiro marked this pull request as ready for review June 7, 2022 10:37
@dtrifiro dtrifiro marked this pull request as draft June 7, 2022 10:50
fixes resolution of relative paths

fixes iterative/dvc#7638
@dtrifiro dtrifiro marked this pull request as ready for review June 7, 2022 15:20
@dtrifiro dtrifiro changed the title gs: add _strip_protocol gs: add {_strip/unstrip}_protocol Jun 7, 2022
@dtrifiro dtrifiro requested a review from skshetry June 7, 2022 16:06
@efiop
Copy link
Contributor

efiop commented Jun 7, 2022

@dtrifiro Could you please explain the issue and the solution? There is no info on it in the issue, so I'm trying to make sure I understand it correctly

@dtrifiro
Copy link
Contributor Author

dtrifiro commented Jun 7, 2022

Since we were not stripping protocol for gs, the from_info in the snippet below started with gs://

to_infos = [
localfs.path.join(
to_info, *self.path.relparts(info, from_info)
)
for info in from_infos
]

this resulted in relparts looking like this ('..', '..', '..', 'import-url-failing-test', 'directory', 'bar') when calling dvc import-url gs://import-url-failing-test/directory ( with directory containing bar) thus ending up writing the importer directory in a a directory starting with ../../../

@efiop
Copy link
Contributor

efiop commented Jun 7, 2022

@dtrifiro I guess it raises another question: should fs.path be able to handle full urls? This is an aspect of fsspec path handling that I'm not sure about. On the one hand it is handy, but feels like path normalisation would be nice to default to. So maybe fs.path should need normalize the path, similar to how _strip_protocol does. This is just a thought though. Your fix is perfectly valid for now.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Might want to check other filesystems too. Might have to come back to this later.

@efiop efiop merged commit 1cca0ca into iterative:main Jun 7, 2022
@dtrifiro dtrifiro deleted the fix-import-url-dir branch June 8, 2022 08:16
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
3 participants