feat: index records with suggestions for search engine #4317

frascuchon · 2023-11-24T13:15:17Z

Description

This PR adds support for indexing suggestions when indexing records in the search index.

In order to be sure that all record attributes are passed when indexing records, a workaround has been implemented by forcing a record, responses, and suggestions to refresh before indexing them. Otherwise is hard to add that info when loading records in the current code base.

This behavior must be reviewed and simplified cc @gabrielmbmb @jfcalvo

Changes related to tests will be moved into a separate PR since extra refactoring work must be done. Once this extra PR is merged here, the PR will be marked as ready for review. Related PR #4318

Refs: #3849

Closes #4230

Also, simplify and remove all extra mapper componets and reuse some code blocks regarding the elastic field definitions

for more information, see https://pre-commit.ci

src/argilla/server/contexts/datasets.py

jfcalvo · 2023-11-24T16:09:52Z

src/argilla/server/contexts/datasets.py

+        await refresh_records(records)
        await search_engine.index_records(dataset, records)


Not sure if it's the best approach but to explicitly link the preload of the records with the search engine maybe we can do the following:

Suggested change

await refresh_records(records)

await search_engine.index_records(dataset, records)

await search_engine.index_records(

dataset,

await _preload_records_associations(records),

)

Or if you don't want to do that:

Suggested change

await refresh_records(records)

await search_engine.index_records(dataset, records)

await _preload_records_associations(records)

await search_engine.index_records(dataset, records)

jfcalvo · 2023-11-24T16:17:01Z

src/argilla/server/contexts/datasets.py

+            await refresh_records(records)
            await search_engine.index_records(dataset, records)


As said before we can do:

Suggested change

await refresh_records(records)

await search_engine.index_records(dataset, records)

await search_engine.index_records(

dataset,

await _preload_records_associations(records)

)

If you don't want to do that in any case you can change it to:

Suggested change

await refresh_records(records)

await search_engine.index_records(dataset, records)

await _preload_records_associations(records)

await search_engine.index_records(dataset, records)

jfcalvo · 2023-11-24T16:21:10Z

src/argilla/server/contexts/datasets.py

+            await refresh_records([record])
            await search_engine.index_records(record.dataset, [record])


As said before:

Suggested change

await refresh_records([record])

await search_engine.index_records(record.dataset, [record])

await search_engine.index_records(

record.dataset,

[await _preload_record_associations(record)]

)

Or:

Suggested change

await refresh_records([record])

await search_engine.index_records(record.dataset, [record])

await _preload_record_associations(record)

await search_engine.index_records(record.dataset, [record])

For the first approach, that new function should return the loaded record list. We can review it together

jfcalvo · 2023-11-24T16:29:20Z

src/argilla/server/search_engine/commons.py

    UserResponseStatusFilter,
 )

 ALL_RESPONSES_STATUSES_FIELD = "all_responses_statuses"


-class SearchDocumentGetter(GetterDict):


Loving these code deletions.

jfcalvo · 2023-11-24T16:40:58Z

src/argilla/server/search_engine/commons.py

+        raise Exception(f"Index configuration for metadata property of type {property_type} cannot be generated")
+
+
+def es_mapping_for_question_type(question_type: QuestionType):


Maybe I'm missing something (tell me if I'm wrong) but should not be this function better named es_mapping_for_question and receive a question instead of the type?

Suggested change

def es_mapping_for_question_type(question_type: QuestionType):

def es_mapping_for_question(question: Question):

Inside you can do the if question.type conditional.

This PR changes and unifies tests related to the search engine. - All tests which will return the same results for the different engine implementations are placed in `test_commons.py` - Specific tests for elasticsearch implementation are placed in `test_elasticsearch.py` - Specific tests for opensearch implementation are placed in `test_elasticsearch.py` Refs: #4318 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Co-authored-by: José Francisco Calvo <jose@argilla.io>

…of github.com:argilla-io/argilla into feat/index-records-with-suggestions-for-search-engine

for more information, see https://pre-commit.ci

…of github.com:argilla-io/argilla into feat/index-records-with-suggestions-for-search-engine

github-actions · 2023-11-27T06:31:07Z

The URL of the deployed environment for this PR is https://argilla-quickstart-pr-4317-ki24f765kq-no.a.run.app

gabrielmbmb · 2023-11-27T08:58:54Z

src/argilla/server/contexts/datasets.py

+    await record.awaitable_attrs.vectors
+
+    for response in record.responses:
+        await response.awiatable_attrs.user


Suggested change

await response.awiatable_attrs.user

await response.awaitable_attrs.user

gabrielmbmb · 2023-11-27T09:00:55Z

src/argilla/server/contexts/datasets.py

+    for suggestion in record.suggestions:
+        await suggestion.awaitable_attrs.question


I think that here we could do await record.dataset.awaitable_attrs.questions

gabrielmbmb · 2023-11-27T09:04:29Z

src/argilla/server/search_engine/commons.py

@@ -500,6 +469,64 @@ def _build_response_status_filter(status_filter: UserResponseStatusFilter) -> Di
    def _inverse_vector(self, vector_value: List[float]) -> List[float]:
        return [vector_value[i] * -1 for i in range(0, len(vector_value))]

+    def _map_record_to_es_document(self, record: Record) -> Dict[str, Any]:


gabrielmbmb · 2023-11-27T09:09:37Z

src/argilla/server/contexts/datasets.py

+        await _preload_record_associations(record)
+
+
+async def _preload_record_associations(record: Record) -> None:


ignore my comments, if at the end we're going to query the data explicitly

… to only do one query per record

frascuchon added 3 commits November 24, 2023 14:11

chore: define property for question type

c70eb65

feat: Adding record suggestions when indexing records

6a3e776

Also, simplify and remove all extra mapper componets and reuse some code blocks regarding the elastic field definitions

chore: remove unused components

a35536a

frascuchon requested review from jfcalvo and gabrielmbmb November 24, 2023 13:15

frascuchon self-assigned this Nov 24, 2023

frascuchon changed the title ~~Feat/index records with suggestions for search engine~~ feat: index records with suggestions for search engine Nov 24, 2023

[pre-commit.ci] auto fixes from pre-commit.com hooks

f5d00c7

for more information, see https://pre-commit.ci

frascuchon linked an issue Nov 24, 2023 that may be closed by this pull request

[FEATURE] Index records in elasticsearch index with suggestion info (agent, value , and score) #4230

Closed

fix: Enable required attributes when indexing records

cf58a17

frascuchon force-pushed the feat/index-records-with-suggestions-for-search-engine branch from 63f9ced to cf58a17 Compare November 24, 2023 14:01

jfcalvo requested changes Nov 24, 2023

View reviewed changes

frascuchon and others added 7 commits November 27, 2023 06:54

chore: Apply suggestion

5ad8561

Update src/argilla/server/contexts/datasets.py

a3ddb91

Co-authored-by: José Francisco Calvo <jose@argilla.io>

Merge branch 'feat/index-records-with-suggestions-for-search-engine' …

7365507

…of github.com:argilla-io/argilla into feat/index-records-with-suggestions-for-search-engine

[pre-commit.ci] auto fixes from pre-commit.com hooks

3561262

for more information, see https://pre-commit.ci

chore: Apply suggestion

96298db

Merge branch 'feat/index-records-with-suggestions-for-search-engine' …

71b3afc

…of github.com:argilla-io/argilla into feat/index-records-with-suggestions-for-search-engine

frascuchon marked this pull request as ready for review November 27, 2023 06:03

fix: missing function renaming

c8a9084

gabrielmbmb requested changes Nov 27, 2023

View reviewed changes

jfcalvo added 2 commits November 27, 2023 13:30

feat: improve how relationships are loaded from database before index…

b45a89e

… to only do one query per record

fix: some failing tests

32faf78

jfcalvo approved these changes Nov 27, 2023

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 27, 2023

jfcalvo merged commit 75ccee4 into feature/responses-and-suggestion-filter Nov 27, 2023

jfcalvo deleted the feat/index-records-with-suggestions-for-search-engine branch November 27, 2023 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: index records with suggestions for search engine #4317

feat: index records with suggestions for search engine #4317

frascuchon commented Nov 24, 2023 •

edited

Loading

jfcalvo Nov 24, 2023

jfcalvo Nov 24, 2023

jfcalvo Nov 24, 2023

frascuchon Nov 27, 2023

jfcalvo Nov 24, 2023

jfcalvo Nov 24, 2023

github-actions bot commented Nov 27, 2023

gabrielmbmb Nov 27, 2023

gabrielmbmb Nov 27, 2023

gabrielmbmb Nov 27, 2023

gabrielmbmb Nov 27, 2023

		await refresh_records(records)
		await search_engine.index_records(dataset, records)

		await refresh_records([record])
		await search_engine.index_records(record.dataset, [record])

		raise Exception(f"Index configuration for metadata property of type {property_type} cannot be generated")


		def es_mapping_for_question_type(question_type: QuestionType):

	def es_mapping_for_question_type(question_type: QuestionType):
	def es_mapping_for_question(question: Question):

	await response.awiatable_attrs.user
	await response.awaitable_attrs.user

		for suggestion in record.suggestions:
		await suggestion.awaitable_attrs.question

		await _preload_record_associations(record)


		async def _preload_record_associations(record: Record) -> None:

feat: index records with suggestions for search engine #4317

feat: index records with suggestions for search engine #4317

Conversation

frascuchon commented Nov 24, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frascuchon commented Nov 24, 2023 •

edited

Loading