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.

Sign up for GitHub

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

Jump to bottom

enable S3Paths to be (de)serialized to/from Arrow #231

Merged
merged 10 commits into from
Feb 7, 2022
Merged

enable S3Paths to be (de)serialized to/from Arrow #231

merged 10 commits into from
Feb 7, 2022

Conversation

jrevels
Copy link
Contributor

@jrevels jrevels commented Feb 1, 2022 •
edited
Loading

closes #184

This adds ArrowTypes as a lightweight dependency, which is hopefully okay.

@ericphanson
Copy link
Member

let's see if bors is working:

bors try

bors bot added a commit that referenced this pull request Feb 1, 2022
@bors
Try #231:
e5cd9ee
jrevels
jrevels commented Feb 1, 2022
View reviewed changes
src/s3path.jl Outdated Show resolved Hide resolved
jrevels
jrevels commented Feb 1, 2022
View reviewed changes
test/s3path.jl Outdated Show resolved Hide resolved
omus
omus reviewed Feb 2, 2022
View reviewed changes
test/s3path.jl Outdated Show resolved Hide resolved
test/s3path.jl Outdated Show resolved Hide resolved
omus
omus reviewed Feb 2, 2022
View reviewed changes
src/s3path.jl Outdated Show resolved Hide resolved
@ericphanson
Copy link
Member

do you know what's wrong with bors by any chance @mattBrzezinski ? Or are the tests themselves hanging now?

@omus
Copy link
Member

omus commented Feb 3, 2022

bors try-

@omus
Copy link
Member

omus commented Feb 3, 2022

bors try

bors bot added a commit that referenced this pull request Feb 3, 2022
@bors
Try #231:
9845f0d
@omus
Copy link
Member

omus commented Feb 3, 2022

I'll try triggering bors again

omus
omus reviewed Feb 3, 2022
View reviewed changes
test/s3path.jl Outdated Show resolved Hide resolved
omus
omus reviewed Feb 3, 2022
View reviewed changes
src/s3path.jl Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors bot commented Feb 3, 2022

try

Timed out.

@omus
Copy link
Member

omus commented Feb 4, 2022

bors try

@omus
Copy link
Member

omus commented Feb 4, 2022

bors try-

@omus
Copy link
Member

omus commented Feb 4, 2022

bors try

@omus
Copy link
Member

omus commented Feb 7, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @jrevels @omus
Merge #231
c0b254f 
231: enable S3Paths to be (de)serialized to/from Arrow r=omus a=jrevels

closes #184 

This adds ArrowTypes as a lightweight dependency, which is hopefully okay. 

Co-authored-by: Jarrett Revels <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

Build failed:

@omus omus mentioned this pull request Feb 7, 2022
Merged
bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @omus
Merge #236
fc96007 
236: Escape unused DateFormat characters r=omus a=omus

Required to fix errors when running test suite with #231:

```
  Got exception outside of a `@test`
  type DateTime has no field zone
  Stacktrace:
    [1] getproperty
      @ ./Base.jl:42 [inlined]
    [2] format(io::IOBuffer, d::Dates.DatePart{'Z'}, zdt::DateTime, locale::Dates.DateLocale)
      @ TimeZones ~/.julia/packages/TimeZones/1ALMR/src/parse.jl:81
    [3] macro expansion
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Dates/src/io.jl:634 [inlined]
    [4] format(io::IOBuffer, dt::DateTime, fmt::DateFormat{:yyyymmddTHHMMSSZ, Tuple{Dates.DatePart{'y'}, Dates.DatePart{'m'}, Dates.DatePart{'d'}, Dates.Delim{Char, 1}, Dates.DatePart{'H'}, Dates.DatePart{'M'}, Dates.DatePart{'S'}, Dates.DatePart{'Z'}}})
      @ Dates /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Dates/src/io.jl:629
    [5] format(dt::DateTime, fmt::DateFormat{:yyyymmddTHHMMSSZ, Tuple{Dates.DatePart{'y'}, Dates.DatePart{'m'}, Dates.DatePart{'d'}, Dates.Delim{Char, 1}, Dates.DatePart{'H'}, Dates.DatePart{'M'}, Dates.DatePart{'S'}, Dates.DatePart{'Z'}}}, bufsize::Int64)
      @ Dates /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Dates/src/io.jl:641
    [6] format(dt::DateTime, fmt::DateFormat{:yyyymmddTHHMMSSZ, Tuple{Dates.DatePart{'y'}, Dates.DatePart{'m'}, Dates.DatePart{'d'}, Dates.Delim{Char, 1}, Dates.DatePart{'H'}, Dates.DatePart{'M'}, Dates.DatePart{'S'}, Dates.DatePart{'Z'}}})
      @ Dates /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Dates/src/io.jl:640
    [7] format(dt::DateTime, f::String; locale::Dates.DateLocale)
      @ Dates /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Dates/src/io.jl:680
    [8] format
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Dates/src/io.jl:680 [inlined]
    [9] awss3_tests(config::MinioConfig)
      @ Main ~/work/AWSS3.jl/AWSS3.jl/test/awss3.jl:2
   [10] macro expansion
      @ ~/work/AWSS3.jl/AWSS3.jl/test/runtests.jl:43 [inlined]
   [11] macro expansion
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
   [12] macro expansion
      @ ~/work/AWSS3.jl/AWSS3.jl/test/runtests.jl:43 [inlined]
   [13] macro expansion
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
   [14] top-level scope
      @ ~/work/AWSS3.jl/AWSS3.jl/test/runtests.jl:23
   [15] include(fname::String)
      @ Base.MainInclude ./client.jl:451
   [16] top-level scope
      @ none:6
   [17] eval
      @ ./boot.jl:373 [inlined]
   [18] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:268
   [19] _start()
      @ Base ./client.jl:495
```

Co-authored-by: Curtis Vogt <[email protected]>
@omus
Copy link
Member

omus commented Feb 7, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @jrevels @omus
Merge #231
fe20b6e 
231: enable S3Paths to be (de)serialized to/from Arrow r=omus a=jrevels

closes #184 

This adds ArrowTypes as a lightweight dependency, which is hopefully okay. 

Co-authored-by: Jarrett Revels <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

Build failed:

@omus
Copy link
Member

omus commented Feb 7, 2022

There was another improper use of DateFormat I missed. Addressed in #237

@omus
Copy link
Member

omus commented Feb 7, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @jrevels @omus
Merge #231
a2e58b3 
231: enable S3Paths to be (de)serialized to/from Arrow r=omus a=jrevels

closes #184 

This adds ArrowTypes as a lightweight dependency, which is hopefully okay. 

Co-authored-by: Jarrett Revels <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

Build failed:

@omus
Copy link
Member

omus commented Feb 7, 2022

Related failure:

Arrow <-> S3Path (de)serialization: Error During Test at /home/runner/work/AWSS3.jl/AWSS3.jl/test/s3path.jl:436
  Got exception outside of a @test
  MethodError: Cannot `convert` an object of type 
    S3Path{AWSConfig} to an object of type 
    S3Path{Nothing}
  Closest candidates are:
    convert(::Type{<:AbstractPath}, ::AbstractString) at ~/.julia/packages/FilePathsBase/KUHsP/src/path.jl:116
    convert(::Type{T}, ::T) where T at /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/base/essentials.jl:218
    S3Path{A}(::Any, ::Any, ::Any, ::Any, ::Any, ::A) where A<:Union{Nothing, AbstractAWSConfig} at ~/work/AWSS3.jl/AWSS3.jl/src/s3path.jl:13
  Stacktrace:
    [1] push!(a::Vector{S3Path{Nothing}}, item::S3Path{AWSConfig})
      @ Base ./array.jl:994
    [2] macro expansion
      @ ~/work/AWSS3.jl/AWSS3.jl/test/s3path.jl:445 [inlined]
    [3] macro expansion
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
    [4] s3path_tests(config::AWSConfig)
      @ Main ~/work/AWSS3.jl/AWSS3.jl/test/s3path.jl:437
    [5] macro expansion
      @ ~/work/AWSS3.jl/AWSS3.jl/test/runtests.jl:56 [inlined]
    [6] macro expansion
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
    [7] macro expansion
      @ ~/work/AWSS3.jl/AWSS3.jl/test/runtests.jl:55 [inlined]
    [8] macro expansion
      @ /opt/hostedtoolcache/julia/1.7.1/x64/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
    [9] top-level scope
      @ ~/work/AWSS3.jl/AWSS3.jl/test/runtests.jl:23
   [10] include(fname::String)
      @ Base.MainInclude ./client.jl:451
   [11] top-level scope
      @ none:6
   [12] eval
      @ ./boot.jl:373 [inlined]
   [13] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:268
   [14] _start()
      @ Base ./client.jl:495

omus
omus reviewed Feb 7, 2022
View reviewed changes
test/s3path.jl Outdated Show resolved Hide resolved
@jrevels @omus
Update test/s3path.jl
cc16887 
Co-authored-by: Curtis Vogt <[email protected]>
@omus
Copy link
Member

omus commented Feb 7, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @jrevels @omus
Merge #231
204aeb4 
231: enable S3Paths to be (de)serialized to/from Arrow r=omus a=jrevels

closes #184 

This adds ArrowTypes as a lightweight dependency, which is hopefully okay. 

Co-authored-by: Jarrett Revels <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

Build failed:

omus
omus reviewed Feb 7, 2022
View reviewed changes
test/s3path.jl Outdated Show resolved Hide resolved
@omus
Copy link
Member

omus commented Feb 7, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @jrevels @omus
Merge #231
09b0314 
231: enable S3Paths to be (de)serialized to/from Arrow r=omus a=jrevels

closes #184 

This adds ArrowTypes as a lightweight dependency, which is hopefully okay. 

Co-authored-by: Jarrett Revels <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

Build failed:

@omus
Copy link
Member

omus commented Feb 7, 2022

Why can't we have nice things? Segfault issue is appearing consistently here. Based upon it not showing up on Julia 1.3 it's probably something related to Minio

@omus omus mentioned this pull request Feb 7, 2022
Closed
@omus
Copy link
Member

omus commented Feb 7, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 7, 2022
@bors @jrevels @omus
Merge #231
cb98946 
231: enable S3Paths to be (de)serialized to/from Arrow r=omus a=jrevels

closes #184 

This adds ArrowTypes as a lightweight dependency, which is hopefully okay. 

Co-authored-by: Jarrett Revels <[email protected]>
Co-authored-by: Curtis Vogt <[email protected]>
@omus
Copy link
Member

omus commented Feb 7, 2022

From some experimentation I'm assuming the segfault is just random so I'll try one more time to merge this.

@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

Build failed:

@omus
Copy link
Member

omus commented Feb 7, 2022

The experimentation in #238 was unsuccessful in reproducing this segfault. I believe the failure is unrelated to this PR so I'll manually merge this. I'll continue investigating this error if it is still occurring.

@omus omus merged commit 08ffed9 into JuliaCloud:master Feb 7, 2022
@jrevels jrevels deleted the jr/arrow branch February 7, 2022 18:23
@omus
Copy link
Member

omus commented Feb 7, 2022

Failure did not occur when merged: https://github.com/JuliaCloud/AWSS3.jl/actions/runs/1808119866

bors bot added a commit that referenced this pull request Feb 8, 2022
@bors
Try #231:
babfc88
@bors
Copy link
Contributor

bors bot commented Feb 8, 2022

try

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omus omus omus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow S3Paths to be stored in Arrow files
3 participants
@jrevels @ericphanson @omus