Repository navigation
feat: [FC-86] implement multivalue faceted search using Meilisearch and Elasticsearch #213
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
Changes from 30 commits
1b9b515
bece993
fa68434
d0f2b0e
718b223
2c721ff
82c5cad
b315d5c
d273949
27ef1a3
a90b9d4
b93e1e5
5ebcdeb
f4f698b
5cf92ce
592d1b9
517e237
e2c6dbc
f3760a7
7514511
e6f6246
1503955
e19eb6e
97149af
580f0e0
18a40b1
244a8b6
b3680b3
6c1ca11
fb406bc
44dbee7
f92d732
08cd07a
d74f8dd
cfafa9e
4c09f37
487419f
79864c6
5e6653e
4594bef
c84c7f1
8697093
6d516c5
704c520
0187b5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,3 +59,53 @@ test: test_with_es ## run tests and generate coverage report | |
| install-local: ## installs your local edx-search into the LMS and CMS python virtualenvs | ||
| docker exec -t edx.devstack.lms bash -c '. /edx/app/edxapp/venvs/edxapp/bin/activate && cd /edx/app/edxapp/edx-platform && pip uninstall -y edx-search && pip install -e /edx/src/edx-search && pip freeze | grep edx-search' | ||
| docker exec -t edx.devstack.cms bash -c '. /edx/app/edxapp/venvs/edxapp/bin/activate && cd /edx/app/edxapp/edx-platform && pip uninstall -y edx-search && pip install -e /edx/src/edx-search && pip freeze | grep edx-search' | ||
|
|
||
| test-all: create-test-network | ||
| @$(MAKE) test-meili | ||
| @$(MAKE) test-elastic | ||
|
|
||
| test-meili: meili-up | ||
| @echo "Running Meilisearch tests..." | ||
| @MEILISEARCH_MASTER_KEY=test_master_key \ | ||
| MEILISEARCH_URL=http://localhost:7700 \ | ||
| python manage.py test \ | ||
| search.tests.test_course_discovery.TestMeilisearchCourseDiscoverySearch \ | ||
| search.tests.test_meilisearch || true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we encode the specific tests to run in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your concern is totally justified, but this hardcoded list of tests is only a temporary crutch to exclude the integration test which will fail before we open/merge another PR that actually adds a new endpoint url. That PR will remove this hardcoded list and simply run all the tests as before.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, I'm fine with keeping this on a feature branch or temporarily marking those tests as skipped in the test code itself (and/or using environment variables as I suggested), but I'm reluctant to merge a workaround like this to master even if it is temporary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remembered that we can actually run the existing integrations tests since they cover the existing endpoint which is definitely supposed to work. I adjusted the Makefile to have one |
||
| @$(MAKE) meili-down | ||
|
|
||
| meili-up: create-test-network | ||
| @echo "Starting Meilisearch..." | ||
| @docker compose up -d test_meilisearch | ||
| @echo "Waiting for Meilisearch to be healthy..." | ||
| @timeout 15 bash -c \ | ||
| 'until curl -sf http://localhost:7700/health > /dev/null; do echo "Waiting..."; sleep 1; done' | ||
|
|
||
| meili-down: | ||
| @echo "Shutting down Meilisearch..." | ||
| @docker compose down test_meilisearch | ||
|
|
||
| test-elastic: elastic-up | ||
| @echo "Running Elasticsearch tests..." | ||
| python manage.py test \ | ||
| search.tests.test_engines \ | ||
| search.tests.test_course_discovery.TestNone \ | ||
| search.tests.test_course_discovery.TestMockCourseDiscoverySearch \ | ||
| search.tests.test_course_discovery.TestElasticCourseDiscoverySearch \ | ||
| search.tests.test_views \ | ||
| search.tests.test_api_timing_events \ | ||
| search.tests.test_search_result_processor \ | ||
| search.tests.tests || true | ||
| @$(MAKE) elastic-down | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, I think we should not have separate If during development, someone wants to run only the ES tests or only the Meili tests, they could easily just run some command like Also, it would be better to run this with pytest instead of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the hardcoded lists and added one |
||
|
|
||
| elastic-up: create-test-network | ||
| @echo "Starting Elasticsearch..." | ||
| @docker compose up -d test_elasticsearch | ||
| @echo "Waiting for Elasticsearch to be healthy..." | ||
| @timeout 30 bash -c 'until curl -s http://localhost:9200/_cluster/health | grep -q "status"; do echo "Waiting..."; sleep 2; done' | ||
|
|
||
| elastic-down: | ||
| @echo "Shutting down Elasticsearch..." | ||
| docker compose down test_elasticsearch | ||
|
|
||
| create-test-network: | ||
| docker network inspect test_network >/dev/null 2>&1 || docker network create --driver bridge test_network | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,7 +18,33 @@ services: | |||||||||
| - data01:/usr/share/elasticsearch/data | ||||||||||
| ports: | ||||||||||
| - "9200:9200" | ||||||||||
| networks: | ||||||||||
| - test_network | ||||||||||
|
|
||||||||||
| test_meilisearch: | ||||||||||
| image: getmeili/meilisearch:v1.7 | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||
| container_name: test_meilisearch | ||||||||||
| ports: | ||||||||||
| - "7700:7700" | ||||||||||
| networks: | ||||||||||
| - test_network | ||||||||||
| environment: | ||||||||||
| MEILISEARCH_MASTER_KEY: test_master_key | ||||||||||
| MEILISEARCH_URL: http://localhost:7700 | ||||||||||
| healthcheck: | ||||||||||
| test: ["CMD", "curl", "-f", "http://localhost:7700/health"] | ||||||||||
| interval: 2s | ||||||||||
| timeout: 1s | ||||||||||
| retries: 10 | ||||||||||
|
|
||||||||||
| version: '3.8' | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to top. |
||||||||||
|
|
||||||||||
| volumes: | ||||||||||
| data01: | ||||||||||
| driver: local | ||||||||||
|
|
||||||||||
| networks: | ||||||||||
| test_network: | ||||||||||
| name: test_network | ||||||||||
| driver: bridge | ||||||||||
| external: true | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,7 @@ def course_discovery_search( | |
| from_=0, | ||
| field_dictionary=None, | ||
| enable_course_sorting_by_start_date=False, | ||
| is_multivalue=False, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document this setting. Also, why would we even want
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this was done with backward compatibility in mind. The intention was to keep the current endpoint (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense for compatibility with the current endpoint. Would we even want to support single-facet search in the new API/endpoint though? Why not just multi-search only?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might actually be right, but this seems like business decision territory, and if we are going to change it at this point, I think we'd better involve other team members to make an informed decision. FYI @cmltaWt0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Serj-N Absolutely! I was going to comment on this. It was a business decision to move forward with multi-selection regardless of the current unstable state of the edx-search codebase.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All I care about is that, for the Ulmo release, operators can use both the legacy Course Catalog and the Course Catalog MFE. For the MFE we need the multifaceted search. If multifaceted also works with the legacy pages, then sure, lets stop supporting single facet. But I haven't done that testing myself so I don't know what would work.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not going to block this PR on either of these things, but from what I've found:
|
||
| ): | ||
| """ | ||
| Course Discovery activities against the search engine index of course details | ||
|
|
@@ -166,6 +167,7 @@ def course_discovery_search( | |
| exclude_dictionary=exclude_dictionary, | ||
| aggregation_terms=course_discovery_aggregations(), | ||
| sort_by=sort_by, | ||
| is_multivalue=is_multivalue, | ||
| ) | ||
|
|
||
| return results | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,7 @@ | |||||||||||||||
| RESERVED_CHARACTERS = "+=><!(){}[]^~*:\\/&|?" | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def _translate_hits(es_response): | ||||||||||||||||
| def _translate_hits(es_response, aggregation_terms, is_multivalue=False): | ||||||||||||||||
| """ | ||||||||||||||||
| Provide result set in our desired format from elasticsearch results. | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -99,7 +99,7 @@ def translate_result(result): | |||||||||||||||
|
|
||||||||||||||||
| def translate_agg_bucket(bucket, agg_result): | ||||||||||||||||
| """ | ||||||||||||||||
| Any conversion from ES aggregations result into our search engine syntax | ||||||||||||||||
| Convert ES aggregation result following our search engine syntax. | ||||||||||||||||
|
|
||||||||||||||||
| agg_result argument needs for getting total number of | ||||||||||||||||
| documents per bucket. | ||||||||||||||||
|
|
@@ -109,19 +109,29 @@ def translate_agg_bucket(bucket, agg_result): | |||||||||||||||
| :return: dict | ||||||||||||||||
| """ | ||||||||||||||||
| agg_item = agg_result[bucket] | ||||||||||||||||
| terms = { | ||||||||||||||||
| bucket["key"]: bucket["doc_count"] | ||||||||||||||||
| for bucket in agg_item["buckets"] | ||||||||||||||||
| } | ||||||||||||||||
| total_docs = ( | ||||||||||||||||
| agg_result[_get_total_doc_key(bucket)]["value"] | ||||||||||||||||
| + agg_item["sum_other_doc_count"] | ||||||||||||||||
| + agg_item["doc_count_error_upper_bound"] | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| if is_multivalue: | ||||||||||||||||
| values_agg = agg_item["values"] | ||||||||||||||||
| terms = { | ||||||||||||||||
| bucket["key"]: bucket["doc_count"] | ||||||||||||||||
| for bucket in values_agg["buckets"] | ||||||||||||||||
|
Comment on lines
+114
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: it's easier to see the difference between this part of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what you mean here: the two dict comprehensions seem to be structrured in the same way - the only difference being the object after "in"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, don't worry about this one, which was really a nitpick. I got a bit confused reading this code because these two parts look exactly the same (but they're not):
I think it's slightly more clear if you write it this way, which makes it more obvious that they're different:
I realize now my suggestion above in my first comment had a bug/error, so yeah don't worry about it. |
||||||||||||||||
| } | ||||||||||||||||
| total_docs = sum(terms.values()) | ||||||||||||||||
| else: | ||||||||||||||||
| terms = { | ||||||||||||||||
| bucket["key"]: bucket["doc_count"] | ||||||||||||||||
| for bucket in agg_item["buckets"] | ||||||||||||||||
| } | ||||||||||||||||
| total_docs = ( | ||||||||||||||||
| agg_result[_get_total_doc_key(bucket)]["value"] | ||||||||||||||||
| + agg_item["sum_other_doc_count"] | ||||||||||||||||
| + agg_item["doc_count_error_upper_bound"] | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| return { | ||||||||||||||||
| "terms": terms, | ||||||||||||||||
| "total": total_docs, | ||||||||||||||||
| "other": agg_item["sum_other_doc_count"], | ||||||||||||||||
| "other": agg_item.get("sum_other_doc_count", 0), | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| results = list(map(translate_result, es_response["hits"]["hits"])) | ||||||||||||||||
|
|
@@ -132,11 +142,19 @@ def translate_agg_bucket(bucket, agg_result): | |||||||||||||||
| "results": results, | ||||||||||||||||
| } | ||||||||||||||||
| if "aggregations" in es_response: | ||||||||||||||||
| response["aggs"] = { | ||||||||||||||||
| bucket: translate_agg_bucket(bucket, es_response["aggregations"]) | ||||||||||||||||
| for bucket in es_response["aggregations"] | ||||||||||||||||
| if "total_" not in bucket | ||||||||||||||||
| } | ||||||||||||||||
| if is_multivalue: | ||||||||||||||||
| global_aggs = es_response["aggregations"].get("global_aggs", {}) | ||||||||||||||||
| response["aggs"] = { | ||||||||||||||||
| facet: translate_agg_bucket(facet, global_aggs) | ||||||||||||||||
| for facet in global_aggs | ||||||||||||||||
| if facet in aggregation_terms | ||||||||||||||||
| } | ||||||||||||||||
| else: | ||||||||||||||||
| response["aggs"] = { | ||||||||||||||||
| bucket: translate_agg_bucket(bucket, es_response["aggregations"]) | ||||||||||||||||
| for bucket in es_response["aggregations"] | ||||||||||||||||
| if "total_" not in bucket | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return response | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -241,6 +259,47 @@ def _process_aggregation_terms(aggregation_terms): | |||||||||||||||
| return elastic_aggs | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def _process_multivalue_aggregations(aggregation_terms, field_dictionary): | ||||||||||||||||
| """ | ||||||||||||||||
| Calculate facet distributions for multi-value faceted search. | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I suggest this wording because "Calculate facet distributions" sounds like post-processing of the results, but this is used to generate the query. Also, could you please add a realistic example of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, your version gives a much more accurate idea. Changed as suggested and added typical arguments to the docstring.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, thanks! |
||||||||||||||||
| """ | ||||||||||||||||
| aggs = {} | ||||||||||||||||
| for facet_field, options in aggregation_terms.items(): | ||||||||||||||||
| filters_excluding_facet = { | ||||||||||||||||
| field: value for field, value in field_dictionary.items() | ||||||||||||||||
| if field != facet_field | ||||||||||||||||
| } | ||||||||||||||||
| filter_clauses = [ | ||||||||||||||||
| _get_filter_field(field, value) | ||||||||||||||||
| for field, value in filters_excluding_facet.items() | ||||||||||||||||
| if value | ||||||||||||||||
| ] | ||||||||||||||||
| facet_filter = { | ||||||||||||||||
| "bool": { | ||||||||||||||||
| "must": filter_clauses | ||||||||||||||||
| } | ||||||||||||||||
| } if filter_clauses else {"match_all": {}} | ||||||||||||||||
|
|
||||||||||||||||
| aggs[facet_field] = { | ||||||||||||||||
| "filter": facet_filter, | ||||||||||||||||
| "aggs": { | ||||||||||||||||
| "values": { | ||||||||||||||||
| "terms": { | ||||||||||||||||
| "field": facet_field, | ||||||||||||||||
| **options | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return { | ||||||||||||||||
| "global_aggs": { | ||||||||||||||||
| "global": {}, | ||||||||||||||||
| "aggs": aggs | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| class ElasticSearchEngine(SearchEngine): | ||||||||||||||||
| """ | ||||||||||||||||
| ElasticSearch implementation of SearchEngine abstraction | ||||||||||||||||
|
|
@@ -654,8 +713,13 @@ def search(self, | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| body = {"query": query} | ||||||||||||||||
|
|
||||||||||||||||
| is_multivalue = kwargs.pop("is_multivalue", False) | ||||||||||||||||
| if aggregation_terms: | ||||||||||||||||
| body["aggs"] = _process_aggregation_terms(aggregation_terms) | ||||||||||||||||
| if is_multivalue: | ||||||||||||||||
| body["aggs"] = _process_multivalue_aggregations(aggregation_terms, field_dictionary) | ||||||||||||||||
| else: | ||||||||||||||||
| body["aggs"] = _process_aggregation_terms(aggregation_terms) | ||||||||||||||||
| if sort_by: | ||||||||||||||||
| body["sort"] = self._transform_sort_by(sort_by) | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -668,7 +732,7 @@ def search(self, | |||||||||||||||
| log.exception("error while searching index - %r", ex) | ||||||||||||||||
| raise | ||||||||||||||||
|
|
||||||||||||||||
| return _translate_hits(es_response) | ||||||||||||||||
| return _translate_hits(es_response, aggregation_terms, is_multivalue) | ||||||||||||||||
|
|
||||||||||||||||
| def _transform_sort_by(self, fields: list[SortField]): | ||||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||


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.
These settings seem to be specified in three different places - the Makefile, the test
settings.pyfile, and thedocker-compose.ymlfile. Could we not just set them in one, e.g. the settings.py 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.
You're right, the value in settings will suffice. Removed from Makefile and docker-compose.yml