Migrate json parser to nlohmann_json #149
Conversation
| @@ -1,6 +1,6 @@ | |||
|
|
|||
| # Set range of valid CMake versions to build the project. | |||
| cmake_minimum_required(VERSION 3.1...3.20) | |||
| cmake_minimum_required(VERSION 3.11...3.20) | |||
There was a problem hiding this comment.
I bumped the minimum to 3.11 in order to use FetchContent below. But that can certainly be changed if you want to keep the minimum at 3.1.
| @@ -63,7 +63,7 @@ namespace lwt { | |||
| struct NodeConfig | |||
| { | |||
| enum class Type { | |||
| INPUT, INPUT_SEQUENCE, FEED_FORWARD, CONCATENATE, SEQUENCE, | |||
| NONE, INPUT, INPUT_SEQUENCE, FEED_FORWARD, CONCATENATE, SEQUENCE, | |||
There was a problem hiding this comment.
Added NONE here as the nlohmann parser falls back to the first enum element in case of an unknown string. All other enums already had NONE as their first element, so this should hopefully be a safe change (I assume nobody uses the bare int values anywhere).
| for (const auto& sub: j.at("sublayers")) { | ||
| lwt::EmbeddingConfig emb; | ||
| sub.at("weights").get_to(emb.weights); | ||
| //emb.index = sub.second.get<int>("index"); FIXME (need an example NN file) |
There was a problem hiding this comment.
Was not able to test this part of the code yet. Need a NN example.
| break; | ||
| case Architecture::MAXOUT: | ||
| add_maxout_info(layer, j); | ||
| layer.activation.function = Activation::NONE; // FIXME: to make throw_if_not_maxout() happy |
There was a problem hiding this comment.
The previous code set function to NONE but the NN file I tested with actually contains "linear":
{
"activation": "linear",
"architecture": "maxout",
"sublayers": [
Could we just leave the value on LINEAR and get rid of this check?
|
I really like this change (was never a huge fan of I'm also mostly offline until September, is this something that someone in ATLAS wants urgently? |
|
Yes, this can (and probably should) wait for a major version bump. In the meantime I can work on some of the remaining issues. But some instructions on how to more extensively test this would be needed at some point. |
|
I had to approve the pipelines, but |
Migrate the json parser from `boost` to `nlohmann_json`, which provides a better JSON API, is signifcantly faster and results in much smaller binaries (up to 30x smaller). In particular, for an ATLAS electron DNN: - 4x faster reading of the JSON - `malloc`s reduced from ~5M to 2k - 10 times smaller resident memory size
I tried that but the |
|
Hi @fwinkl, pulling from github sounds fine to me. I'm a bit worried about relying on any bleeding-edge dependencies, but it's reasonable to think nlohmann has come a long way since 2018. Since we're going to bump the major release anyway I'm just wondering if we should also clean a few other things up... |
Migrate the json parser from
boosttonlohmann_json, which providesa better JSON API, is signifcantly faster and results in much smaller
binaries (up to 30x smaller).
In particular, for an ATLAS electron DNN:
mallocs reduced from ~5M to 2kIf this is a welcome change, this PR needs a bit more work, i.e. to validate the changes on all possible network configurations. I would need some advice on how to do that (the unit test coverage seems not very good so far,
test-GRU.shpasses but it does not seem to test all code paths).