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

Recherche via Elasticsearch #4096

Merged
merged 110 commits into from Feb 5, 2017
Merged

Recherche via Elasticsearch #4096

merged 110 commits into from Feb 5, 2017

Conversation

@pierre-24
Copy link
Member

@pierre-24 pierre-24 commented Dec 24, 2016

Q R
Type de modification évolution
Ticket(s) (issue(s)) concerné(s) #4133 #3833 #3748 #3725 #3678 #3625 #3549 #3386 #3393 #3310 #2769 #2857 #1755

Joyeux noël, zeste de savoir ! Ça fait 2 mois que je me suis proposé, mais cette fois-ci, c'est officiel, je m'y attaque. But du jeu : virer Solr et utiliser elasticsearch !

Pourquoi ?

  • Possibilité de booster des champs ou des types de document ;
  • Possibilité d'utiliser des analyzer et des tokenizer particulièrement puissants ;
  • Possibilité d'indexer facilement des classes qui ne sont pas de Django
  • (...)

Comment ?

  • En virant solr et haystack, et en se basant directement sur les API's "bas niveau" et "plus haut niveau" directement écrites en python et maintenues par l'équipe d'elastic itself (ce qui évite d'avoir à attendre la mise à jour, puisque l'API est mise à niveau en même temps que le produit) ;
  • Avec une classe abstraite et des méthodes à surclasser
  • En tapant les différents types dans le même index d'elastisearch (les différents modèles étant différenciés par leur type) afin de pouvoir facilement faire des recherches sur tout les modèles en même temps (même si elastic permet de la recherche multiindex).
  • En implémentant un mécanisme pour ne réindexer que ce qui est vraiment nécessaire (donc juste les données qui ont changé).
  • En tentant d'éviter un maximum les allez-retour entre BDD "traditionnelle" et elastic, au niveau de l'indexation et au niveau de la recherche elle-même (dans l'idéal, la recherche ne devrait faire aucun appel à la BDD, juste à elastic, quitte à stocker des infos supplémentaires dans elastic avec index="not_analyzed").
  • En indexant enfin de manière correcte les contenus, sans devoir repasser par la BDD.

Je tiens à créditer le package django-el pour certaines des idées et manières de faire.

TODO

Pour le moment, j'ai fait comme avec la ZEP-12, je travaille sur zds/searchv2/ pour éviter les rebases trop chiants (même si il y en aura).

  • les mixins
  • le mécanisme de flagging
  • les commandes d'indexations
  • Insérer les différents types
  • le modèle et la vue de recherche (en combinant les dizaines de champs à considérer)
  • boosting
  • highlighting
  • utiliser le logger au lieu de print()
  • Gérer le cas des suppressions (en particulier des PublishedContent, qui doivent être modifié convenablement quand ils sont mis à jour)
  • définitivement virer solr et haystack (donc mapper la recherche sur le nouveau module)
  • la documentation (utilisateur et code!)
  • un minimum de tests unitaires (dans la mesure du possible)
  • pip freeze (et d'ici là, ce sera probablement plus la 5.1.1).
  • déjà virer le dossier zds/search/ ?
  • Le update.md
  • Attendre la ZEP-13.

À discuter → #4098

  • Qu'est ce qu'on indexe comme modèle ?
  • Qu'est ce qu'on indexe comme champ pour chacun de ces modèles ?
  • Qu'est ce qu'on booste comment ?
  • Qu'est ce qu'on utilise comme tokenizer et analyzer ?
  • Est ce qu'on en profite pour modifier le front de la recherche ?
  • Est ce que si tout fonctionne, on peut récupérer la barre en page d’accueil (quitte à tuner le comportement pour favoriser la recherche par mots-clés).

Sujet sur le forum à venir, probablement demain, sauf si @vhf préfère pas.

QA

C'est à peu près OK, mais il faudra tester sur le "vrai zds" pour ajuster les différents paramètres de boost.

  • Téléchargez et installez Elasticsearch grâce à la procédure décrite dans la documentation.
  • Mettez à jour vos requirements et vos migrations
  • Indexez à l'aide de python ./manage.py es_manager index-all. Une fois que c'est fait, vous pouvez vous amuser à créer des topics, posts ou publier des contenus et utiliser python ./manage.py es_manager index-flagged pour mettre à jour l'index.
  • Vous pouvez faire une recherche via la page d’accueil ou l'en-tête du site.
  • Vous pouvez ensuite jouer avec les différents paramètres de boost pour voir comment ça influe sur la recherche :)

Un peu de screenshots:

screenshot from 2017-01-02 14 54 28

screenshot from 2017-01-02 14 49 35

@@ -2,6 +2,8 @@
pysolr==3.4.0
pygments==2.1.3
python-social-auth==0.2.19
elasticsearch>=2.0.0,<3.0.0

This comment has been minimized.

@pierre-24

pierre-24 Dec 24, 2016
Author Member

Bon, ça, c'est moche, faut que je me fixe une version et que je m'y tienne.

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

Tu es en dev, tu mettras la dernière version en date lorsque le WIP sera terminée. (pip freeze tout ça)

@Anto59290
Copy link
Contributor

@Anto59290 Anto59290 commented Dec 24, 2016

Salut Pierre,

C'est surement pas la priorité mais tu peux noter ça dans quel modèle indexer : #2807.

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Dec 24, 2016

C'est noté ;)

@@ -0,0 +1,34 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

tu sais que je t'aime toi.

This comment has been minimized.

@pierre-24

pierre-24 Dec 25, 2016
Author Member

<3

self.es_already_indexed = True


class ESDjangoIndexableMixin(ESIndexableMixin, models.Model):

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

Je ne suis pas trop pour dire que c'est un modèle puisque sa sémantique c'est "Mixin".

Maintenant ma relecture ne fait que commencer, peut-être me fourvoie-je.

This comment has been minimized.

@pierre-24

pierre-24 Dec 24, 2016
Author Member

Je sais plus trop si j'ai vraiment besoin de mettre le model.Model (parce que je surclasse save(), par exemple) ou si c'est juste une facilité. Je crois que j'ai changé d'avis à un moment mais que j'ai pas tout modifié. Je testerai :)

This comment has been minimized.

@pierre-24

pierre-24 Dec 25, 2016
Author Member

Ouais, je confirme, si je le met pas, j'ai des ennuis. Du coup, soit je change le mot Mixin pour autre chose, soit je trouve une autre manière de dériver save()

This comment has been minimized.

@artragis

artragis Dec 25, 2016
Member

Bah plutôt que Mixin je pense que "Abstract" est une bonne chose. D'autant que tu utilises l'attribut de django "abstract = True".

self.setup_es()
elif options['action'] == 'index-all':
self.index_documents(force_reindexing=True)
elif options['action'] == 'index-flagged':

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

Du coup j'ai pas compris cette notion de "flagged". Peux-tu l'expliquer?

This comment has been minimized.

@pierre-24

pierre-24 Dec 25, 2016
Author Member

Ben en gros, ça évite de réindexer ce qui est déjà indexé et qui à priori n'a pas changé. Lorsqu'on récupère les objets à indexer, on ne récupère que ceux qui sont à es_flagged=True. C'est le must_reindex de solr, que d'ailleurs j'aurais probablement du appeller pour ça, mais je me suis fourvoyé avec l'option coté solr, qui s'appelle --only-flagged.

This comment has been minimized.

@artragis

artragis Dec 27, 2016
Member

Merci 👍

content_type = cls.__name__.lower()

# fetch parents
for base in cls.__bases__:

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

serait-il possible d'ajouter un logger.debug("will iter into %s", cls.__bases__) avant la boucle et un logger.debug("content type is %", content_type") lorsque la boucle est matchée.

de plus en mode "micro optimisation mais au moins on comprend explicitement la volonté du développeur", peux-tu ajouter un "break" s'il te plait?

This comment has been minimized.

@pierre-24

pierre-24 Dec 25, 2016
Author Member

Oui ;)

:rtype: elasticsearch_dsl.Mapping
"""

m = Mapping(self.get_es_content_type())

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

Sûrement dû au fait que c'est le début du développement. Mais ce n'est jamais une bonne chose de garder des variable monolettrales. (c'est noël, offrons un nouveau mot à notre langue :p)

data = {}

for field in fields:
if exclude_field and field in exclude_field:

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

afin de faire en sorte que cette condition fonctionne aussi dans le cas où exclude_field vaut None, je propose:

excluded_fields = excluded_fields or []  # please note the renaming into excluded_field, let's offer vhf a christmas english code
for field in fields:
    if field in excluded_fields:
        continue
    # next

This comment has been minimized.

@pierre-24

pierre-24 Dec 25, 2016
Author Member

ah, et très objectivement, je le comprend comme exclude a field et pas comme a field is exluded. Mais chui pas bien dans ma tête :p

This comment has been minimized.

@vhf

vhf Dec 25, 2016
Contributor

"Exclude a field" est une action, ça pourrait être une fonction ou méthode. Ici la variable contient "an excluded field". ;)

for field in fields:
if exclude_field and field in exclude_field:
continue
v = getattr(self, field, None)

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

Même si le "v" est une sémantique habituelle de "value", je pense que renommer en "value" ne coûtera pas trop cher et rendra le code pérenne.

obj = objs[index]
action = 'update' if obj.es_already_indexed and not force_reindexing else 'index'
objs[index].es_done_indexing(es_id=hit[action]['_id'])
print(hit)

This comment has been minimized.

@artragis

artragis Dec 24, 2016
Member

oh? un print. Il faudra le remplacer par un log.info/log.debug.

This comment has been minimized.

@pierre-24

pierre-24 Dec 24, 2016
Author Member

Oui :)

@Anto59290
Copy link
Contributor

@Anto59290 Anto59290 commented Dec 24, 2016

Petite question : comment vas-tu gérer les topics/forums privés dans la recherche. Ne pas les indexer du tout ? Ou alors les indexer et choisir de les afficher en fonction de l'utilisateur (staff ou non) ? J'imagine que ça va peut être t'obliger à indexer des champs supplémentaires ?

@SpaceFox
Copy link
Member

@SpaceFox SpaceFox commented Dec 24, 2016

@artragis
Copy link
Member

@artragis artragis commented Dec 24, 2016

@artragis
Copy link
Member

@artragis artragis commented Dec 24, 2016

attention ceci est une réflexion à chaud d'une perssonne qui n'a pas l'expérience ou l'expertise nécessaire dans elasticsearch.

Pour ce qui est des tokenizers : il me semble que @cgabard a rendu le moteur de markdown capable de gérer un AST ou du moins de générer des méta info bien sympa. On pourrait donc wrapper ce système dans un adaptateur qui permettrait de générer un tokenizing intéressant qui boosterait les mot en fonctions de :

  • est-ce dans le titre principal (s'applique à : contenu, partie, chapitre, extrait, topic)
  • est-ce dans un titre textuel (élément qui commence par # ou ## => plus il y a de # plus la priorité est faible)
  • est-ce un paragraphe/le titre d'un tableau/d'une figure
@vhf
Copy link
Contributor

@vhf vhf commented Dec 24, 2016

Faisable, mais je propose qu'on s'y penche dans un 2e temps. 👍

@artragis
Copy link
Member

@artragis artragis commented Dec 24, 2016

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Dec 25, 2016

Petite question : comment vas-tu gérer les topics/forums privés dans la recherche. Ne pas les indexer du tout ? Ou alors les indexer et choisir de les afficher en fonction de l'utilisateur (staff ou non) ? J'imagine que ça va peut être t'obliger à indexer des champs supplémentaires ?

Je vais dans le sens des réponses de @SpaceFox et @artragis , tout dois être indexé. Mais comme je dis, faut que je lance un topic avec une réflexion globale sur le sujet :)

@vhf
Copy link
Contributor

@vhf vhf commented Dec 25, 2016

A choisir, je préfère qu'on ouvre une issue pour discuter de ça plutôt qu'un topic, ça permet d'avoir un lien entre la PR, le repo et la discussion justifiant les choix qu'on fera. :)

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Dec 25, 2016

M'en doutais que t'allais dire ça :p

Je le fais, parce que j'ai envie d’essayer, mais je trouve que la question "qu'est ce que vous voulez comme résultat d'une recherche" (sans les détails techniques) devrait également être posée sur le forum. @FanJiyong , je te laisse décider de ça :-)

GO #4098 !!

@@ -100,6 +102,40 @@ def get_es_document_source(self, excluded_fields=None):

return data

def get_es_document_as_bulk_action(self, index, action='index'):
"""Create a document as formatted in a ``_bulk`` operation. Formatting is done based on action.

This comment has been minimized.

@artragis

artragis Dec 27, 2016
Member

very good idea

This comment has been minimized.

@pierre-24

pierre-24 Dec 27, 2016
Author Member

Je fais ce que je peux pour pas tuer ES ;)

if self.es_id != '':
document['_id'] = self.es_id
document['_source'] = self.get_es_document_source()
if action == 'update':

This comment has been minimized.

@artragis

artragis Dec 27, 2016
Member

elif est ici plus logique

This comment has been minimized.

@pierre-24

pierre-24 Dec 27, 2016
Author Member

Yup :)

return data


class FakeChapter(AbstractESIndexable):

This comment has been minimized.

@artragis

artragis Dec 27, 2016
Member

Juste : pourquoi Fake?

This comment has been minimized.

@pierre-24

pierre-24 Dec 27, 2016
Author Member

Bonne question. Parce que c'est pas le vrai conteneur mais une version batarde ?

This comment has been minimized.

@artragis

artragis Dec 27, 2016
Member

En re re relisant, j'ai compris: c'est une sorte de chapitre fantôme que tu utilisais pour toujours avoir un chapitre même quand on a un article.

def add_arguments(self, parser):
parser.add_argument('action', type=str)
parser.add_argument(
'action', type=str, help='action to perform, either "setup", "clear", "index-all" or "index-flagged"')

This comment has been minimized.

@artragis

artragis Dec 28, 2016
Member

Sais-tu que l'argument "choices" existe et te permettra de définir les options disponibles?

This comment has been minimized.

@pierre-24

pierre-24 Dec 28, 2016
Author Member

Non. Et pourtant, dieu sais que j'utilise argparse :-°

@artragis
Copy link
Member

@artragis artragis commented Dec 28, 2016

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 1, 2017

Ou bien celui là.

@vhf
Copy link
Contributor

@vhf vhf commented Feb 4, 2017

@pierre-24 peux-tu stp me résumer la situation ? Dans l'idéal, j'aimerais que ce soit mergé mercredi au plus tard, si possible avant mercredi.

En l'état j'arrive pas à déterminer si c'est mergeable ou pas (problème de migrations/travis mis à part).

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 4, 2017

@vhf
Copy link
Contributor

@vhf vhf commented Feb 4, 2017

Super, merci @pierre-24 . Du coup il manque plus que les tests passent sur cette PR et quand c'est vert, on mergera.

@DevHugo
Copy link
Contributor

@DevHugo DevHugo commented Feb 4, 2017

C'est un souci de migration:

CommandError: Conflicting migrations detected (0016_auto_20170108_1148, 0016_auto_20170201_1940 in tutorialv2; 0011_auto_20170130_1823, 0011_auto_20161224_1310 in forum).
To fix them run 'python manage.py makemigrations --merge'

@gcodeur gcodeur mentioned this pull request Feb 4, 2017
@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 4, 2017

@pierre-24 pierre-24 force-pushed the pierre-24:add_elasticsearch branch from e9ec98f to c03dca7 Feb 4, 2017
@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 4, 2017

Voilà, j'ai réglé les souchis de migration, j'ai intégré dev et j'ai mis à jour sur les dernières versions des librairies.Tout est bon, normalement :)

@coveralls
Copy link

@coveralls coveralls commented Feb 4, 2017

Coverage Status

Coverage increased (+1.4%) to 88.363% when pulling c03dca7 on pierre-24:add_elasticsearch into f4d041b on zestedesavoir:dev.

@DevHugo
Copy link
Contributor

@DevHugo DevHugo commented Feb 5, 2017

@vhf Pour moi, on est bon ! J'ai plus aucune remarque. Je te laisse appuyer sur le bouton final ?

@vhf
Copy link
Contributor

@vhf vhf commented Feb 5, 2017

Super, merci !

@vhf vhf merged commit 807fa2d into zestedesavoir:dev Feb 5, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 5, 2017

@vhf
Copy link
Contributor

@vhf vhf commented Feb 5, 2017

Yep, une affaire rondement menée, bravo à tous !

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 5, 2017

@Situphen
Copy link
Contributor

@Situphen Situphen commented Feb 5, 2017

@pierre-24 dans un peu plus d'un mois alors xD y'aura juste un petit problème de distance :-°

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 5, 2017

@Situphen
Copy link
Contributor

@Situphen Situphen commented Feb 5, 2017

Nan mais oh ! Je suis breton moi ! :P

@pierre-24
Copy link
Member Author

@pierre-24 pierre-24 commented Feb 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.