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.

Sign up for GitHub

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

Jump to bottom

Refacto datasources with a factory #288

Merged
merged 9 commits into from Dec 2, 2021
Merged

Refacto datasources with a factory #288

merged 9 commits into from Dec 2, 2021

Conversation

sdrll
Copy link
Contributor

@sdrll sdrll commented Nov 30, 2021 •
edited

@sdrll sdrll marked this pull request as draft November 30, 2021 14:27
@sdrll sdrll changed the base branch from master to trigger_condition_tripadvisor November 30, 2021 14:27
@sdrll sdrll mentioned this pull request Nov 30, 2021
Closed
@sdrll sdrll marked this pull request as ready for review November 30, 2021 17:32
@sdrll sdrll changed the base branch from trigger_condition_tripadvisor to es7 December 1, 2021 09:21
remi-dupre
remi-dupre reviewed Dec 2, 2021
View reviewed changes
idunn/api/places_list.py Outdated
Comment on lines 216 to 217
async def is_brand_detected_or_pj_category_cover(params):
return params.q or (params.category and all(c.pj_what() for c in params.category))
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't require to be async, which implies that select_datasource_* either

idunn/api/places_list.py
filters = [MimirPoiFilter.from_url_raw_filter(f) for f in params.raw_filter]

async def select_datasource_for_france(params):
if any(cat in params.category for cat in TRIPADVISOR_CATEGORIES_COVERED_IN_FRANCE):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue that this any should be all if we want to be sure to cover all categories with the selected datasource. I don't think it actually matters because for a regular use of Qwant Maps this category parameter should always be a list of exactly one element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it's always one element and I have one issue with all should reverse, I should reverse the expression : all(cat in TRIPADVISOR_CATEGORIES_COVERED_IN_FRANCE for cat in params.category): and add a specific behavior when there is no category detected. I think I will keep this any 😅

idunn/api/places_list.py Outdated
Comment on lines 249 to 250
logger.error("%s is not a valid source type, OSM source is used as fallback", source_type)
return Osm()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should crash the function as this could only happen if there is a critical issue in our implementation.

I would reserve this kind of behavior (error log + fallback) for cases where an external API returns an unexpected result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely

@sdrll sdrll merged commit 9c76c82 into es7 Dec 2, 2021
sdrll added a commit that referenced this pull request Dec 8, 2021
@sdrll
Refacto datasources with a factory (#288)
4417307 
* simply add tripadvisor trigger logic to suggest

* add tests for tripadvisor

* first version refacto datasources with factory pattern

* unravel circular depedencies : multiple utils module file were shared between multiples modules -> I migrate then to the utils module in a "half-dirty" way

* improve factory and clean `get_name` function that was in utils but only used once

* use classic factory instead to make test success

* divide `get_places_bbox_impl` into sub-functions

* create two functions to differentiate france and worldwide behavior for the datasource selection

* fix review

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>
sdrll added a commit that referenced this pull request Mar 16, 2022
@sdrll @amatissart @remi-dupre
Migrate mimir to ElasticSearch7.x (#277) (#279)
f560d55 
* Migrate mimir to ElasticSearch7.x (#277)

* fix lint

* keep httpx 1.18 in master

* PoC: compatibility with es7

* tests migration from ES2 to ES7 for mimir

* fix lint

* fix linting warning of pylint 2.11

* black reformat

* update getting type with ES7

* only maintained ES7 for mimir

* fix linting test

* fix wiki tests

* Build docker image for es7 branch with github action

* remove encoding warning from lintage + weird import position

* disable='unspecified-encoding' for the lintage

* disable='unspecified-encoding' for the lintage

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>
Co-authored-by: Adrien Matissart <a.matissart@qwantresearch.com>

* Hotel pricing Tripadvisor api endpoint (#281)

* add hotel pricing tripadvisor api call endpoint

* limit docker elastic memory size + upgrade to latest elasticsearch version

* fix review (add todo cleanup endpoint and clean useless fixture)

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* Add tripadvisor feeds endpoint with categories (#285)

* add tripadvisor feeds endpoint with categories

* fix fomat + add enum for type of POI called from mimir

* fix tests

* fix formatting

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* add logger url bragi

* QMAPS-2334 fix readonly fs gunicorn pid file (#286)

Co-authored-by: Aureliano Sinatra <a.sinatra@qwant.com>

* Refacto datasources with a factory (#288)

* simply add tripadvisor trigger logic to suggest

* add tests for tripadvisor

* first version refacto datasources with factory pattern

* unravel circular depedencies : multiple utils module file were shared between multiples modules -> I migrate then to the utils module in a "half-dirty" way

* improve factory and clean `get_name` function that was in utils but only used once

* use classic factory instead to make test success

* divide `get_places_bbox_impl` into sub-functions

* create two functions to differentiate france and worldwide behavior for the datasource selection

* fix review

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* fix linting

* Trigger instant answer with tripadvisor  (#290)

* add tests instant answer tripadvisor with intention detected

* create OSMPoi and TripadvisorPoi and rename index to poi-tripadvisor + tests

* fix previous behavior/tests. New feature test is still in red state

* fix new behavioral tests to fallback on pages if no hotel were fetched with tripadvisor for a single poi

* fix async task and trgger order between OSM and PagesJaunes

* reverse pagesjaunes and osm priority

* Update default argument with Optional type hint

Co-authored-by: Rémi Dupré <r.dupre@qwant.com>

* Make POI an abstract class

* add type source choice on Bragi init

* fix lint

* fix instantiate POI type depending on id

* Update idunn/places/poi.py

Co-authored-by: Rémi Dupré <r.dupre@qwant.com>

* convert poi-tripadvisor index to poi_tripadvisor + with review

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>
Co-authored-by: Rémi Dupré <r.dupre@qwant.com>

* Add MCID tripadvisor url (#292)

* add MCID tripadvisor url in PlaceMeta object with source_url and contribute_url

* fix format

* fix review

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* Bragi new params (#293)

* update bragi API model

* codes is optional with new Bragi

* geocoder/models/params: stops type is not used

* Handle POI detail for tripadvisor (#294)

* fix review

* fix consistent naming

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* Add bubble star url for tripadvisor (#296)

* add rating url for tripadvisor

* fix lint

* fix tests

* format

* add review block for tripadvisor

* fix format

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* Fix wrong merge (#299)

* add rating url for tripadvisor

* fix lint

* fix tests

* format

* add review block for tripadvisor

* fix format

* fix instant answer for the bragi version (#298)

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* update dependencies (#302)

* update dependencies

* disable failing pylint

* add heathcheck for all external sources (#300)

* add heathcheck for all external sources

* use dict to display heathcheck

* fix lint

* fix tests

* add bragi healthcheck

* fix lint

* add tagger/classifier healthcheck and clean bragi healthcheck

* add redis heathcheck

* fix format

* remove print

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* fix wrong import

* add timeout on status call

* build tripadvisor url with lang (#303)

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* Catch exception on healthcheck timeout (#304)

* catch exception on healthcheck

* fix format

* log error

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* Qmaps 2453 fix TA module priority (#305)

* dirty code to test now module tripadvisor priority

* fix tests

* add comment

* fix lint

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* fix property address and category in poi list (#306)

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* fix None type exception

* Add TRIPADVISOR_ENABLED flag (#307)

* add TRIPADVISOR_ENABLED flag

* fix call hotel pj when tripadvisor is disabled

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>

* wrap tripadvisor's ratings image in thumbr (#308)

* wrapping tripadvisor's ratings image in thumbr

* tripadvisor: fix rating url for full values

* add small comment

* image block: resize images images to fit what is displayed in erdapfel (#309)

* image block: resize images images to fit what is displayed in erdapfel

Some images can get pretty big, especially when displaying several of
them in the list view.

If we want more granularity it could make sense to specify that as a
parameter, but I don't think this would make any improvement in our
current use.

* image block: fix broken tests due to Thumbr url change

* instant_answer: mix OSM and tripadvisor in direct query (#311)

* instant_answer: mix OSM and tripadvisor in direct query

As weights for TA POIs are rather high, they are already prioritized
compared to OSM POIs and this won't be too agressive by hiding potential
much more relevent OSM documents.

* instant_answer: we do not need to filter tripadvisor results

* instant_answer: fix exception for unsupported nlu langs (#312)

* urlsolver: do not raise an exception for 4xx/5xx and fix errors for 3xx (#313)

httpx seems to have changed its behavior, and it doesn't really make
sense to raise an exception in Idunn when provided with an invalid URL
anyway.

* fix import get_mimir_elasticsearch

* fix lint

* fix lint

Co-authored-by: sdirollo <sebastien.dirollo@gmail.com>
Co-authored-by: Adrien Matissart <a.matissart@qwantresearch.com>
Co-authored-by: aureliano sinatra <sinaure@gmail.com>
Co-authored-by: Aureliano Sinatra <a.sinatra@qwant.com>
Co-authored-by: Rémi Dupré <r.dupre@qwant.com>
Co-authored-by: Rémi Dupré <remi@dupre.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remi-dupre remi-dupre remi-dupre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sdrll @remi-dupre