Change #1080045 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] WIP: search for EntitySchema by label

https://gerrit.wikimedia.org/r/1080045

Change #1080050 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[integration/config@master] Zuul: [mediawiki/extensions/EntitySchema] Add Elastica+CirrusSearch phan deps

https://gerrit.wikimedia.org/r/1080050

Change #1080050 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/EntitySchema] Add Elastica+CirrusSearch phan deps

https://gerrit.wikimedia.org/r/1080050

@dcausse following up from our discussion in T376250 – now that we reuse the fields from WikibaseCirrusSearch (both for the definitions and for the actual field data), should we also try to reuse as much as possible of EntitySearchElastic for our own EntitySearchHelper implementation? (Currently the WIP patch attached above copy+pastes a lot of that code instead.)

In T376252#10348116, @Lucas_Werkmeister_WMDE wrote:

@dcausse following up from our discussion in T376250 – now that we reuse the fields from WikibaseCirrusSearch (both for the definitions and for the actual field data), should we also try to reuse as much as possible of EntitySearchElastic for our own EntitySearchHelper implementation? (Currently the WIP patch attached above copy+pastes a lot of that code instead.)

I had a quick look and I'm not sure... I believe that EntitySchema might not need such a complicated query (my understanding is that this namespace is not meant to grow very big?) but on the other hand I don't see why the current query would not work for schemas... if you don't plan to invest in a custom query for EntitySchema and had planned to use the exact same query strategy (with similar config/profiles) it might make sense to extract expandGenericProfile & getElasticSearchQuery into a re-usable component (possibly extending Elastica's AbstractQuery) but in general, when it comes to query building we found few overlaps in the past and rarely re-used such big queries across different contexts...
What might make sense to re-use is ElasticTermResult, I'm not clear how you plan to return an EntitySchema as a Term but ElasticTermResult::extractTermFromHighlight embarks some fiddly logic around label & aliases extraction from elastic result set that are definitely worth re-using if possible.

Thanks! I agree that EntitySchema probably doesn’t need such a complicated query (it’s going to remain a relatively small namespace compared to items, and I don’t think we expect a lot of EntitySchemas with similar labels which would have to be ranked like items), but I’m still inclined to reuse the existing query, for two main reasons: I don’t know how to write Elasticsearch queries 😅 and also, I assume the structure of the query is directly related to the structure of the search fields, and if the search fields stay WikibaseCirrusSearch’s responsibility, then we probably don’t want to hard-code a lot of knowledge about those fields separately in EntitySchema. (Though I suppose WikibaseCirrusSearch could provide a separate search query for EntitySchema, further blurring the line between EntitySchema and other entity types.)

A more practical question: in the current Gerrit change, a lot of the code is copied from WikibaseCirrusSearch, but I refactored the strings to replace hard-coded labels and labels_all with references to the constants LabelsField::NAME and AllLabelsField::NAME, which currently makes the diff between the original code and the copied version annoying to review. Do you think it would make sense to make these changes in WikibaseCirrusSearch too? (I.e. change 'labels_all.prefix' to "{$allLabelsName}.prefix" etc.) If not, I would just revert that part in the Gerrit change and then we can see the more meaningful differences more easily.

In T376252#10352379, @Lucas_Werkmeister_WMDE wrote:

Thanks! I agree that EntitySchema probably doesn’t need such a complicated query (it’s going to remain a relatively small namespace compared to items, and I don’t think we expect a lot of EntitySchemas with similar labels which would have to be ranked like items), but I’m still inclined to reuse the existing query, for two main reasons: I don’t know how to write Elasticsearch queries 😅 and also, I assume the structure of the query is directly related to the structure of the search fields, and if the search fields stay WikibaseCirrusSearch’s responsibility, then we probably don’t want to hard-code a lot of knowledge about those fields separately in EntitySchema. (Though I suppose WikibaseCirrusSearch could provide a separate search query for EntitySchema, further blurring the line between EntitySchema and other entity types.)

Sure, I think it's reasonable to assume that we might need a re-uable query that does completion on the labels field (hopefully without anything specific to EntitySchemas), I think the query is complicated enough that it might justify depending even further on WBCS, I'll give it a try, if this works this might help to remove most of the complexity you have in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EntitySchema/+/1080045/11/src/Wikibase/Search/EntitySchemaSearchHelper.php by keeping only the glue code in getRankedSearchResults.

A more practical question: in the current Gerrit change, a lot of the code is copied from WikibaseCirrusSearch, but I refactored the strings to replace hard-coded labels and labels_all with references to the constants LabelsField::NAME and AllLabelsField::NAME, which currently makes the diff between the original code and the copied version annoying to review. Do you think it would make sense to make these changes in WikibaseCirrusSearch too? (I.e. change 'labels_all.prefix' to "{$allLabelsName}.prefix" etc.) If not, I would just revert that part in the Gerrit change and then we can see the more meaningful differences more easily.

Yes, this would make total sense, but if I manage to get a re-usable query in place we might get that for free, I'll to get a quick WIP patch in place to illustrate the discussion.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseCirrusSearch/+/1098514 is a suggestion to help reduce the complexity of EntitySchemaSearchHelper. We might possibly want to use a separate context (different than EntitySearchElastic::CONTEXT_WIKIBASE_PREFIX) so that profiles are different (possibly simpler) for EntitySchema searches.
I did not look into how to generalize ESElasticTermResult (not sure it changed much compared to the WBCS one btw?).

Thanks! I think there’s a lot of search-specific stuff in it that I don’t understand :D but it looks promising to me. I’ll see if I can put up a version of the EntitySchema WIP patch which depends on that change.

One thing I noticed is the handling of $normalizedId based on the optional ?EntityIdParser $idParser – if the parser isn’t specified (presumably EntitySchema would set it to null), one of the matches is left out of the query. Is that what we want, or should EntitySchema have some different way to specify the normalized ID, e.g. by passing in a callable? (I’m not sure what that match does, to be honest… is this the part that lets you search for an entity by its ID? But I thought that was done by the separate EntityIdSearchHelper class, whose results are combined with EntitySearchElastic by a CombinedEntitySearchHelper.)

I’ve updated the change and it seems to work well locally. (Note that it’s also based on the patches for T380720, which includes a WBCS patch.) EntitySchemaSearchHelper definitely looks a lot nicer now!

I did not look into how to generalize ESElasticTermResult (not sure it changed much compared to the WBCS one btw?).

Yeah, I don’t think it changed very much either. (Now that we’re also putting descriptions in the index, the difference is even less than it used to be.) I think the only real difference is how we parse the $sourceData['title'] and which parameters we pass into the TermSearchResult (the $entityId parameter is null, so we have to pass additional $metaData instead which would otherwise be automatically added based on the entity ID).

In T376252#10361930, @Lucas_Werkmeister_WMDE wrote:

One thing I noticed is the handling of $normalizedId based on the optional ?EntityIdParser $idParser – if the parser isn’t specified (presumably EntitySchema would set it to null), one of the matches is left out of the query. Is that what we want, or should EntitySchema have some different way to specify the normalized ID, e.g. by passing in a callable? (I’m not sure what that match does, to be honest… is this the part that lets you search for an entity by its ID? But I thought that was done by the separate EntityIdSearchHelper class, whose results are combined with EntitySearchElastic by a CombinedEntitySearchHelper.)

I think the purpose of the match on the normalized ID is to help people copy/pasting some text possibly containing the entity ID (originally added in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/387025) :

• (q123) -> Q123
• q123 -> Q123
• url_or_a_path/q123 -> Q123

It makes sense to generalize this a bit more and allow a simple optional callable like function ( string $str ): ?string so that you can still benefit from it if you want to.
The CombinedEntitySearchHelper deserves a slightly different purpose by allowing to search the DB mainly to benefit from the fact that mysql replicas are generally less lagged than elastic while the former adds a bit more tolerance in how users find explicit entity IDs after a quick copy/paste of text containing the Q id.

In T376252#10362166, @Lucas_Werkmeister_WMDE wrote:

I did not look into how to generalize ESElasticTermResult (not sure it changed much compared to the WBCS one btw?).

Yeah, I don’t think it changed very much either. (Now that we’re also putting descriptions in the index, the difference is even less than it used to be.) I think the only real difference is how we parse the $sourceData['title'] and which parameters we pass into the TermSearchResult (the $entityId parameter is null, so we have to pass additional $metaData instead which would otherwise be automatically added based on the entity ID).

Thanks! then it might make sense to wait for descriptions to be there to see if & how we can reduce code duplication there. Out of curiosity, are you planning on shipping https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EntitySchema/+/1080045 first or wait for descriptions? (or they're totally unrelated perhaps?).

In T376252#10364675, @dcausse wrote:

It makes sense to generalize this a bit more and allow a simple optional callable like function ( string $str ): ?string so that you can still benefit from it if you want to.

That makes sense to me – EntitySearchElastic would then pass in a callable that calls the entity ID parser and returns null if it throws.

In T376252#10364705, @dcausse wrote:

Thanks! then it might make sense to wait for descriptions to be there to see if & how we can reduce code duplication there. Out of curiosity, are you planning on shipping https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EntitySchema/+/1080045 first or wait for descriptions? (or they're totally unrelated perhaps?).

I was expecting descriptions to be merged first, and in fact they’ve already been merged now ^^ (I hope you’re okay with adding those fields.)

In T376252#10365276, @Lucas_Werkmeister_WMDE wrote:

In T376252#10364675, @dcausse wrote:

It makes sense to generalize this a bit more and allow a simple optional callable like function ( string $str ): ?string so that you can still benefit from it if you want to.

That makes sense to me – EntitySearchElastic would then pass in a callable that calls the entity ID parser and returns null if it throws.

Hm, looking at it now I’m not sure what this would look like in practice. Do we change EntitySearchUtils::normalizeId() to make the ID parser optional? (Because IIUC that method does the “strip away parentheses” part that we may also want for EntitySchema…)

Maybe it’s easier to leave that alone for now ^^

Change #1098957 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseCirrusSearch@master] [WIP] Make ElasticTermResult reusable in EntitySchema

https://gerrit.wikimedia.org/r/1098957

Change #1098958 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] WIP: use WikibaseCirrusSearch ElasticTermResult

https://gerrit.wikimedia.org/r/1098958

Alright, I’ve uploaded a new patch to make ElasticTermResult reusable and an EntitySchema patch to use it. It’s not perfect, but hopefully it’s a starting point (and it seems to work locally). What do you think?

I was expecting descriptions to be merged first, and in fact they’ve already been merged now ^^ (I hope you’re okay with adding those fields.)

makes total sense thanks for adding them now, they'll definitely be useful if/when we do fulltext searches on schemas

Hm, looking at it now I’m not sure what this would look like in practice. Do we change EntitySearchUtils::normalizeId() to make the ID parser optional? (Because IIUC that method does the “strip away parentheses” part that we may also want for EntitySchema…)

I'll take a closer look, if there's an easy solution let's go for it, users might have some habits and if we're not consistent for entity schemas this could be slightly frustrating.

Alright, I’ve uploaded a new patch to make ElasticTermResult reusable and an EntitySchema patch to use it. It’s not perfect, but hopefully it’s a starting point (and it seems to work locally). What do you think?

Thanks, I took a quick look and it looks great, left a small regarding the same concern you have, let's continue the discussion in gerrit, but overall it seems like that we're heading in the right direction! :)

Change #1099206 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Introduce TermSearchResult::getEntityIdSerialization()

https://gerrit.wikimedia.org/r/1099206

Change #1099662 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/WikibaseCirrusSearch@master] Don’t hard-code field names in ElasticTermResult

https://gerrit.wikimedia.org/r/1099662

Change #1098958 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/EntitySchema@master] WIP: use WikibaseCirrusSearch ElasticTermResult

Reason:

squashed into the parent change, as it sounds like we’ll follow this approach

https://gerrit.wikimedia.org/r/1098958

The patch implementing the actual search is now mostly out of WIP territory, except for the tests. And looking at what EntitySchemaSearchHelper looks like now, with most of the code living in WikibaseCirrusSearch (likewise for ESElasticTermResult, I suppose), I’m honestly not sure how we should test it at all. I don’t see much to unit-test – mostly it’s calling into WBCS anyway, and also several of the interesting calls (like LabelsCompletionQuery::build() and new WikibasePrefixSearcher()) aren’t injected, so we can’t mock them easily. I have no idea how to integration-test CirrusSearch – I see some code for that in WikibaseCirrusSearch (EntitySearchElasticTest, and I think also SearchEntitiesIntegrationTest), but that feels like it would get deeply into WBCS internals again (look at all the JSON files in extensions/WikibaseCirrusSearch/tests/phpunit/data/entitySearch/), which we’ve been trying to avoid. If the selenium CI job has Elasticsearch installed (not sure), then I think a browser test could work, but currently it’s failing (the screenshot shows “no match was found”) – I think it’s because we would need to wait for the job to run first, and I’m not sure how to do that.

@dcausse or others, any thoughts?

Change #1099662 merged by jenkins-bot:

[mediawiki/extensions/WikibaseCirrusSearch@master] Don’t hard-code field names in ElasticTermResult

https://gerrit.wikimedia.org/r/1099662

In T376252#10371919, @Lucas_Werkmeister_WMDE wrote:

The patch implementing the actual search is now mostly out of WIP territory, except for the tests. And looking at what EntitySchemaSearchHelper looks like now, with most of the code living in WikibaseCirrusSearch (likewise for ESElasticTermResult, I suppose), I’m honestly not sure how we should test it at all. I don’t see much to unit-test – mostly it’s calling into WBCS anyway, and also several of the interesting calls (like LabelsCompletionQuery::build() and new WikibasePrefixSearcher()) aren’t injected, so we can’t mock them easily. I have no idea how to integration-test CirrusSearch – I see some code for that in WikibaseCirrusSearch (EntitySearchElasticTest, and I think also SearchEntitiesIntegrationTest), but that feels like it would get deeply into WBCS internals again (look at all the JSON files in extensions/WikibaseCirrusSearch/tests/phpunit/data/entitySearch/), which we’ve been trying to avoid. If the selenium CI job has Elasticsearch installed (not sure), then I think a browser test could work, but currently it’s failing (the screenshot shows “no match was found”) – I think it’s because we would need to wait for the job to run first, and I’m not sure how to do that.

@dcausse or others, any thoughts?

Testing Cirrus code is generally a bit painful (elastic is not setup in CI), what we've been doing so far is:

• have fixture files to exercice the code but the main goal of these is to capture unexpected changes in the generate query, I think it does not make much sense to have this since most of the elastic query building logic is tested by WBCS
• real integration tests, these are run outside of CI by a bot named cindy, but these are run only on Cirrus patches and I'm not sure we've configured Wikibase in there anyways

None of these approaches seem appropriate for EntitySchema imo. I think that what is left untested is mainly the glue which is generally the part harder to test in unit test. If we want some of the code to be exercised (with few very broad assertions) you could add a test running EntitySchemaSearchHelper with some cirrus debug options this should tell cirrus to return the raw query in which you could perhaps do very basic assertions, but I'm unsure if this is worthwhile... On the other hand ESElasticTermResult::getTermSearchResult might be testable?

Alright, thanks. I agree that ESElasticTermResult looks testable (using TestingAccessWrapper to get at the protected method). I think for the rest I could live with not having tests.

Although it’s probably a good idea to make one more change. In the PS14 commit message, I wrote:

There is still one TODO left, which I think is legitimate – at the moment, searching for an EntitySchema ID doesn’t seem to work. Either we make that work within this helper (by passing an $idNormalizer into LabelsCompletionQuery::build()?), or we implement it in a separate helper (which would be basically equivalent to the old code) and then use a CombinedEntitySearchHelper to merge the two (this is the approach used in Wikibase).

I removed that part from the commit message after I found that $idNormalizer worked just fine, but now I think it would be good to go for the CombinedEntitySearchHelper approach after all: this way, there’s still one search implementation which a) is unit-tested, and b) works in the browser tests.

I implemented the CombinedEntitySearchHelper approach but the browser test still fails. I assume the search request gets the same result that I see when I stop Elasticsearch locally:

{"errors":[{"code":"cirrussearch-backend-error","module":"wbsearchentities","*":"We could not complete your search due to a temporary problem. Please try again later."}],"*":"See http://localhost/wiki1/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/postorius/lists/mediawiki-api-announce.lists.wikimedia.org/> for notice of API deprecations and breaking changes."}

Currently the condition for whether to enable the WBCS-based search is just ExtensionRegistry::getInstance()->isLoaded( 'WikibaseCirrusSearch' ) – I’m guessing it should also take into account whether the search is actually enabled (assuming that in CI it isn’t enabled). @dcausse would it be okay for EntitySchema to access WikibaseSearchConfig::enabled() for this check? (Whether by calling WikibaseSearchConfig::newFromGlobals(), making Wikibase\Search\Elastic\Hooks::getWBCSConfig() public, calling $configFactory->makeConfig( 'WikibaseCirrusSearch' ), or something else.) Or should we use something else to check this condition?

In T376252#10376061, @Lucas_Werkmeister_WMDE wrote:

@dcausse would it be okay for EntitySchema to access WikibaseSearchConfig::enabled() for this check? (Whether by calling WikibaseSearchConfig::newFromGlobals(), making Wikibase\Search\Elastic\Hooks::getWBCSConfig() public, calling $configFactory->makeConfig( 'WikibaseCirrusSearch' ), or something else.) Or should we use something else to check this condition?

WikibaseSearchConfig::enabled() should be fine, IIRC WikibaseMediaInfo relies on this config var to disable all cirrus dependent components too (it directly inspects global $wgWBCSUseCirrus so getting the WikibaseSearchConfig is preferable I think?)

Thanks, I’ve gone with that for now. (I still need to add those tests for ESElasticTermResult.)

In T376252#10376925, @Lucas_Werkmeister_WMDE wrote:

(I still need to add those tests for ESElasticTermResult.)

Done. With that, I think I’m done with this test and it’s fully ready for review (though Introduce TermSearchResult::getEntityIdSerialization() has already been ready for review for a few days).

In T376252#10375264, @Lucas_Werkmeister_WMDE wrote:

…now I think it would be good to go for the CombinedEntitySearchHelper approach after all: this way, there’s still one search implementation which a) is unit-tested, and b) works in the browser tests.

and c) still allows people to search for EntitySchemas by label while the search index is being updated for the changes here, hopefully? I notice that E43?action=cirrusDump doesn’t show the labels field yet, even though it was edited earlier today and IIUC should contain the new fields (does the Search team need to do some manual update to the index config first?).

In T376252#10380103, @Lucas_Werkmeister_WMDE wrote:

I notice that E43?action=cirrusDump doesn’t show the labels field yet, even though it was edited earlier today and IIUC should contain the new fields (does the Search team need to do some manual update to the index config first?).

The data does not seem to be populated by WBCS and/or EntitySchema when looking at EntitySchema:E43 but I can see them on testwikidata EntitySchema:E17. At the time of writing testwikidata is running 1.44.0-wmf.6 but wikidata is on 1.44.0-wmf.5, could this explain what we see? If not there must some config that we have missed?

In T376252#10381546, @dcausse wrote:

In T376252#10380103, @Lucas_Werkmeister_WMDE wrote:

I notice that E43?action=cirrusDump doesn’t show the labels field yet, even though it was edited earlier today and IIUC should contain the new fields (does the Search team need to do some manual update to the index config first?).

testwikidata is running 1.44.0-wmf.6 but wikidata is on 1.44.0-wmf.5, could this explain what we see?

That was it I think, with wikidata now on wmf.6 EntitySchema:E43 is now producing the labels, I'll trigger a refresh of all the schemas to populate them.

Mentioned in SAL (#wikimedia-operations) [2024-12-05T10:43:56Z] <dcausse> reindexed all wikidata entity schemas (T376252)

Okay, then I guess I just got confused about the train status ^^ thanks!

Change #1099206 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Introduce TermSearchResult::getEntityIdSerialization()

https://gerrit.wikimedia.org/r/1099206

Change #1098957 merged by jenkins-bot:

[mediawiki/extensions/WikibaseCirrusSearch@master] Make ElasticTermResult reusable in EntitySchema

https://gerrit.wikimedia.org/r/1098957

Change #1080045 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Search for EntitySchema by label

https://gerrit.wikimedia.org/r/1080045