Update to new protobuf and removed workaround for hparams encoding #135
Conversation
JamieMair
changed the title
|
Tests will fail until the next protobuf release, after which I will add a compat to the PR. |
|
New protobuf was released with the fix. This PR is ready to be merged now. |
There was a problem hiding this comment.
Bottom line: looks good. No real change to the protobuf files, and removed our edits for PB functions which is great.
The effort to censor paths is commendable, but at the moment it doesn't seem to be ready.
I've pointed out a few places where the clean_file function seems to have messed up.
Since the changes are only in comments no harm done, but I don't see the point in making this change. If the paths are incorrect it would be harder to track down bugs (I imagine this is the reason for those comments).
| # Autogenerated using ProtoBuf.jl v1.0.11 on 2023-06-19T18:18:24.780 | ||
| # original file: /home/lior/.julia/dev/ProtoBuf/src/google/protobuf/wrappers.proto (proto3 syntax) | ||
| # Autogenerated using ProtoBuf.jl v1.0.11 on 2023-08-09T10:18:18.634 | ||
| # original file: proto/tensorboard/google/protobuf/wrappers.proto (proto3 syntax) |
There was a problem hiding this comment.
This censored path doesn't look quite right
| # Autogenerated using ProtoBuf.jl v1.0.11 on 2023-06-19T18:18:24.964 | ||
| # original file: /home/lior/TensorBoardLogger.jl/gen/proto/tensorboard/plugins/custom_scalar/layout.proto (proto3 syntax) | ||
| # Autogenerated using ProtoBuf.jl v1.0.11 on 2023-08-09T10:18:18.779 | ||
| # original file: proto/tensorboard/plugins/custom_scalar/tensorboard/layout.proto (proto3 syntax) |
There was a problem hiding this comment.
Here it's unclear that this file came from the gen folder, and the path seems inconsistent?
| @@ -61,5 +102,7 @@ append!(files_to_include, (process_module("tensorboard/plugins/$plugin", base_mo | |||
| # files_to_include contains all the proto files, can be used for printing and inspection | |||
| println("generated code for \n$files_to_include") | |||
|
|
|||
| clean_files(out_dir) | |||
There was a problem hiding this comment.
Let's leave this for a separate PR. This censoring is redundant and messes up file paths.
| clean_files(out_dir) | |
| # clean_files(out_dir) |
This censoring can be done with a shell script:
>>> cat file_list | xargs -I f sed -i 's|/home/lior||g' f
There was a problem hiding this comment.
Likewise the _pb.jl files can be restored to their previous version with the full path
This PR should not be merged until JuliaIO/ProtoBuf.jl#234 is through and in the next release.
The following changes are made:
gencode to use the new protobuf version, including adding some scripts to clean the autogenerated files so that it does not print the user's path into the source code.hparamsplugin, as it now has been fixed upstream.