From bcff8b2893f251b71ee09e2a077a1f76146c47d3 Mon Sep 17 00:00:00 2001 From: Jack Gerrits Date: Thu, 23 Feb 2023 15:57:27 -0500 Subject: [PATCH 1/4] allow tags for options --- .../config/include/vw/config/option.h | 19 +++++++++++++++++++ .../config/include/vw/config/option_builder.h | 7 +++++++ vowpalwabbit/config/tests/options_cli_test.cc | 4 ++++ vowpalwabbit/config/tests/options_test.cc | 16 ++++++++++++++++ 4 files changed, 46 insertions(+) diff --git a/vowpalwabbit/config/include/vw/config/option.h b/vowpalwabbit/config/include/vw/config/option.h index 24d9cbd8f97..3954a183a58 100644 --- a/vowpalwabbit/config/include/vw/config/option.h +++ b/vowpalwabbit/config/include/vw/config/option.h @@ -56,6 +56,25 @@ class base_option virtual void accept(typed_option_visitor& handler) = 0; virtual ~base_option() = default; + + VW_ATTR(nodiscard) const std::vector& get_tags() const { return _tags; } + void set_tags(std::vector tags) { + std::sort(tags.begin(), tags.end()); + auto last = std::unique(tags.begin(), tags.end()); + if (last != tags.end()) { + std::stringstream ss; + ss << "Duplicate tags found in option: " << m_name << ". Tags: "; + for (auto it = tags.begin(); it != last; ++it) { + ss << *it << ", "; + } + ss << *last; + THROW(ss.str()); + } + _tags = std::move(tags); + } + +private: + std::vector _tags; }; template diff --git a/vowpalwabbit/config/include/vw/config/option_builder.h b/vowpalwabbit/config/include/vw/config/option_builder.h index 2955a16fbc4..567cdca5de6 100644 --- a/vowpalwabbit/config/include/vw/config/option_builder.h +++ b/vowpalwabbit/config/include/vw/config/option_builder.h @@ -6,6 +6,7 @@ #include "option.h" +#include #include #include #include @@ -125,6 +126,12 @@ class option_builder return std::make_shared(std::move(option._option_obj)); } + option_builder& tags(std::initializer_list input_tags) + { + _option_obj.set_tags(input_tags); + return *this; + } + private: T _option_obj; }; diff --git a/vowpalwabbit/config/tests/options_cli_test.cc b/vowpalwabbit/config/tests/options_cli_test.cc index ec01c6d0f74..b710993a329 100644 --- a/vowpalwabbit/config/tests/options_cli_test.cc +++ b/vowpalwabbit/config/tests/options_cli_test.cc @@ -536,3 +536,7 @@ TEST(OptionsCli, CheckWasSuppliedCommonPrefixBefore) EXPECT_TRUE(!options->was_supplied("int_opt")); EXPECT_TRUE(options->was_supplied("int_opt_two")); } + +TEST(OptionsCli, CheckWasSuppliedCommonPrefixBefore) +{ +} \ No newline at end of file diff --git a/vowpalwabbit/config/tests/options_test.cc b/vowpalwabbit/config/tests/options_test.cc index 96ece0db6ce..edcd03dc0c8 100644 --- a/vowpalwabbit/config/tests/options_test.cc +++ b/vowpalwabbit/config/tests/options_test.cc @@ -4,6 +4,7 @@ #include "vw/config/options.h" +#include "vw/config/option.h" #include "vw/config/options_name_extractor.h" #include @@ -189,3 +190,18 @@ TEST(Options, NameExtractionRecycle) EXPECT_EQ(name_extractor.generated_name, "im_necessary_v2_opt2"); EXPECT_EQ(result, false); } + +TEST(Options, SetTags) +{ + base_option opt; + opt.set_tags(std::vector{"tag1", "tag2", "tag3"}); + ASSERT_THAT(opt.get_tags(), ElementsAre("tag1", "tag2", "tag3")); +} + +TEST(Options, SetTagsDuplicate) +{ +} + +TEST(Options, SetTagsOnBuilder) +{ +} From cc9ebffa5d88c1b10b61b909102e0e9824de38fe Mon Sep 17 00:00:00 2001 From: Jack Gerrits Date: Thu, 23 Feb 2023 16:05:25 -0500 Subject: [PATCH 2/4] add tests --- vowpalwabbit/config/tests/options_cli_test.cc | 9 ++++++++- vowpalwabbit/config/tests/options_test.cc | 12 +++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/vowpalwabbit/config/tests/options_cli_test.cc b/vowpalwabbit/config/tests/options_cli_test.cc index b710993a329..35e188aaa75 100644 --- a/vowpalwabbit/config/tests/options_cli_test.cc +++ b/vowpalwabbit/config/tests/options_cli_test.cc @@ -4,6 +4,7 @@ #include "vw/config/options_cli.h" +#include "vw/common/vw_exception.h" #include "vw/config/cli_options_serializer.h" #include "vw/test_common/test_common.h" @@ -537,6 +538,12 @@ TEST(OptionsCli, CheckWasSuppliedCommonPrefixBefore) EXPECT_TRUE(options->was_supplied("int_opt_two")); } -TEST(OptionsCli, CheckWasSuppliedCommonPrefixBefore) +TEST(OptionsCli, MakeOptionTags) { + int int_opt{}; + option_group_definition arg_group("group1"); + arg_group.add(make_option("int_opt", int_opt).tags({"tag1"})); + + int int_opt2{}; + EXPECT_THROW(arg_group.add(make_option("int_opt2", int_opt2).tags({"tag1", "tag1"})), VW::vw_exception); } \ No newline at end of file diff --git a/vowpalwabbit/config/tests/options_test.cc b/vowpalwabbit/config/tests/options_test.cc index edcd03dc0c8..c2febb75f81 100644 --- a/vowpalwabbit/config/tests/options_test.cc +++ b/vowpalwabbit/config/tests/options_test.cc @@ -193,15 +193,13 @@ TEST(Options, NameExtractionRecycle) TEST(Options, SetTags) { - base_option opt; - opt.set_tags(std::vector{"tag1", "tag2", "tag3"}); - ASSERT_THAT(opt.get_tags(), ElementsAre("tag1", "tag2", "tag3")); + typed_option opt("my_opt"); + opt.set_tags(std::vector{"tag1", "tag2", "tag0"}); + ASSERT_THAT(opt.get_tags(), ::testing::ElementsAre("tag0", "tag1", "tag2")); } TEST(Options, SetTagsDuplicate) { -} - -TEST(Options, SetTagsOnBuilder) -{ + typed_option opt("my_opt"); + EXPECT_THROW(opt.set_tags(std::vector{"tag1", "tag1", "tag3"}), VW::vw_exception); } From 6ef8972160adc920739c3b9c1f99d61827d3c2e2 Mon Sep 17 00:00:00 2001 From: Jack Gerrits Date: Thu, 23 Feb 2023 16:06:17 -0500 Subject: [PATCH 3/4] Formatting --- vowpalwabbit/config/include/vw/config/option.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/vowpalwabbit/config/include/vw/config/option.h b/vowpalwabbit/config/include/vw/config/option.h index 3954a183a58..b30482743d6 100644 --- a/vowpalwabbit/config/include/vw/config/option.h +++ b/vowpalwabbit/config/include/vw/config/option.h @@ -58,15 +58,15 @@ class base_option virtual ~base_option() = default; VW_ATTR(nodiscard) const std::vector& get_tags() const { return _tags; } - void set_tags(std::vector tags) { + void set_tags(std::vector tags) + { std::sort(tags.begin(), tags.end()); auto last = std::unique(tags.begin(), tags.end()); - if (last != tags.end()) { + if (last != tags.end()) + { std::stringstream ss; ss << "Duplicate tags found in option: " << m_name << ". Tags: "; - for (auto it = tags.begin(); it != last; ++it) { - ss << *it << ", "; - } + for (auto it = tags.begin(); it != last; ++it) { ss << *it << ", "; } ss << *last; THROW(ss.str()); } From a3812b730465385f93ee14b27e81f54043c3d55d Mon Sep 17 00:00:00 2001 From: Jack Gerrits Date: Thu, 23 Feb 2023 16:21:25 -0500 Subject: [PATCH 4/4] constrain tags to lowercase with underscores --- vowpalwabbit/config/CMakeLists.txt | 1 + .../config/include/vw/config/option.h | 15 +-------- vowpalwabbit/config/src/option.cc | 32 +++++++++++++++++++ vowpalwabbit/config/tests/options_cli_test.cc | 5 +-- vowpalwabbit/config/tests/options_test.cc | 15 +++++++-- 5 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 vowpalwabbit/config/src/option.cc diff --git a/vowpalwabbit/config/CMakeLists.txt b/vowpalwabbit/config/CMakeLists.txt index deb5df88825..6b6379a7c74 100644 --- a/vowpalwabbit/config/CMakeLists.txt +++ b/vowpalwabbit/config/CMakeLists.txt @@ -15,6 +15,7 @@ set(vw_config_sources src/options_cli.cc src/options_name_extractor.cc src/options.cc + src/option.cc ) vw_add_library( diff --git a/vowpalwabbit/config/include/vw/config/option.h b/vowpalwabbit/config/include/vw/config/option.h index b30482743d6..fa584b15f32 100644 --- a/vowpalwabbit/config/include/vw/config/option.h +++ b/vowpalwabbit/config/include/vw/config/option.h @@ -58,20 +58,7 @@ class base_option virtual ~base_option() = default; VW_ATTR(nodiscard) const std::vector& get_tags() const { return _tags; } - void set_tags(std::vector tags) - { - std::sort(tags.begin(), tags.end()); - auto last = std::unique(tags.begin(), tags.end()); - if (last != tags.end()) - { - std::stringstream ss; - ss << "Duplicate tags found in option: " << m_name << ". Tags: "; - for (auto it = tags.begin(); it != last; ++it) { ss << *it << ", "; } - ss << *last; - THROW(ss.str()); - } - _tags = std::move(tags); - } + void set_tags(std::vector tags); private: std::vector _tags; diff --git a/vowpalwabbit/config/src/option.cc b/vowpalwabbit/config/src/option.cc new file mode 100644 index 00000000000..d4b69ef17ee --- /dev/null +++ b/vowpalwabbit/config/src/option.cc @@ -0,0 +1,32 @@ +// Copyright (c) by respective owners including Yahoo!, Microsoft, and +// individual contributors. All rights reserved. Released under a BSD (revised) +// license as described in the file LICENSE. + +#include "vw/config/option.h" + +#include + +void VW::config::base_option::set_tags(std::vector tags) +{ + std::sort(tags.begin(), tags.end()); + auto last = std::unique(tags.begin(), tags.end()); + if (last != tags.end()) + { + std::stringstream ss; + ss << "Duplicate tags found in option: " << m_name << ". Tags: "; + for (auto it = tags.begin(); it != last; ++it) { ss << *it << ", "; } + ss << *last; + THROW(ss.str()); + } + for (const auto& tag : tags) + { + if (!std::regex_match(tag, std::regex("^[a-z_]+$"))) + { + std::stringstream ss; + ss << "Invalid tag found in option: " << m_name << ". Tag: " << tag + << ". Tags must be lowercase and contain only letters and underscores."; + THROW(ss.str()); + } + } + _tags = std::move(tags); +} diff --git a/vowpalwabbit/config/tests/options_cli_test.cc b/vowpalwabbit/config/tests/options_cli_test.cc index 35e188aaa75..afa2ec7d35b 100644 --- a/vowpalwabbit/config/tests/options_cli_test.cc +++ b/vowpalwabbit/config/tests/options_cli_test.cc @@ -542,8 +542,9 @@ TEST(OptionsCli, MakeOptionTags) { int int_opt{}; option_group_definition arg_group("group1"); - arg_group.add(make_option("int_opt", int_opt).tags({"tag1"})); + arg_group.add(make_option("int_opt", int_opt).tags({"taga"})); + ASSERT_THAT(arg_group.m_options.back()->get_tags(), ::testing::ElementsAre("taga")); int int_opt2{}; - EXPECT_THROW(arg_group.add(make_option("int_opt2", int_opt2).tags({"tag1", "tag1"})), VW::vw_exception); + EXPECT_THROW(arg_group.add(make_option("int_opt2", int_opt2).tags({"taga", "taga"})), VW::vw_exception); } \ No newline at end of file diff --git a/vowpalwabbit/config/tests/options_test.cc b/vowpalwabbit/config/tests/options_test.cc index c2febb75f81..eb5a142ea5d 100644 --- a/vowpalwabbit/config/tests/options_test.cc +++ b/vowpalwabbit/config/tests/options_test.cc @@ -194,12 +194,21 @@ TEST(Options, NameExtractionRecycle) TEST(Options, SetTags) { typed_option opt("my_opt"); - opt.set_tags(std::vector{"tag1", "tag2", "tag0"}); - ASSERT_THAT(opt.get_tags(), ::testing::ElementsAre("tag0", "tag1", "tag2")); + opt.set_tags(std::vector{"tagb", "taga", "tagc"}); + ASSERT_THAT(opt.get_tags(), ::testing::ElementsAre("taga", "tagb", "tagc")); } TEST(Options, SetTagsDuplicate) { typed_option opt("my_opt"); - EXPECT_THROW(opt.set_tags(std::vector{"tag1", "tag1", "tag3"}), VW::vw_exception); + EXPECT_THROW(opt.set_tags(std::vector{"taga", "tagb", "tagb"}), VW::vw_exception); +} + +TEST(Options, SetTagsInvalidName) +{ + typed_option opt("my_opt"); + EXPECT_THROW(opt.set_tags(std::vector{"tag1"}), VW::vw_exception); + EXPECT_THROW(opt.set_tags(std::vector{"Tag"}), VW::vw_exception); + EXPECT_THROW(opt.set_tags(std::vector{"a b"}), VW::vw_exception); + EXPECT_THROW(opt.set_tags(std::vector{"t-a-g"}), VW::vw_exception); }