use latest config in S3Path's unless passed explicitly #175

ericphanson · 2021-07-19T21:21:29Z

Closes #168

needs tests
decide if its breaking (edit: likely should be breaking to ensure there aren't subtle errors downstream, even though it's somewhat unlikely this behavior is being relied on)

As an aside, I think this plus #161 will make S3Paths very ergonomic for reproducible usage, e.g.

module MyModellingPackage

const MyDataSet = S3Path(bucket, key; version)

...fun modelling work...

end # module

Then, since it's hardcoded, bumping the dataset means making a new commit, so the code and data versions are always tied for reproducibility (assuming you commit a manifest of course).

ericphanson · 2021-07-19T21:21:41Z

bors try

bors · 2021-07-19T21:23:18Z

try

Build failed:

Julia 1 - ubuntu-latest - x64

Project.toml

src/AWSS3.jl

ericphanson · 2021-07-19T21:36:43Z

bors try

bors · 2021-07-19T21:41:53Z

try

Build succeeded:

ericphanson · 2021-07-19T21:50:01Z

No tests failed, which is a bit encouraging that it's not breaking. Basically, the case that will fail is:

global_aws_config(; arguments_1...)
path = S3Path(bucket, key) # only valid with `arguments_1`

global_aws_config(; arguments_2...)
read(path) # works on master/release; fails on this PR because it picks up the new config

On release/master, path will hold the config with arguments_1 so this will work, whereas with this PR, it will pick up the latest global config (with arguments_2) and fail.

However, if you pass a config like

config = global_aws_config(; arguments_1...)
path = S3Path(bucket, key; config) # only valid with `arguments_1`

global_aws_config(; arguments_2...)
read(path) # still works

then it will "freeze" that config in like it does now. So it's only about the default when you don't pass a config explicitly.

I think the current behavior is surprising/unintuitive; most users expect a "path" to just be a stateless indicator/pointer/etc to an asset, not something stateful that depends on when it was created. Therefore, I think there's a decent argument to be made that this is a bugfix and likely anyone who wanted to create their paths with a particular config (different from the global one they'd be using later) would already be passing a config explictly. However, given AWSS3 likely has a large set of users, I also wouldn't be that surprised if someone was relying on freezing the global config in a path and then changing the global config.

If anyone has any thoughts about whether this should be a breaking release or not please share them. I think we shouldn't be too afraid to make breaking releases that improve the semantics, however, especially since we are pre-1.0 anyway, so I would be fine with just doing the breaking version bump here.

ericphanson · 2021-07-19T22:54:54Z

bors try

bors · 2021-07-19T22:58:12Z

try

Build failed:

Julia 1 - ubuntu-latest - x64

ericphanson · 2021-07-19T23:01:46Z

bors try

bors · 2021-07-19T23:07:55Z

try

Build succeeded:

ExpandingMan · 2021-07-19T23:20:06Z

In my opinion this should be a breaking change because the default behavior has changed. Had you left the defaults alone, it would be non-breaking.

On second thought, it makes me a little nervous to change the default at all... it seems reasonable to expect S3Path not to have to suffer the overhead of making this additional request every time it is used... Not sure if anyone has any strong opinions about it, but after a few moments of reflection my instinct would be not to make this a new default.

ericphanson · 2021-07-19T23:24:27Z

There is no additional request; once the global config has been set once it is just deferencing a ref: https://github.com/JuliaCloud/AWS.jl/blob/76619b840682cea50c4069f89e2e36ee6f63b802/src/AWS.jl#L44-L62

ExpandingMan · 2021-07-19T23:31:12Z

Clearly I did not read this carefully enough. That sounds great, I suppose this is non-breaking then. Might be worth waiting for someone more lucid than I to chime in before merging. I'm not sure if this will break anything for anyone, but I suspect that if it does it will be in a subtle and infuriating way.

ericphanson · 2021-07-19T23:43:07Z

Ok, I think breaking is a reasonable choice then. That will help make sure no one is caught off guard by this change, and those relying on the previous behavior will have the opportuntity to update (by passing in the config as e.g. changing S3Path(bucket, key) to S3Path(bucket, key; config = global_aws_config())).

To help justify why I think this change is still worth a breaking release: the current behavior can be pretty unexpected, and if you aren't in us-east-1 which is the default region, the wrong config means 301 errors when you try to use the path. Users often don't expect the config to be "baked in", so they are confused when they create a path (maybe at precompile time), then set the region (maybe at __init__ time), then use the path (maybe at runtime), and then get an error. Region is just one example; any other choice requiring a custom config will be similar. Additionally, one can always pass a config object directly in order to recover the previous behavior.

rofinn · 2021-07-20T04:08:46Z

Hmm, I wonder if this would be better handled in AWS.jl with a LazyAWSConfig <: AbstractAWSConfig type? Introducing that change wouldn't be breaking to either package and would allow you to use that config outside of the S3Path context. Thoughts?

ericphanson · 2021-07-20T10:39:38Z

Hm, interesting idea. Are there other objects that "bake in" the config like this? Most of what I've used AWS.jl for has been with S3Paths or explicit API calls like

@service CloudWatch_Logs
CloudWatch_Logs.describe_log_streams(log_group, params)

which gets the latest config automatically (or you can pass one with aws_config).

The other concern I have with that idea is that I don't think it would work to support this kind of use-case:

module MyModellingPackage

const MyDataSet = S3Path(bucket, key)

function __init__()
    global_aws_config(; region="us-east-2")
    return nothing
end

...fun modelling work...

end # module

Here, the S3Path is created at precompilation time, which means it bakes in the wrong region (since the "right" region in this scenario is set at __init__ time). If we created a LazyAWSConfig, we could set it in __init__, but it's too late at that point, since the S3Path is created at precompile time with the wrong config.

omus

Overall I agree with being able to decouple an S3Path from a specific AWS configuration. One thought on this: is it useful to have an S3Path with an embedded config? I think the dynamic behaviour allowed by nothing here is probably what most people want. I suspect including the config as field was done mainly as a convenience to avoid having to pass in a configuration when making a call such as stat(::S3Path).

This makes me think if we're making a breaking change anyway possibly we should remove the config field from S3Path and allow passing in an alternate config when one is actually used.

omus · 2021-07-21T14:08:22Z

src/s3path.jl

@@ -1,4 +1,4 @@
-struct S3Path{A<:AbstractAWSConfig} <: AbstractPath
+struct S3Path{A<:Union{Nothing, AbstractAWSConfig}} <: AbstractPath


Shouldn't be breaking but there may be some incompatibilities with packages that dispatch on S3Path{<:AbstractAWSConfig}

src/s3path.jl

src/AWSS3.jl

rofinn · 2021-07-21T15:27:46Z

@omus Removing the config field defeats the point of the type though. At that point you'll need to know it's an S3Path at the call-site which removes the entire benefit of this type. I still think we could replace the config fields with a Function that returns a config. If folks want to memoize that function for efficiency they can.

omus · 2021-07-21T15:33:38Z

I'm definitely not going to push on removing config as part of this PR as it'll most definitely cause this to stall out. We can discuss the idea further in the original issue: #168

bors · 2021-08-13T17:16:06Z

try

Build succeeded:

src/s3path.jl

christopher-dG · 2021-08-16T16:14:19Z

@ericphanson 0.8.8 is now out, so this can move forward. needs some conflict resolution to start.

ericphanson · 2021-08-16T16:15:53Z

very exciting! if your schedule allows it please feel free to pick this up, otherwise I will try to get to it soon

christopher-dG · 2021-08-17T14:58:50Z

bors try

bors · 2021-08-17T15:01:17Z

try

Build failed:

Julia 1 - ubuntu-latest - x64

christopher-dG · 2021-08-17T15:27:42Z

bors try

bors · 2021-08-17T15:30:54Z

try

Build failed:

Julia 1 - ubuntu-latest - x64

christopher-dG · 2021-08-17T15:38:24Z

bors try

bors · 2021-08-17T15:44:40Z

try

Build succeeded:

christopher-dG · 2021-08-17T15:57:11Z

Tests are passing. Not sure what else needs to be done for this, if anything.

mattBrzezinski

LGTM!

ericphanson

last changes looks good to me, thanks @christopher-dG ! Not sure how we want to do merging for v0.9, there's a few breaking items on the milestone so it would be good to get them in together. Should we just call master v0.9 and start merging stuff in? (And not tag a release until the milestone is completed? We can of course still backport stuff on a branch if something comes up).

omus

LGTM

src/s3path.jl

test/s3path.jl

omus · 2021-08-17T16:35:13Z

Not sure how we want to do merging for v0.9, there's a few breaking items on the milestone so it would be good to get them in together. Should we just call master v0.9 and start merging stuff in? (And not tag a release until the milestone is completed? We can of course still backport stuff on a branch if something comes up).

I would just merge the changes we want to be in v0.9 into master, bump the version, then tag a release.

Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>

christopher-dG · 2021-08-17T19:24:16Z

bors r+

bors · 2021-08-17T19:29:08Z

Build succeeded:

use latest config in S3Path's unless passed explicitly

1c5829d

bors bot added a commit that referenced this pull request Jul 19, 2021

Try #175:

a2c848e

add Compat

537c546

ericphanson commented Jul 19, 2021

View reviewed changes

Project.toml Show resolved Hide resolved

src/AWSS3.jl Outdated Show resolved Hide resolved

bors bot added a commit that referenced this pull request Jul 19, 2021

Try #175:

97e8cd0

add test

4341048

bors bot added a commit that referenced this pull request Jul 19, 2021

Try #175:

c701113

fix test

82d3416

bors bot added a commit that referenced this pull request Jul 19, 2021

Try #175:

caa01b5

ericphanson requested review from rofinn and omus July 19, 2021 23:43

omus reviewed Jul 21, 2021

View reviewed changes

omus mentioned this pull request Jul 21, 2021

S3Paths: get config at use-time, not construction time? #168

Closed

omus reviewed Aug 13, 2021

View reviewed changes

src/s3path.jl Outdated Show resolved Hide resolved

src/s3path.jl Outdated Show resolved Hide resolved

src/s3path.jl Outdated Show resolved Hide resolved

src/s3path.jl Show resolved Hide resolved

ericphanson mentioned this pull request Aug 16, 2021

version_id helper function? #189

Open

ericphanson mentioned this pull request Aug 17, 2021

fix exists functions for directory on AWS S3 #194

Merged

christopher-dG added 2 commits August 17, 2021 11:54

Merge branch 'master' into eph/default-config-nothing

9e4a8f4

S3PathConfig -> AbstractS3PathConfig

88cfe4d

bors bot added a commit that referenced this pull request Aug 17, 2021

Try #175:

ae9e584

christopher-dG added 2 commits August 17, 2021 12:04

format

3882c95

Fix constructor argument order, I think?

c95b8d2

bors bot added a commit that referenced this pull request Aug 17, 2021

Try #175:

20cbf58

Fix a bad merge

09849c3

bors bot added a commit that referenced this pull request Aug 17, 2021

Try #175:

0cdaa80

mattBrzezinski approved these changes Aug 17, 2021

View reviewed changes

ericphanson commented Aug 17, 2021

View reviewed changes

omus approved these changes Aug 17, 2021

View reviewed changes

src/s3path.jl Outdated Show resolved Hide resolved

test/s3path.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

eed96a7

Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>

bors bot merged commit 4a3b7a4 into JuliaCloud:master Aug 17, 2021

omus deleted the eph/default-config-nothing branch August 18, 2021 14:31

		@@ -1,4 +1,4 @@
		struct S3Path{A<:AbstractAWSConfig} <: AbstractPath
		struct S3Path{A<:Union{Nothing, AbstractAWSConfig}} <: AbstractPath

use latest config in S3Path's unless passed explicitly #175

use latest config in S3Path's unless passed explicitly #175

Conversation

ericphanson commented Jul 19, 2021 • edited

ericphanson commented Jul 19, 2021

bors bot commented Jul 19, 2021

try

ericphanson commented Jul 19, 2021

bors bot commented Jul 19, 2021

try

ericphanson commented Jul 19, 2021

ericphanson commented Jul 19, 2021

bors bot commented Jul 19, 2021

try

ericphanson commented Jul 19, 2021

bors bot commented Jul 19, 2021

try

ExpandingMan commented Jul 19, 2021

ericphanson commented Jul 19, 2021

ExpandingMan commented Jul 19, 2021

ericphanson commented Jul 19, 2021

rofinn commented Jul 20, 2021

ericphanson commented Jul 20, 2021

omus left a comment

Choose a reason for hiding this comment

omus Jul 21, 2021

Choose a reason for hiding this comment

rofinn commented Jul 21, 2021 • edited

omus commented Jul 21, 2021

bors bot commented Aug 13, 2021

try

christopher-dG commented Aug 16, 2021

ericphanson commented Aug 16, 2021

christopher-dG commented Aug 17, 2021

bors bot commented Aug 17, 2021

try

christopher-dG commented Aug 17, 2021

bors bot commented Aug 17, 2021

try

christopher-dG commented Aug 17, 2021

bors bot commented Aug 17, 2021

try

christopher-dG commented Aug 17, 2021

mattBrzezinski left a comment

Choose a reason for hiding this comment

ericphanson left a comment

Choose a reason for hiding this comment

omus left a comment

Choose a reason for hiding this comment

omus commented Aug 17, 2021

christopher-dG commented Aug 17, 2021

bors bot commented Aug 17, 2021

ericphanson commented Jul 19, 2021 •

edited

rofinn commented Jul 21, 2021 •

edited