search

uber / petastorm

Petastorm library enables single machine or distributed training and evaluation of deep learning models from datasets in Apache Parquet format. It supports ML frameworks such as Tensorflow, Pytorch, and PySpark and can be used from pure Python code.
Apache License 2.0
1.78k stars 285 forks source link

handle names like [path]/part/ #535

Closed xiaohanhuang closed 4 years ago

xiaohanhuang commented 4 years ago

gcsfs returns name as [path]/part/, and in this case. Following code will set directories to list of empty strings.

        directories = sorted([posixpath.split(x)[1]
                              for x in directories])
CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

codecov[bot] commented 4 years ago

Codecov Report

Merging #535 into master will decrease coverage by 0.03%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
- Coverage   86.14%   86.10%   -0.04%     
==========================================
  Files          87       87              
  Lines        4965     4967       +2     
  Branches      790      791       +1     
==========================================
  Hits         4277     4277              
- Misses        560      562       +2     
  Partials      128      128
Impacted Files Coverage Δ
petastorm/gcsfs_helpers/gcsfs_wrapper.py 16.66% <0.00%> (-0.84%) :arrow_down:

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 6bb3ecd...6f6ed4b. Read the comment docs.

xiaohanhuang commented 4 years ago

@selitvin Is the test coverage required? This is very minor and safe change. If not required, could you merge this change? Thanks

selitvin commented 4 years ago

Yep. Can merge.

xiaohanhuang commented 4 years ago

Yep. Can merge.

@selitvin A reminder. Thanks

xiaohanhuang commented 4 years ago

Yep. Can merge.

@selitvin Could you merge this? It is a minor fix but it is blocking and hard to find work around. Thanks

selitvin commented 4 years ago

Sure. Please ping me on slack if you see I am not responding. Sometimes some git notifications slip through holes in my inbox.