-
Notifications
You must be signed in to change notification settings - Fork 189
benchmark/nixlbench: --config_file param supported #989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
benchmark/nixlbench: --config_file param supported #989
Conversation
|
👋 Hi anton-nayshtut! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
26925b6 to
86ac305
Compare
80c13c4 to
8a829d4
Compare
|
@anton-nayshtut Thanks so much adding this. I had two general comments. For CXXOPTS, Im unclear why this was required? If you use the gflags flagfile now, any other options after the file should overwrite the ones in the file. WDYT? |
vvenkates27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, and why not JSon?
| @@ -0,0 +1,624 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License needs to be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not our code. It's an external MIT-licensed library. We can't change the license, AFAIK. But I exempted the external files from the relevant CI checks.
| @@ -0,0 +1,2925 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here .. fix license
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| # include <regex> | ||
| #endif // CXXOPTS_NO_REGEX | ||
|
|
||
| // Nonstandard before C++17, which is coincidentally what we also need for <optional> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of CXX_OPTS compared to gflags? Why can't we parse using glags even for a config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the commit message for benchmark/nixlbench: switched to cxxopts - I discuss it at length there. Hope it makes sense.
As for the questions: INI vs JSON Sure, I considered using JSON for config files and I agree that, as you mentioned, it has some advantages. However, JSON also has some downsides that led me to prefer INI format. My thoughts were along these lines: INI format is often a better choice than JSON for simple configuration files because it’s more approachable, easier to manually edit, and visually cleaner for basic use cases. INI files are highly readable and easy to work with - especially for non-programmers or for quick manual edits. They support organizing settings into basic sections and key-value pairs, so simple configs stay tidy and quick to review. Comments are supported in INI (unlike JSON by default), letting you explain settings directly in the file, which is a huge advantage for documentation and collaboration. INI files are much simpler than JSON, and avoid extra punctuation, strict syntax, and nested requirements - reducing the chance of errors when hand-editing. In summary, if your configs are flat, small, and likely to be edited by hand, INI is much easier for users to read and update than JSON. JSON is better when you need complex or hierarchical data, but INI shines for basic, human-friendly settings. Some additional considerations:
Of course, I'm certainly open to revisiting this decision and would be happy to go with JSON if you or others in the community feel it's a better fit. CXXOPTS vs gflags See the commit message for |
To me it looks like INI are an upgraded version from just using a flagfile with gflags (https://gflags.github.io/gflags/#special). That also allows us to specify comments, and just copy gflags into it as they are today (no need for extra deps). We are not changing any parameters anyways so this would be the easiest option. CXXOPTS vs GFlags I didnt understand the advantage of dynamic arguments, since the file parsing and command line parsing are handled separately. Wouldn't you still need logic to match the ini parsed value to cxxopts? Wouldn't these be interpreted separately, i.e. task0.local and task0.remote are considered two different parameters without any hierarchy? There is also TOML (its made for config files) which is a cross between INI and JSON files. That might be a nicer option over INI to use when multi-tests can be run to give mroe flexibility in specifying config. |
Oh, perfect! I wasn’t aware of the Just a small note:
Yeah, this is definitely an advantage of However, my original idea was that if a user defines an option in a config file but also specifies it on the command line, the command line value would override the config file - whereas
My intent was that we could support both dynamically defined config sections (like in fio config) and command-line overrides for the same parameters. For example, you could define a job in a config file ( [example_job1]
backend=POSIX
posix_api_type=AIO
filepath=/mnt/test1
max_batch_size=128
start_batch_size=128
op_type=READThen, parameters could be overridden from the command line like so: We don't need this now, so I probably overstepped. I mostly wanted an infrastructure flexible enough to handle any scenario we might want in the future. :-)
I'm perfectly fine with TOML as well, and I’d be happy to rewrite everything in that format if you think it’s better. Let’s decide whether we want to keep using |
8a829d4 to
d83a3df
Compare
Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
gflags-specific parameter definitions have been replaced with the NB_ARG_x macros. This will help minimize changes when we move to a different parameter parsing infrastructure in upcoming patches. This patch doesn't change any logic. Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
NB_ARG parameter getter macro introduced. This will help us to minimize changes when we'll move to a different parameter parsing infrastructure in upcoming patches. This patch doesn't change any logic. Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
We'll use cxxopts for command line parsing in upcoming patches. Copied from https://github.com/jarro2783/cxxopts SHA: 8df9a4d2716ebf1c68eced0b6d9de185c6cfd50a Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
This patch replaces gflags-based parameter parsing with cxxopts-based parsing, and removes the gflags dependency. This change will enable us to support config file-based parameters in upcoming patches. Unfortunately, gflags is not flexible enough for our needs. For example, it does not indicate whether a parameter was explicitly provided by the user or if it is using a default. This functionality is required for correct parameter specification priority: first, a value provided by the user on the command line; then a value from the config file; and finally, the default value. Additionally, gflags does not support dynamically formed arguments, which is needed to handle a list of tasks as discussed. For this use case, the config file would look like: [general] ... num_tasks=2 [task0] local=... remote=... type=write ... [task1] local=... remote=... type=read ... This translates to command line parameters like: --num_tasks=2 --task0.local=... ... --task1.local=... Since gflags is a compile-time mechanism, it cannot support this level of dynamic argument handling. Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
We'll use ini-cpp for config file parsing in upcoming patches. Copied from https://github.com/SSARCandy/ini-cpp/ SHA: 513c0947477855d3925446084779697145246714 Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
d83a3df to
fbfda41
Compare
| loadParams(cxxopts::ParseResult &result); | ||
| template<class T> | ||
| static T | ||
| getParamValue(std::unique_ptr<inih::INIReader> &ini, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You rarely need to pass unique_ptr by non-const pointer, unless you modify it inside.
If object must exist - pass it by reference
If object might be nullptr - pass it by pointer
name can be std::string_view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const - Thanks! Done!
name - I'm not sure I get it. Is there something wrong with const char \"?
| try { | ||
| return ini->Get<T>("global", name); | ||
| } | ||
| catch (std::runtime_error &) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better catch exceptions by const ref
Why not printing an error to std::cerr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const - Thanks a lot! Fixed!
error - it's not a real error. It just means that a specific value is not present in the config file.
| try { | ||
| ini = std::make_unique<inih::INIReader>(config_file); | ||
| } | ||
| catch (std::runtime_error &e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Fixed!
| xferBenchConfig::loadParams(cxxopts::ParseResult &result) { | ||
| std::unique_ptr<inih::INIReader> ini; | ||
|
|
||
| if (result.count("config_file")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe define constants for these string literals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config_file is the only special-case string literal, and it's only being treated specially in these two consecutive lines of code. So, I thought defining a constant in this case would not be necessary. Please comment.
| return result[name].as<T>(); | ||
| } | ||
|
|
||
| int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if function returns only 0/-1 - maybe use bool as a return type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. In the current code, the xferBenchConfig::loadFromFlags function that has this prototype, and I just kept it that way to minimize the diff.
| * @throws std::runtime_error if there is an error parsing the INI file | ||
| */ | ||
| inline INIReader::INIReader(FILE* file) { | ||
| _error = ini_parse_file(file, ValueHandler, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit weird design to store an error in tmp variable
why not just throw an exception from ini_parse_file?
| const T& v); | ||
|
|
||
| template <typename T = std::string> | ||
| void UpdateEntry(const std::string& section, const std::string& name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to our policy methods should be named with camelCase.
You can check the entire change with clang-tidy:
git diff -U0 HEAD~1 | clang-tidy-diff-19.py -clang-tidy-binary=clang-tidy-19 -p1 -path=build-x86_64
| * @return The map representing the values in the given section | ||
| * @throws std::runtime_error if the section is not found | ||
| */ | ||
| inline const std::unordered_map<std::string, std::string> INIReader::Get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to make a copy of the map I guess
| const std::string& name, const T& v) { | ||
| if (!_values[section][name].size()) { | ||
| throw std::runtime_error("key '" + std::string(name) + | ||
| "' not exist in section '" + section + "'."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "' not exist in section '" + section + "'."); | |
| "' does not exist in section '" + section + "'."); |
| inline void INIReader::UpdateEntry(const std::string& section, | ||
| const std::string& name, | ||
| const std::vector<T>& vs) { | ||
| if (!_values[section][name].size()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's used in several places - maybe create a function checkDuplicate
This patch introduces config file support to nixlbench.
The name of a config file can be specified using the --config_file
command-line parameter. The config file is in INI format.
Each existing command-line parameter can also be placed in the
"[global]" section of the configuration file, so the following
invocations:
nixlbench --etcd_endpoints http://localhost:2379 --backend POSIX
--filepath /mnt/test --posix_api_type AIO
and
nixlbench --config_file /tmp/nixlbench.config
where /tmp/nixlbench.config contains:
[global]
etcd_endpoints=http://localhost:2379
backend=POSIX
filepath=/mnt/test
posix_api_type=AIO
are identical.
If a parameter exists in the config file and is also explicitly
specified on the command line, the latter takes precedence.
Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
Signed-off-by: Anton Nayshtut <anayshtut@nvidia.com>
fbfda41 to
e3d710c
Compare
This PR introduces config file support to
nixlbench.The name of a config file can be specified using the
--config_filecommand-line parameter. The config file is in INI format.Each existing command-line parameter can also be placed in the
[global]section of the configuration file, so the following invocations:and
where
/tmp/nixlbench.configcontains:are identical.
If a parameter exists in the config file and is also explicitly specified on the command line, the latter takes precedence.