Commit 21dba6f0 authored by tby's avatar tby Committed by Commit Bot

[Cros SR] Resubmit of reverted adaptive ranker change.

The previous CL [0], implementing an adaptive ranker for search results, was
reverted due to an uninitialised member in a test class. This CL fixes the
error, and is identical except for the following diff:

  --- a/chrome/browser/ui/app_list/search/tests/mixer_unittest.cc
  +++ b/chrome/browser/ui/app_list/search/tests/mixer_unittest.cc
  @@ -68,6 +68,7 @@ class TestSearchProvider : public SearchProvider {
	 : prefix_(prefix),
	   count_(0),
	   bad_relevance_range_(false),
  +        small_relevance_range_(false),
	   display_type_(ash::SearchResultDisplayType::kList

[0] https://chromium-review.googlesource.com/c/chromium/src/+/1474966

Bug: 931149
Change-Id: Id079f664b2eba64f88108b8133be7bf1f8087d2d
Reviewed-on: https://chromium-review.googlesource.com/c/1481138Reviewed-by: default avatarJia Meng <jiameng@chromium.org>
Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634403}
parent 279741ef
......@@ -27,6 +27,8 @@ const base::Feature kEnableZeroStateSuggestions{
"EnableZeroStateSuggestions", base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kEnableAppListSearchAutocomplete{
"EnableAppListSearchAutocomplete", base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kEnableAdaptiveResultRanker{
"EnableAdaptiveResultRanker", base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kEnableAppSearchResultRanker{
"EnableAppSearchResultRanker", base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kEnableAppReinstallZeroState{
......@@ -68,6 +70,10 @@ bool IsAppListSearchAutocompleteEnabled() {
return base::FeatureList::IsEnabled(kEnableAppListSearchAutocomplete);
}
bool IsAdaptiveResultRankerEnabled() {
return base::FeatureList::IsEnabled(kEnableAdaptiveResultRanker);
}
bool IsAppSearchResultRankerEnabled() {
return base::FeatureList::IsEnabled(kEnableAppSearchResultRanker);
}
......
......@@ -44,6 +44,9 @@ ASH_PUBLIC_EXPORT extern const base::Feature kEnableZeroStateSuggestions;
// Enables the feature to autocomplete text typed in the AppList search box.
ASH_PUBLIC_EXPORT extern const base::Feature kEnableAppListSearchAutocomplete;
// Enable an adaptive model that tweaks search result scores.
ASH_PUBLIC_EXPORT extern const base::Feature kEnableAdaptiveResultRanker;
// Enables the feature to rank app search result using AppSearchResultRanker.
ASH_PUBLIC_EXPORT extern const base::Feature kEnableAppSearchResultRanker;
......@@ -62,6 +65,7 @@ bool ASH_PUBLIC_EXPORT IsAppDataSearchEnabled();
bool ASH_PUBLIC_EXPORT IsSettingsShortcutSearchEnabled();
bool ASH_PUBLIC_EXPORT IsZeroStateSuggestionsEnabled();
bool ASH_PUBLIC_EXPORT IsAppListSearchAutocompleteEnabled();
bool ASH_PUBLIC_EXPORT IsAdaptiveResultRankerEnabled();
bool ASH_PUBLIC_EXPORT IsAppSearchResultRankerEnabled();
bool ASH_PUBLIC_EXPORT IsAppReinstallZeroStateEnabled();
bool ASH_PUBLIC_EXPORT IsEmbeddedAssistantUIEnabled();
......
......@@ -16,6 +16,7 @@
#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
#include "chrome/browser/ui/app_list/search/search_provider.h"
#include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
#include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.h"
namespace app_list {
......@@ -108,6 +109,10 @@ void Mixer::MixAndPublish(size_t num_max_results) {
// number* will be kept (e.g., an app result takes priority over a web store
// result with the same ID).
RemoveDuplicates(&results);
// TODO(https://crbug.com/931149): tweak the scores of |results| using
// |ranker_|.
std::sort(results.begin(), results.end());
const size_t original_size = results.size();
......@@ -156,9 +161,17 @@ void Mixer::FetchResults() {
group->FetchResults();
}
void Mixer::SetRecurrenceRanker(std::unique_ptr<RecurrenceRanker> ranker) {
ranker_ = std::move(ranker);
}
void Mixer::Train(const std::string& id, RankingItemType type) {
// TODO(https://crbug.com/931149)) train a ranking model using this training
// signal.
if (!ranker_)
return;
if (type == RankingItemType::kFile || type == RankingItemType::kOmnibox) {
ranker_->Record(std::to_string(static_cast<int>(type)));
}
}
} // namespace app_list
......@@ -22,6 +22,7 @@ namespace test {
FORWARD_DECLARE_TEST(MixerTest, Publish);
}
class RecurrenceRanker;
class SearchProvider;
enum class RankingItemType;
......@@ -48,6 +49,9 @@ class Mixer {
// Collects the results, sorts and publishes them.
void MixAndPublish(size_t num_max_results);
// Add a |RecurrenceRanker| to tweak mixing results.
void SetRecurrenceRanker(std::unique_ptr<RecurrenceRanker> ranker);
// Handle a training signal.
void Train(const std::string& id, RankingItemType type);
......@@ -81,6 +85,9 @@ class Mixer {
Groups groups_;
// Adaptive category ranking model, which tweaks the score of search results.
std::unique_ptr<RecurrenceRanker> ranker_;
DISALLOW_COPY_AND_ASSIGN(Mixer);
};
......
......@@ -17,6 +17,7 @@
#include "chrome/browser/ui/app_list/app_list_model_updater.h"
#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
#include "chrome/browser/ui/app_list/search/search_provider.h"
#include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.h"
#include "chrome/browser/ui/ash/tablet_mode_client.h"
namespace app_list {
......@@ -114,6 +115,11 @@ ChromeSearchResult* SearchController::GetResultByTitleForTest(
return nullptr;
}
void SearchController::SetRecurrenceRanker(
std::unique_ptr<RecurrenceRanker> ranker) {
mixer_->SetRecurrenceRanker(std::move(ranker));
}
void SearchController::Train(const std::string& id, RankingItemType type) {
for (const auto& provider : providers_)
provider->Train(id, type);
......
......@@ -21,6 +21,7 @@ class ChromeSearchResult;
namespace app_list {
class RecurrenceRanker;
class SearchProvider;
enum class RankingItemType;
......@@ -49,6 +50,9 @@ class SearchController {
ChromeSearchResult* FindSearchResult(const std::string& result_id);
ChromeSearchResult* GetResultByTitleForTest(const std::string& title);
// Set a |RecurrenceRanker| to tweak search results.
void SetRecurrenceRanker(std::unique_ptr<RecurrenceRanker> ranker);
// Sends training signal to each |providers_|
void Train(const std::string& id, RankingItemType type);
......
......@@ -8,10 +8,12 @@
#include "ash/public/cpp/app_list/app_list_features.h"
#include "ash/public/cpp/app_list/app_list_switches.h"
#include "base/metrics/field_trial_params.h"
#include "base/strings/string_util.h"
#include "base/time/default_clock.h"
#include "build/build_config.h"
#include "chrome/browser/chromeos/arc/arc_util.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider.h"
#include "chrome/browser/ui/app_list/search/app_search_provider.h"
......@@ -24,6 +26,7 @@
#include "chrome/browser/ui/app_list/search/mixer.h"
#include "chrome/browser/ui/app_list/search/omnibox_provider.h"
#include "chrome/browser/ui/app_list/search/search_controller.h"
#include "chrome/browser/ui/app_list/search/search_result_ranker/recurrence_ranker.h"
#include "chrome/browser/ui/app_list/search/settings_shortcut/settings_shortcut_provider.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/chrome_switches.h"
......@@ -165,6 +168,28 @@ std::unique_ptr<SearchController> CreateSearchController(
std::make_unique<CrostiniRepositorySearchProvider>(profile));
}
if (app_list_features::IsAdaptiveResultRankerEnabled()) {
RecurrenceRankerConfigProto group_ranker_config;
auto* predictor =
group_ranker_config.mutable_zero_state_frecency_predictor();
predictor->set_target_limit(base::GetFieldTrialParamByFeatureAsInt(
app_list_features::kEnableAdaptiveResultRanker, "target_limit", 200));
predictor->set_decay_coeff(base::GetFieldTrialParamByFeatureAsDouble(
app_list_features::kEnableAdaptiveResultRanker, "decay_coeff", 0.8f));
auto* fallback = group_ranker_config.mutable_fallback_predictor();
fallback->set_target_limit(base::GetFieldTrialParamByFeatureAsInt(
app_list_features::kEnableAdaptiveResultRanker, "fallback_target_limit",
200));
fallback->set_decay_coeff(base::GetFieldTrialParamByFeatureAsDouble(
app_list_features::kEnableAdaptiveResultRanker, "fallback_decay_coeff",
0.8f));
controller->SetRecurrenceRanker(std::make_unique<RecurrenceRanker>(
profile->GetPath().AppendASCII("adaptive_result_ranker.proto"),
group_ranker_config,
chromeos::ProfileHelper::IsEphemeralUserProfile(profile)));
}
return controller;
}
......
......@@ -16,7 +16,9 @@
#include "base/strings/string16.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/ui/app_list/search/search_provider.h"
#include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
#include "chrome/browser/ui/app_list/test/fake_app_list_model_updater.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -66,6 +68,7 @@ class TestSearchProvider : public SearchProvider {
: prefix_(prefix),
count_(0),
bad_relevance_range_(false),
small_relevance_range_(false),
display_type_(ash::SearchResultDisplayType::kList) {}
~TestSearchProvider() override {}
......@@ -80,6 +83,10 @@ class TestSearchProvider : public SearchProvider {
// of the canonical [0.0, 1.0] range.
if (bad_relevance_range_)
relevance = 10.0 - i * 10;
// If |small_relevance_range_|, keep the relevances in the same order, but
// make the differences very small: 0.5, 0.499, 0.498, ...
if (small_relevance_range_)
relevance = 0.5 - i / 100.0;
TestSearchResult* result = new TestSearchResult(id, relevance);
result->SetDisplayType(display_type_);
Add(std::unique_ptr<ChromeSearchResult>(result));
......@@ -92,11 +99,13 @@ class TestSearchProvider : public SearchProvider {
}
void set_count(size_t count) { count_ = count; }
void set_bad_relevance_range() { bad_relevance_range_ = true; }
void set_small_relevance_range() { small_relevance_range_ = true; }
private:
std::string prefix_;
size_t count_;
bool bad_relevance_range_;
bool small_relevance_range_;
ChromeSearchResult::DisplayType display_type_;
DISALLOW_COPY_AND_ASSIGN(TestSearchProvider);
......@@ -114,6 +123,16 @@ class MixerTest : public testing::Test {
providers_.push_back(std::make_unique<TestSearchProvider>("app"));
providers_.push_back(std::make_unique<TestSearchProvider>("omnibox"));
providers_.push_back(std::make_unique<TestSearchProvider>("webstore"));
}
void CreateMixer(bool use_adaptive_ranker) {
if (use_adaptive_ranker) {
scoped_feature_list_.InitWithFeatures(
{app_list_features::kEnableAdaptiveResultRanker}, {});
} else {
scoped_feature_list_.InitWithFeatures(
{}, {app_list_features::kEnableAdaptiveResultRanker});
}
mixer_ = std::make_unique<Mixer>(model_updater_.get());
......@@ -150,12 +169,18 @@ class MixerTest : public testing::Test {
return result;
}
void Train(const std::string& id, const RankingItemType& type) {
mixer_->Train(id, type);
}
Mixer* mixer() { return mixer_.get(); }
TestSearchProvider* app_provider() { return providers_[0].get(); }
TestSearchProvider* omnibox_provider() { return providers_[1].get(); }
TestSearchProvider* webstore_provider() { return providers_[2].get(); }
private:
base::test::ScopedFeatureList scoped_feature_list_;
std::unique_ptr<Mixer> mixer_;
std::unique_ptr<FakeAppListModelUpdater> model_updater_;
......@@ -165,6 +190,9 @@ class MixerTest : public testing::Test {
};
TEST_F(MixerTest, Basic) {
// Create mixer without adaptive ranker.
CreateMixer(false);
// Note: Some cases in |expected| have vastly more results than others, due to
// the "at least 6" mechanism. If it gets at least 6 results from all
// providers, it stops at 6. If not, it fetches potentially many more results
......@@ -216,6 +244,9 @@ TEST_F(MixerTest, Basic) {
}
TEST_F(MixerTest, RemoveDuplicates) {
// Create mixer without adaptive ranker.
CreateMixer(false);
const std::string dup = "dup";
// This gives "dup0,dup1,dup2".
......@@ -236,5 +267,22 @@ TEST_F(MixerTest, RemoveDuplicates) {
EXPECT_EQ("dup0,dup1,dup2", GetResults());
}
TEST_F(MixerTest, RankerIsDisabledWithFlag) {
CreateMixer(false);
for (int i = 0; i < 20; ++i)
Train("omnibox2", RankingItemType::kOmnibox);
app_provider()->set_count(4);
app_provider()->set_small_relevance_range();
omnibox_provider()->set_count(4);
omnibox_provider()->set_small_relevance_range();
RunQuery();
// Expect training calls to have not affected rankings.
EXPECT_EQ(GetResults(),
"app0,omnibox0,app1,omnibox1,app2,omnibox2,app3,omnibox3");
}
} // namespace test
} // namespace app_list
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment