feat: Match synonyms and xx: entries when computing taxonomy suggestions #8190

stephanegigandet · 2023-03-13T14:13:08Z

Previously, taxonomy suggestions were only returned when the canonical taxonomy entry matched the input string from the user. Now we also try to match the input string to synonyms of the entry, including in the xx: wildcard language.

Also added a unit test to ease debugging.

stephanegigandet · 2023-03-13T14:13:53Z

...ntegration/expected_test_results/api_v3_taxonomy_suggestions/categories-term-strawberry.json

@@ -2,6 +2,7 @@
   "errors" : [],
   "status" : "success",
   "suggestions" : [
+      "Mint-flavoured syrup with sugar diluted in water",


This entry is because we have a weird CIQUAL category that applies to both strawberry and mint flavoured syrups... To be fixed in the taxonomy.

stephanegigandet · 2023-03-13T14:14:03Z

...egration/expected_test_results/api_v3_taxonomy_suggestions/categories-string-strawberry.json

@@ -2,6 +2,7 @@
   "errors" : [],
   "status" : "success",
   "suggestions" : [
+      "Mint-flavoured syrup with sugar diluted in water",


This entry is because we have a weird CIQUAL category that applies to both strawberry and mint flavoured syrups... To be fixed in the taxonomy.

stephanegigandet · 2023-03-13T14:14:15Z

tests/integration/expected_test_results/api_cgi_suggest/categories-string-strawberry.json

@@ -1,4 +1,5 @@
 [
+   "Mint-flavoured syrup with sugar diluted in water",


This entry is because we have a weird CIQUAL category that applies to both strawberry and mint flavoured syrups... To be fixed in the taxonomy.

stephanegigandet · 2023-03-13T14:15:30Z

...tegration/expected_test_results/api_v3_taxonomy_suggestions/packaging-recycling-fr-recy.json

@@ -8,7 +8,8 @@
      "Recycle in paper bin",
      "Recycle with drink cartons",
      "Recycle with plastics",
-      "Recycle with plastics - metal and bricks"
+      "Recycle with plastics - metal and bricks",
+      "Discard"


The extra "Discard" entry is added because the input "recy" partially matches the synonym "non-recyclable".

alexgarel

Cool ! That will help a lot.

I added some remarks.

alexgarel · 2023-03-21T09:40:59Z

lib/ProductOpener/Ecoscore.pm

+			my $target_material
+				= canonicalize_taxonomy_tag("en", "packaging_materials", $assignment_ref->{target_material});
+			my $source_material
+				= canonicalize_taxonomy_tag("en", "packaging_materials", $assignment_ref->{source_material});
+
+			if (not exists_taxonomy_tag("packaging_materials", $target_material)) {
+				die(      "target_material "
+						. $assignment_ref->{target_material}
+						. " does not exist in the packaging_materials taxonomy");
+			}
+			if (not exists_taxonomy_tag("packaging_materials", $source_material)) {
+				die(      "source_material "
+						. $assignment_ref->{source_material}
+						. " does not exist in the packaging_materials taxonomy");
+			}


a small sub to canonicalize and verify existence, would improve readability a lot. (usable also below)

I refactored canonicalize_taxonomy_tag so that it can indicate if the returned entry exists in the taxonomy.

Is it still to be pushed ?

yes, I'm still addressing the other suggestions

lib/ProductOpener/Ecoscore.pm

alexgarel · 2023-03-21T09:45:43Z

lib/ProductOpener/Ecoscore.pm

+			$ecoscore_data{packaging_materials}{$target} = $ecoscore_data{packaging_materials}{$source};
+			$properties{packaging_materials}{$target}{"ecoscore_score:en"}
+				= $ecoscore_data{packaging_materials}{$source}{"score"};


Shan't we verify that $ecoscore_data{packaging_materials}{$source} is defined ?

Sure I can add that.

alexgarel · 2023-03-21T10:00:19Z

lib/ProductOpener/TaxonomySuggestions.pm

+			# fuzzy match
+			elsif ($best_match eq "fuzzy") {
+				push @suggestions_f, $tag;
+			}
 		}
 	}


While we are at it, don't you think it would be a good opportunity to add a memoize on this function ? (and with perf problem we have at this moment, it could help) :-)

Why not. In the worse case scenario (huge taxonomy like categories, and a text query that does not match anything so we have to go through everything, it takes about 0.5 seconds in prod). I'm not sure we will gain much on smaller taxonomies, but we only have a bit of space to lose.

Another option is to use memcached (so that the cache is shared between processes), but then it might be tricky when we update things and get old cache results even after restarting Apache.

Oh yes, I didn't remember we where using multiple processes (and quite a lot) so memcached would really be needed. Only thing is that I didn't see ready to use memoize using memcached with a RLU strategy (MRU would be even better).

Right, I chose to use memcached, I'm refactoring some stuff around it.

alexgarel · 2023-03-21T10:03:02Z

tests/unit/taxonomy_suggestions.t

+		lc => "en",
+		string => "yog",
+		expected => ['Yogurts', 'Banana yogurts'],
+	},


can we add some more tests ?

I added a few more tests

Co-authored-by: Alex Garel <alex@garel.org>

codecov-commenter · 2023-03-21T15:31:39Z

Codecov Report

Merging #8190 (37b690f) into main (b989be3) will decrease coverage by 0.97%.
The diff coverage is 75.37%.

@@            Coverage Diff             @@
##             main    #8190      +/-   ##
==========================================
- Coverage   47.06%   46.09%   -0.97%     
==========================================
  Files         104      105       +1     
  Lines       20449    20523      +74     
  Branches     4650     4668      +18     
==========================================
- Hits         9624     9460     -164     
- Misses       9678     9936     +258     
+ Partials     1147     1127      -20

Impacted Files	Coverage Δ
lib/ProductOpener/Index.pm	`50.00% <ø> (-1.29%)`	⬇️
lib/ProductOpener/Orgs.pm	`14.56% <ø> (-0.83%)`	⬇️
lib/ProductOpener/Users.pm	`5.22% <ø> (-0.24%)`	⬇️
lib/ProductOpener/Display.pm	`4.67% <20.00%> (-0.02%)`	⬇️
lib/ProductOpener/Tags.pm	`37.87% <72.72%> (-16.21%)`	⬇️
lib/ProductOpener/Ecoscore.pm	`75.17% <73.91%> (-0.76%)`	⬇️
lib/ProductOpener/TaxonomySuggestions.pm	`54.92% <77.50%> (+42.32%)`	⬆️
tests/unit/taxonomy_suggestions.t	`78.94% <78.94%> (ø)`
lib/ProductOpener/Cache.pm	`94.11% <87.50%> (-5.89%)`	⬇️
tests/unit/tags.t	`88.97% <100.00%> (+0.22%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

stephanegigandet · 2023-03-22T19:04:23Z

@alexgarel the PR is ready for review again, I added some caching, did a bit of refactoring and added more tests

alexgarel · 2023-03-23T09:02:07Z

lib/ProductOpener/Tags.pm

@@ -1797,7 +1797,7 @@ Build all taxonomies
 =cut

 sub build_all_taxonomies ($publish) {
-	foreach my $taxonomy (@taxonomy_fields) {
+	foreach my $taxonomy (@taxonomy_fields, "test") {


alexgarel · 2023-03-23T09:13:20Z

lib/ProductOpener/TaxonomySuggestions.pm

@@ -97,17 +98,56 @@ Hash of fields that can be taken into account to generate relevant suggestions
 - categories: comma separated list of categories (tags ids or strings in the $search_lc language)
 - shape: packaging shape (tag id or string in the $search_lc language)

+=head3 Note
+
+The results of this function are cached using memcached. Restart memcached if you want fresh results


Shouldn't we also add a time to live to our cached entries ? 1 day seems enough.

So right now as we use it for mongodb we might have old entries served ?

Memcached has built-in support for that in set you can add an expiration time.

alexgarel

Really cool.

Two comments though:

I think it would be better to integrate the key prefix as a parameter to the cache key function (more explicit for future usage)
For refactor I did propose, I was seeing it at a higher level (but it's ok if you keep things as is)

alexgarel · 2023-03-23T09:22:22Z

lib/ProductOpener/Cache.pm

+	my $key = $server_domain . "/" . $json->encode($context_ref);
+	my $md5_key = md5_hex($key);
+	$log->debug("generate_cache_key", {context_ref => $context_ref, key => $key, md5_key => $md5_key})
+		if $log->is_debug();
+	return $md5_key;


I find it a bit dangerous. Collision may happens.
They are not maybe a big issue if we at least prefix the key by - a domain + a function.

So I would associate each domain with a single character, same for each function, and then encode the rest of $context_ref with md5.

I'm not sure for the domain prefix for there might be too much case to handler, don't we have a variable saying the "product-family" ?

SERVER_DOMAINS_PREFIX = {"openfoodfacts.org" => 'a', …} FN_PREFIX = {"suggestion_cache" => 'a', "mongo_query" => 'b'} sub generace_cache_key($fn, $context_ref) { $key = SERVER_DOMAINS_PREFIX->{$server_domain} . FN_PREFIX->{$fn}; ... $key .= $md5_key }

I see below that you did it indeed, by adding a prefix… but in this case, let's add the prefix as a parameter to this function, it will be more clear.

And ok then for the domain in the hash (the most important thing is to have a "result type" prefix.

ok, I made the key server domain + result type + hash of context, directly in the generate_cache_key function

alexgarel · 2023-03-23T09:29:56Z

lib/ProductOpener/Ecoscore.pm

+			my $target_material_id_exists_in_taxonomy;
+			my $target_material = canonicalize_taxonomy_tag(
+				"en", "packaging_materials",
+				$assignment_ref->{target_material},
+				\$target_material_id_exists_in_taxonomy
+			);
+
+			my $source_material_id_exists_in_taxonomy;
+			my $source_material = canonicalize_taxonomy_tag(
+				"en", "packaging_materials",
+				$assignment_ref->{source_material},
+				\$source_material_id_exists_in_taxonomy
+			);
+
+			if (not $target_material_id_exists_in_taxonomy) {
+				die(      "target_material "
+						. $assignment_ref->{target_material}
+						. " does not exist in the packaging_materials taxonomy");
+			}
+			if (not $source_material_id_exists_in_taxonomy) {
+				die(      "source_material "
+						. $assignment_ref->{source_material}
+						. " does not exist in the packaging_materials taxonomy");
+			}


Although the exists in taxonomy parameter make sense, I don't think you get my point with the refactoring. For me it was more that you are doing 4 times the same thing…

I would have reduced that to:

my $target_material = get_canonical_or_die($assignement_ref, "target_material", "packaging_material"); my $source_material = get_canonical_or_die($assignement_ref, "source_material", "packaging_material"); my $target_shape = get_canonical_or_die($assignement_ref, "target_shape", "packaging_shapes"); my $source_shape = get_canonical_or_die($assignement_ref, "source_shape", "packaging_shapes");

Sounds good, I added a function like that. I did the other because there are many other places in the code where we canonicalize a tag and then check its existence in the taxonomy (e.g. for all the parsing of ingredients lists etc.)

sonarcloud · 2023-03-23T12:17:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

alexgarel

Perfect !

stephanegigandet added 2 commits March 1, 2023 17:54

taxonomy: add packaging material code in canonical name

9cb5ab6

feat: match synonyms for taxonomy suggestions #8002

05c66e6

stephanegigandet requested a review from a team as a code owner March 13, 2023 14:13

github-actions bot assigned stephanegigandet Mar 13, 2023

github-actions bot added ✏️ Editing - Auto Suggest Providing autosuggest for taxonomized fields. Mostly used in editing scenarii 📦 Packaging https://wiki.openfoodfacts.org/Category:Recycling 🧬 Taxonomies https://wiki.openfoodfacts.org/Global_taxonomies 🧪 tests labels Mar 13, 2023

stephanegigandet commented Mar 13, 2023

View reviewed changes

stephanegigandet added 2 commits March 13, 2023 15:58

lint

268d0a2

update packaging taxonomy, changed canonical names

5d396cb

github-actions bot added the 🌱 Eco-Score https://world.openfoodfacts.org/eco-score-the-environmental-impact-of-food-products label Mar 13, 2023

stephanegigandet added 3 commits March 13, 2023 18:20

plastic codes in canonical name

0cbb94d

lint

74aa0cf

update tests

e966209

alexgarel approved these changes Mar 21, 2023

View reviewed changes

Update lib/ProductOpener/Ecoscore.pm

ce767c6

Co-authored-by: Alex Garel <alex@garel.org>

stephanegigandet added 3 commits March 22, 2023 19:49

refactor memcached caching

40bccaf

update tests

fad4f92

lint

188280e

github-actions bot added Display Orgs 🏭 Producers Platform https://wiki.openfoodfacts.org/Platform_for_producers Tags 🧪 unit tests 👥 Users labels Mar 22, 2023

merged main + lint

37b690f

alexgarel reviewed Mar 23, 2023

View reviewed changes

suggestions from code review

f49230e

alexgarel approved these changes Mar 23, 2023

View reviewed changes

alexgarel merged commit e1304de into main Mar 23, 2023

alexgarel deleted the packaging_materials_suggestions branch March 23, 2023 13:39

openfoodfacts-bot mentioned this pull request Mar 23, 2023

chore(main): release 2.9.0 #8231

Merged

stephanegigandet mentioned this pull request Mar 28, 2023

taxonomy: add packaging material code in canonical name #8154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Match synonyms and xx: entries when computing taxonomy suggestions #8190

feat: Match synonyms and xx: entries when computing taxonomy suggestions #8190

stephanegigandet commented Mar 13, 2023

stephanegigandet Mar 13, 2023

stephanegigandet Mar 13, 2023

stephanegigandet Mar 13, 2023

stephanegigandet Mar 13, 2023

alexgarel left a comment

alexgarel Mar 21, 2023

stephanegigandet Mar 22, 2023

alexgarel Mar 22, 2023

alexgarel Mar 22, 2023

stephanegigandet Mar 22, 2023

alexgarel Mar 21, 2023

stephanegigandet Mar 22, 2023

alexgarel Mar 21, 2023

stephanegigandet Mar 22, 2023

alexgarel Mar 22, 2023

stephanegigandet Mar 22, 2023

alexgarel Mar 21, 2023

stephanegigandet Mar 22, 2023

codecov-commenter commented Mar 21, 2023 •

edited

Loading

stephanegigandet commented Mar 22, 2023

alexgarel Mar 23, 2023

alexgarel Mar 23, 2023

alexgarel left a comment

alexgarel Mar 23, 2023

alexgarel Mar 23, 2023

stephanegigandet Mar 23, 2023

alexgarel Mar 23, 2023

stephanegigandet Mar 23, 2023

sonarcloud bot commented Mar 23, 2023

alexgarel left a comment

		@@ -1,4 +1,5 @@
		[
		"Mint-flavoured syrup with sugar diluted in water",

feat: Match synonyms and xx: entries when computing taxonomy suggestions #8190

feat: Match synonyms and xx: entries when computing taxonomy suggestions #8190

Conversation

stephanegigandet commented Mar 13, 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

alexgarel left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 21, 2023 • edited Loading

Codecov Report

stephanegigandet commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgarel left a 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

Choose a reason for hiding this comment

sonarcloud bot commented Mar 23, 2023

alexgarel left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 21, 2023 •

edited

Loading