beacon-biosignals / OndaEDF.jl

utilities for importing/exporting EDF Files to/from Onda datasets
Other
3 stars 2 forks source link

Directory creation failure with `S3Path` and `store_edf_as_onda` #40

Closed omus closed 2 years ago

omus commented 2 years ago

When passing in an S3Path as the onda_dir into store_edf_as_onda an error will occur if the samples subdirectory does not exist:

ERROR: ArgumentError: S3Path folders must end with '/': s3://bucket/prefix/file.edf/samples
Stacktrace:
  [1] mkdir(fp::AWSS3.S3Path{Nothing}; recursive::Bool, exist_ok::Bool)
    @ AWSS3 ~/.julia/packages/AWSS3/bwC3m/src/s3path.jl:401
  [2] mkpath
    @ ~/.julia/packages/FilePathsBase/kMipS/src/aliases.jl:17 [inlined]
  [3] store_edf_as_onda(edf::EDF.File{IOBuffer}, onda_dir::AWSS3.S3Path{Nothing}, recording_uuid::Base.UUID; custom_extractors::Vector{OndaEDF.var"#2#4"{_A, Vector{String}} where _A}, import_annotations::Bool, postprocess_samples::typeof(TemporarySpikeRunner.correct_channel_names!), signals_prefix::String, annotations_prefix::String)
    @ OndaEDF ~/.julia/packages/OndaEDF/Z5Aq7/src/import_edf.jl:380
...

In this example p"s3://bucket/prefix/file.edf/" was passed in. The creation of the subdirectory is handled by store_edf_as_onda and we should add `* '/`` when creating the samples dir to have this code work generically.

omus commented 2 years ago

~The work around to this problem is to pre-create the samples subdir prior to calling store_edf_as_onda~

Update: I am wrong about this, the mkpath call will just continue to fail...

jrevels commented 2 years ago

if you're using S3 it might also be good to not use the high-level convenience functions (meant more for scripts/interactive work) but instead use the more primitive API functions in composition with the save/load functions you want, e.g.

https://beacon-biosignals.github.io/OndaEDF.jl/stable/#OndaEDF.edf_to_onda_samples

IIRC i leaned toward getting rid of store_edf_as_onda because was afraid people would want to keep adding features to it instead of more simply composing with other functions 😁

omus commented 2 years ago

if you're using S3 it might also be good to not use the high-level convenience functions

Overall I agree with the sentiment of using lower-level functions and making use of composition. As I'm trying to quickly iterate through this work I'll make the fix for this here and look into refactoring my code as a follow up.