Skip to content

Conversation

@dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Jun 6, 2022

fixes resolution of relative paths (treeverse/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 force-pushed the fix-import-url-dir branch from b3aeade to 562418c Compare June 7, 2022 09:13
@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 treeverse/dvc#7638
@dtrifiro dtrifiro force-pushed the fix-import-url-dir branch from 562418c to 9c92c56 Compare June 7, 2022 15:18
@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://

https://github.com/iterative/dvc-objects/blob/feecfbaed390b909eb60784c5efc093a211ef06c/src/dvc_objects/fs/base.py#L475-L480

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 treeverse: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