Support s3 paths #103

ericphanson · 2021-07-17T17:36:09Z

Widens IOStream types to IO and logdir::String to logdir::Any. Runs all tests with both a local path and an S3Path (via Minio.jl which uses minio to run a local s3-like server).

Closes #102

I had to change lots of * to joinpath to pass the config along correctly, and do a few other minor workarounds to get all the tests passing.

ericphanson · 2021-07-17T18:30:56Z

Test failures on v1.3 is because Minio only supports v1.5+. I made a PR to loosen that restriction: https://gitlab.com/ExpandingMan/Minio.jl/-/merge_requests/2

ExpandingMan · 2021-07-17T18:48:59Z

To be honest, I'm rather opposed to making patches to be compatible with anything as old as 1.3 on principle. Since Minio.jl is a tiny package and the changes to make it compatible with 1.3 are small, I don't plan on standing in the way of your PR if that's what it comes down to, but let me at least make a case for not doing that.

For a language as young as Julia, 1.3 seems positively ancient, and the new LTS version will likely be announced very soon and I imagine it won't be all that long before explicit support for pre-1.6 becomes significantly less common.

More importantly for this particular case, tests seem to pass perfectly fine for this package on 1.6, is there some specific reason it is capped at 1.3 and testing on it? I'd much prefer to resolve any lingering issues for tensorboard world on 1.6 than start making things backward compatible with a really old Julia version.

ericphanson · 2021-07-17T18:55:00Z

Not testing at all on Julia v1.3 here would be fine by me, but I think that’s up to the maintainers.

oxinabox

This is basically just a plain improvement to the code quality.

oxinabox · 2021-07-17T19:27:36Z

src/Loggers/LogEmbeddings.jl

@@ -65,7 +65,9 @@ function write_sprite(img_labels::AbstractArray, matrix_path::AbstractString)
    arranged_augment_square_CHW = zeros((3, sprite_size, sprite_size))
    arranged_augment_square_CHW[:, 1:size(arranged_img_CHW, 2), :] = arranged_img_CHW
    sprite_path = joinpath(matrix_path, "sprite.png")
-    save(sprite_path, colorview(RGB, arranged_augment_square_CHW))
+    open(sprite_path; write=true) do io


It is annoying the this is needed
Should an issue be openned in FileIO.jl?

So there's two issues here; one is easy (JuliaCloud/AWSS3.jl#174), the other is that ImageMagick types paths to AbstractString which is too strict (FilePathsBase PosixPaths for example are probably OK too), but it isn't fully generic because it eventually goes to

ccall((:MagickWriteImages, libwand), Cint, (Ptr{Cvoid}, Ptr{UInt8}, Cint), wand, filename, true)

in writeimage and I don't think that can handle s3 paths. Possibly ImageMagick should add another method for writeimage where unknown filename types are handled by opening a stream like we do here?

PhilipVinc · 2021-07-17T20:40:21Z

To be honest, I'm rather opposed to making patches to be compatible with anything as old as 1.3 on principle.

I feel you! I was testing TensorBoardLogger on v1.0 (judging from mails I get from time to time, I think a fair number of people use it in production environments where they work with older versions) and eventually gave up.

@ericphanson It's fine for me to test this stuff only on more recent versions.
If possible, I'd like to keep testing on older versions (1.3).
Eventually all this will be dropped, but it would be unfortunate to drop compatibility only because of testing requirements...

It should be possible to do that if you edit the Project.toml to delete the Minio dependency when running on 1.3. You can do it with some bash magic. Then hide the using Minio and it's tests behind a version check...

Does this sound feasible?

--

Widens IOStream types to IO and logdir::String to logdir::Any.

I'm mildly against adding the dependency (TBLogger already has many more than I'd like).
About the type parameters... I'm not sure if it would really be a win for performance, but would it cost anything to add those two fields as type parameters to TBLogger?
then we'd have static dispatch.

It may add a little bit compilation cost, but we could offset it by generating precompile statements later on, and has no drawbacks that I can think of.
@oxinabox do you agree?

PhilipVinc · 2021-07-17T20:42:36Z

tests seem to pass perfectly fine for this package on 1.6, is there some specific reason it is capped at 1.3 and testing on it?

Because I wanted to support the 1.0 LTS.
Eventually dropped it a few months ago and the oldest version supported became 1.3 (because of JLLs).

ericphanson · 2021-07-17T20:51:32Z

@ericphanson It's fine for me to test this stuff only on more recent versions.
If possible, I'd like to keep testing on older versions (1.3).
Eventually all this will be dropped, but it would be unfortunate to drop compatibility only because of testing requirements...

It should be possible to do that if you edit the Project.toml to delete the Minio dependency when running on 1.3. You can do it with some bash magic. Then hide the using Minio and it's tests behind a version check...

Does this sound feasible?

Ok, just gave this a try based on the stuff already in the workflow for v1.3, good idea!

About the type parameters... I'm not sure if it would really be a win for performance, but would it cost anything to add those two fields as type parameters to TBLogger?
then we'd have static dispatch.

That sounds fine to me, I think most users are probably not generating many different loggers but rather just 1 or at least 1 type, so I think there might not be too much compilation cost. Pushed a commit to do this.

PhilipVinc · 2021-07-17T20:55:25Z

@ericphanson I was more thinking of keeping Minio in project (otherwise it fails for newer versions) and literally remove the line with Minio from the project.toml file on v1.3?

grep -v '^Minio =' Project.toml > Project.tml

should take care of removing the UUID. Then to remove "Minio", from the testing deps line it should be another command.

PhilipVinc · 2021-07-17T20:56:26Z

If you remove Minio from Project.toml tests will fail for users who clone the repository on v>1.6

This reverts commit 325f50c.

ericphanson · 2021-07-17T21:14:33Z

Ok, I gave that a try

ericphanson · 2021-07-18T10:45:08Z

Tests passing 😄. Should I bump the version?

PhilipVinc · 2021-07-18T12:09:13Z

ops, I saw your comment only now.
I did it myself.

Thanks for your PR!

PhilipVinc · 2021-07-18T12:23:09Z

@ericphanson for some reason CI was passing in 1.3 the PR branch but failed as soon at it was merged in master.
It's a test that shouldn't really fail but... maybe I'm missing something.
Could you give a look?

ericphanson · 2021-07-18T12:26:08Z

Sure, let me take a look

ericphanson · 2021-07-18T12:32:41Z

That test passes for me locally on Julia 1.3, as does a full Pkg.test() (once I do the workarounds in CI to setup the environment). So I am a bit lost aha.

ericphanson · 2021-07-18T12:35:16Z

It looks like this was happening even before this PR: https://github.com/JuliaLogging/TensorBoardLogger.jl/runs/2764508008

ericphanson · 2021-07-18T16:19:49Z

I filed #105 to track the CI failure. Since it’s unrelated to this PR, can we get a release so I can use the s3 path feature downstream?

ericphanson added 2 commits July 17, 2021 18:59

widen types to accomodate s3 paths; test with them

bfdc58f

fix log_embeddings tests

1100d48

oxinabox approved these changes Jul 17, 2021

View reviewed changes

ericphanson added 3 commits July 17, 2021 22:44

test with minio only on v1.5+

cc99799

parametrize TBLogger

69bdc56

fix typo

27ae409

rm Minio from project

325f50c

ericphanson added 2 commits July 17, 2021 22:57

Revert "rm Minio from project"

0d50b0f

This reverts commit 325f50c.

try snipping minio out of the project

f36d5cb

fix bugs

4d09841

PhilipVinc approved these changes Jul 18, 2021

View reviewed changes

PhilipVinc merged commit c1e8cc8 into JuliaLogging:master Jul 18, 2021

ericphanson deleted the eph/support_s3_paths branch July 18, 2021 12:20

ericphanson mentioned this pull request Jul 18, 2021

CI failures on merge on Julia 1.3 #105

Open

ericphanson mentioned this pull request Jul 21, 2021

S3Paths: get config at use-time, not construction time? JuliaCloud/AWSS3.jl#168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support s3 paths #103

Support s3 paths #103

ericphanson commented Jul 17, 2021

ericphanson commented Jul 17, 2021

ExpandingMan commented Jul 17, 2021

ericphanson commented Jul 17, 2021

oxinabox left a comment

oxinabox Jul 17, 2021

ericphanson Jul 17, 2021 •

edited

Loading

PhilipVinc commented Jul 17, 2021

PhilipVinc commented Jul 17, 2021

ericphanson commented Jul 17, 2021

PhilipVinc commented Jul 17, 2021 •

edited

Loading

PhilipVinc commented Jul 17, 2021

ericphanson commented Jul 17, 2021

ericphanson commented Jul 18, 2021

PhilipVinc commented Jul 18, 2021

PhilipVinc commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson commented Jul 18, 2021

Support s3 paths #103

Support s3 paths #103

Conversation

ericphanson commented Jul 17, 2021

ericphanson commented Jul 17, 2021

ExpandingMan commented Jul 17, 2021

ericphanson commented Jul 17, 2021

oxinabox left a comment

Choose a reason for hiding this comment

oxinabox Jul 17, 2021

Choose a reason for hiding this comment

ericphanson Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

PhilipVinc commented Jul 17, 2021

PhilipVinc commented Jul 17, 2021

ericphanson commented Jul 17, 2021

PhilipVinc commented Jul 17, 2021 • edited Loading

PhilipVinc commented Jul 17, 2021

ericphanson commented Jul 17, 2021

ericphanson commented Jul 18, 2021

PhilipVinc commented Jul 18, 2021

PhilipVinc commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson commented Jul 18, 2021

ericphanson Jul 17, 2021 •

edited

Loading

PhilipVinc commented Jul 17, 2021 •

edited

Loading