Skip to content
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

GPS: Amélioration de l'autocomplete #5363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tonial
Copy link
Contributor

@tonial tonial commented Jan 9, 2025

🤔 Pourquoi ?

Les performances sont clairement inférieures, mais au moins le résultat est exploitable.
La version actuelle ne sera à rien sur des noms ou prénom trop communs (trop de résultats possibles pour être sur que la personne que l'on cherche apparaisse dans les 10 premiers)

On ajoute le titre si on le connaît, ainsi que la date de naissance pour mieux gérer les homonymes.

Niveau performances : ~ 1.25 secondes par requete (c'est donc super long)

Une alternative est présente ici :#5365 en utilisant la recherche plein texte

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@tonial tonial added the modifié label Jan 9, 2025
@tonial tonial requested a review from francoisfreitag January 9, 2025 14:11
@tonial tonial self-assigned this Jan 9, 2025
@tonial tonial added the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Jan 9, 2025
@tonial tonial force-pushed the alaurent/gps_autocomplete branch from e689e49 to 49f0de1 Compare January 9, 2025 14:15
@tonial tonial added 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC and removed 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC labels Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

users = [
{
"text": user.get_full_name(),
"text": f"{user.title}. {user.get_full_name()} ({user.jobseeker_profile.birthdate})",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On révèle maintenant encore plus d’informations avec ce endpoint, je ne sais pas si c’est OK point de vue juridique et sécu ?

Copy link
Contributor Author

@tonial tonial Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ce sont des infos auxquelles on a accès une fois la personne ajoutée, donc pour moi cela ne change pas (le endpoint a les même restrictions d'usage que la vu de détail)

EDIT: j'ajoute le titre qui n'est en fait pas dans la page suivante, mais on y trouve le NIR dont le premier chiffre donne la même info en général :)

@francoisfreitag
Copy link
Contributor

Je ne suis pas trop fan des perfs, ça impacte le service des emplois en général. Serait-il possible pour le métier de trouver quelques critères plus discriminants ? Genre département, civilité 🤷

@tonial
Copy link
Contributor Author

tonial commented Jan 9, 2025

Hum... c'est vrai que c'est un problème...
Peut être passer sur un formulaire avec une liste, et donc pas un autocomplete ?
un peut comme la recherche de l'admin

Comment on lines 165 to 169
Exists(
User.objects.filter(pk=OuterRef("pk")).filter(
Q(first_name__unaccent__icontains=term) | Q(last_name__unaccent__icontains=term)
),
),
Copy link
Contributor

@xavfernandez xavfernandez Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça me semble bien violent avec autant de subqueries qu'il y a de termes 😱
Je pense qu'on doit pouvoir faire sans subqueries...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si tu as une idée, je suis preneur !
Le truc c'est qu'afin de pouvoir mettre un score qui dépend du nombre de correspondances trouvées, ce sera forcément une subquery par terme, non ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

         def match_term(term):
             return Cast(
-                Exists(
-                    User.objects.filter(pk=OuterRef("pk")).filter(
-                        Q(first_name__unaccent__icontains=term) | Q(last_name__unaccent__icontains=term)
-                    ),
-                ),
+                Q(first_name__unaccent__icontains=term) | Q(last_name__unaccent__icontains=term),
                 IntegerField(),
             )

semble aussi bien marcher 😌

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Après ça reste violent vu que postgres est obligé de regarder et trier tous les comptes candidats...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donc je pense que tu devrais également garder le filter(or_queries(name_q)) histoire que postgresql n'aie pas besoin systématiquement de traiter 1M+ de comptes à chaque requête 😬

Copy link
Contributor Author

@tonial tonial Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est en effet beaucoup mieux, on passe de 3 à 2 secondes dans un cas de test, mais c'est pas suffisant pour le laisse en autocomplete dans cet état.

Le or_queries n'améliore pas grand chose. Je pense que l'élément coûteux c'est de rechercher chaque terme dans prénom et nom. Donc le faire pour filter et compter, ou uniquement pour compter c'est kif-kif.

Par contre, en forçant la présence du premier terme, ça limite pas mal l'impact en performances quand il y a plusieurs termes.

Copy link
Contributor Author

@tonial tonial Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Et sinon, ts_vector de psql accepte un dictionnaire "simple" qui évite le stemming 🤩

Ce qui permet donc de laisser psql faire un truc plus optimisé niveau performances que ma première piste un peu à la mano

(voir la seconde PR )

@tonial tonial force-pushed the alaurent/gps_autocomplete branch 4 times, most recently from b8b0713 to 4a76c83 Compare January 10, 2025 10:25
@tonial tonial changed the title GPS: Amélioration de l'autocomple GPS: Amélioration de l'autocomplete Jan 10, 2025
@tonial tonial force-pushed the alaurent/gps_autocomplete branch from 4a76c83 to e723ed3 Compare January 10, 2025 20:46
@tonial tonial removed the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Jan 10, 2025
Match every term against the full name, and order the results
using the number of matched term.

Using a manual match that allow to search with only part of the name
(while full text search will only match full "words")
@tonial tonial force-pushed the alaurent/gps_autocomplete branch from e723ed3 to 13658c1 Compare January 10, 2025 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants