Add --config-json-path to accept flags specified in a JSON file. #265

angela28chen · 2023-08-10T04:35:52Z

Add flag --config-json-path to accept commandline flags that are specified in a JSON file. This flag can be used multiple times. The priority of any conflicting flags is as follows:

later flag on commandline > earlier flag on commandline > flag in later JSON file > flag in earlier JSON file

For flags that accept multiple values, the same priority is used and only flags specified in the same JSON file or flags all on the commandline will be combined.

Detailed changes:

Changed map mAllOptions to hold strings instead of string_views since there are now temporary strings originating from the JSON structure, changed some input parameters for CliOptions member functions to match.
Created some CommandLineParser methods to aid in parsing options (these are public for unit testing purposes)
- ParseJson
- ParseOption
Created more methods for CliOptions
- AddOption overload that accepts one key and a vector of values to reduce the time needed to insert arrays. This function is only used by the JSON parsing flow. On the command line the flag must still be specified multiple times (--flag-name 1,2,3 is still invalid)
- OverwriteOptions
Updated CommandLineParser::Parse
- Added a second initial pass of argv during commandline parsing to identify any provided JSON config files
- Implemented overwriting
Added mJsonConfigFlagName to CommandLineParser so that the string config-json-path isn't specified in multiple places

angela28chen · 2023-08-10T23:17:12Z

Note: I tried to keep the input parameters for CliOptions as std::string_view in some cases, but I was running into issues with heterogeneous lookup for unordered containers (could get it to work on Windows but not on the current versions of the Android and Linux CI builds)

Keenuts · 2023-08-11T09:13:02Z

Note: I tried to keep the input parameters for CliOptions as std::string_view in some cases, but I was running into issues with heterogeneous lookup for unordered containers (could get it to work on Windows but not on the current versions of the Android and Linux CI builds)

Arf, that's sad...
Seeing this https://groups.google.com/a/isocpp.org/g/std-proposals/c/oXBQ6WVXLCU
meaning map does supports it since 14(https://en.cppreference.com/w/cpp/container/map/find)

What's sadder is it wasn't done to the unordered_map until c++20 😥
(https://en.cppreference.com/w/cpp/container/unordered_map/find)

So this explains the lack of support, at least from the Android NDK.
Surprising to see clang also refuses it...
Thanks for looking into that!

Keenuts

Thanks for this!
A few things to fix, but overall OK.

The the JSON overriding flags, even when specified after, this is fine IMO. Complexity to manage the ordering is not yet justified. But it should be documented.

src/ppx/command_line_parser.cpp

include/ppx/command_line_parser.h

Keenuts · 2023-08-11T10:12:51Z

src/ppx/command_line_parser.cpp

+        std::string value = ss.str();
+        ss.str("");
+
+        if (value.length() > 0 && value.substr(0, 1) == "[") {


you probably shouldn't parse the data again. What about using functions like it->is_array().

src/ppx/command_line_parser.cpp

src/ppx/application.cpp

angela28chen · 2023-08-14T20:36:18Z

Thank you for the feedback, I've tried to address it and made two major changes:

Better error-reporting for finding/opening/parsing JSON files, and better validation for JSON structure (explicitly checking for when it's expected to be an object/array)
Implementing JSON values as lower priority than commandline flags, which involves identifying JSON file paths and adding those options first (since the later ones have higher priority). I combined this with the existing initial pass that splits arguments specified with '='.

apazylbe

some optional suggestions, otherwise lgtm

include/ppx/application.h

src/ppx/command_line_parser.cpp

Keenuts

Forgot to send the review, sorry!

src/ppx/application.cpp

src/ppx/command_line_parser.cpp

angela28chen · 2023-08-18T20:59:51Z

Note: This PR is not ready for submitting.

To summarize the result of the discussion Nathan and I had about the ordering for list arrays, we decided that combining the contents of the JSON file and the commandline flags is confusing, so any time that a list flag is used on the command-line it will completely overwrite the value for that flag specified inside the JSON file.

Detailed example:

file.json: [A, B]
file2.json: [C, D]

./benchmark --config-json-path=file.json 
Final CliOptions: A, B
asset dir search order: A, B, default

./benchmark --config-json-path=file.json --assets-path=E --assets-path=F
Final CliOptions: E, F
asset dir search order: E, F, default

./benchmark --config-json-path=file.json --config-json-path=file2.json
Final CliOptions: C, D
asset dir search order: C, D, default

./benchmark --config-json-path=file.json --config-json-path=file2.json --assets-path=E --assets-path=F 
Final CliOptions: E, F
//asset dir search order: E, F, default

android/linux builds * Make mAllOptions strings and not string_views * Make the functions that need to search mAllOptions take input parameters of const std::string& instead

* JSON file flags are lower priority than flags specified on the command line * Using JSON is_array and is_object functions * Using round bracket syntax for initalization * Fix Parse() function * Better error reporting for parsing the JSON file * Fixed flag description typo

* Rename pConfigJsonPath to pConfigJsonPaths * Change commandline parsing to 2 initial passes for better readability

and command line flags to overwrite json files after that.

angela28chen · 2023-08-19T01:37:49Z

This is ready for review/merging. I don't have unit tests for commandline VS JSON file priority but I did check it manually and it should behave as described in the previous comment.

angela28chen force-pushed the json-config branch 3 times, most recently from 12d0a8e to 7fe93d3 Compare August 10, 2023 23:13

angela28chen requested a review from Keenuts August 11, 2023 06:03

Keenuts reviewed Aug 11, 2023

View reviewed changes

angela28chen force-pushed the json-config branch from 5ff18a9 to f0a7a69 Compare August 14, 2023 17:27

angela28chen requested review from Keenuts and apazylbe August 14, 2023 20:36

angela28chen mentioned this pull request Aug 15, 2023

Move string parsing logic from CommandLineParser class to ppx::string_util::ParseOrDefault() #271

Closed

apazylbe reviewed Aug 16, 2023

View reviewed changes

include/ppx/application.h Outdated Show resolved Hide resolved

src/ppx/command_line_parser.cpp Outdated Show resolved Hide resolved

src/ppx/command_line_parser.cpp Outdated Show resolved Hide resolved

angela28chen requested a review from apazylbe August 16, 2023 18:32

apazylbe approved these changes Aug 17, 2023

View reviewed changes

Keenuts reviewed Aug 17, 2023

View reviewed changes

src/ppx/application.cpp Outdated Show resolved Hide resolved

src/ppx/command_line_parser.cpp Show resolved Hide resolved

angela28chen force-pushed the json-config branch from c4be397 to 8706bea Compare August 18, 2023 15:18

angela28chen requested a review from Keenuts August 18, 2023 15:26

angela28chen added 8 commits August 18, 2023 20:40

Add --config-json-path to accept flags specified in a JSON file.

0b40729

Remove custom hash for unordered map since not supported by CI

c69b842

android/linux builds * Make mAllOptions strings and not string_views * Make the functions that need to search mAllOptions take input parameters of const std::string& instead

Add error for invalid config file name

7af7d2d

Make only one initial pass, handling both '=' and --config-json-path

889f1f1

Cleaning up comments and refactoring a bit for clarity

4e8e56d

Minor changes

334569b

* Rename pConfigJsonPath to pConfigJsonPaths * Change commandline parsing to 2 initial passes for better readability

Change priority for --assets-path to match flag descriptions.

655de0c

angela28chen force-pushed the json-config branch from 8706bea to 306397a Compare August 19, 2023 00:58

Make changes to allow subsequent json files to overwrite earlier ones,

dc32011

and command line flags to overwrite json files after that.

angela28chen force-pushed the json-config branch from 306397a to dc32011 Compare August 19, 2023 01:01

angela28chen requested a review from apazylbe August 19, 2023 01:38

apazylbe approved these changes Aug 21, 2023

View reviewed changes

apazylbe merged commit 4bdd70e into google:main Aug 21, 2023

angela28chen deleted the json-config branch August 21, 2023 14:44

hysw mentioned this pull request Aug 22, 2023

Change C++ standard to C++17. #277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --config-json-path to accept flags specified in a JSON file. #265

Add --config-json-path to accept flags specified in a JSON file. #265

angela28chen commented Aug 10, 2023 •

edited

Loading

angela28chen commented Aug 10, 2023

Keenuts commented Aug 11, 2023

Keenuts left a comment

Keenuts Aug 11, 2023

angela28chen commented Aug 14, 2023

apazylbe left a comment

Keenuts left a comment

angela28chen commented Aug 18, 2023

angela28chen commented Aug 19, 2023

Add --config-json-path to accept flags specified in a JSON file. #265

Add --config-json-path to accept flags specified in a JSON file. #265

Conversation

angela28chen commented Aug 10, 2023 • edited Loading

angela28chen commented Aug 10, 2023

Keenuts commented Aug 11, 2023

Keenuts left a comment

Choose a reason for hiding this comment

Keenuts Aug 11, 2023

Choose a reason for hiding this comment

angela28chen commented Aug 14, 2023

apazylbe left a comment

Choose a reason for hiding this comment

Keenuts left a comment

Choose a reason for hiding this comment

angela28chen commented Aug 18, 2023

angela28chen commented Aug 19, 2023

angela28chen commented Aug 10, 2023 •

edited

Loading