Page MenuHomePhabricator

Port recommendation api to node
Closed, ResolvedPublic5 Story Points

Description

The current synchronous stack isn't performant and doesn't make sense for what this service provides. Moving to Node using https://github.com/wikimedia/service-template-node should remedy these issues.

Spawned from: T159544#3228543

Details

Related Gerrit Patches:
mediawiki/services/recommendation-api/deploy : masterInitial import of recommendation-api

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 2 2017, 5:29 PM
schana updated the task description. (Show Details)May 3 2017, 12:40 PM
schana moved this task from Backlog to In Progress on the Recommendation-API board.

The port is complete at this point with at least the following limitations:

  1. There is no option to include pageviews in the result
  2. Morelike is always used as the search method if a seed is provided
  3. The results are ranked by sitelink count
  4. There is not an option to specify the number of results
  5. There is not logic for a multi-entry seed with | delimiter

@mobrovac There is documentation for deploying to production, but is it possible to stand this up in labs? Also, if you have time, would you mind giving a look through the changes to ensure I'm on the right path? https://github.com/schana/recommendation-api/compare/e7f6037fe3c3d99b87b81d28103a838bb10cc345...master

Really nice work @schana! Very well done! In general everything is looking good, I just have some minor comments that I will lay down here.

lib/api-util.js

  • wdqsApiGet() should use a request template just like mwApiGet() and restApiGet() do, and it should set the default values in case the template is not present in the config. Additionally, for easier maintenance, it would be better for that function to be in a file created by your service (so perhaps lib/translation.js). We probably could consider integrating wqdsApiGet() into the service template at a later point as it does seem like a useful feature.

lib/translation.js

  • Similarly to the point above, setupTemplates() should (a) check that app.conf.queries exists (if it doesn't set it up as {}); and (b) check that .seed, .pageviews and .article exist (and if they don't, provide defaults).
  • In getArticles() there is no need for the catch() block on line 47 since mwApiGet() will error out by itself in case of an error.
  • In getArticlesBySeed()both .then() blocks (L76 and L85) declare a variable named result so the latter hinders the former. Regardless of whether that was intentional or not, the code is more readable if they are named differently.
  • In recommend(), L154, better to check for the truthfulness of seed instead of explicitly checking for undefined, since that covers the cases 0, '', false, etc. as well. Simply use if (seed) instead to avoid unwanted situations.

A general comment is that it is customary in WMF services to use the same indentation level for function continuations:

return promisedFunction(x)
.then((result) => {
    return Object.keys(result)
    .map((item) => {
        // do whatever here with the item
    });
});

Finally, it would be much easier and more productive if you could be creating PRs so as to make the review process easier and collaborative ;)

For deploying the service, you can follow the guide on Wikitech. TL;DR is that you need two repos in gerrit, one for the source repo (a clone of your github repo) and a deploy repo. You also need to submit a ticket that presents an intent of first deployment.

For setting it up in BetaCluster, concretely, it is really easy to do it once the steps outlined in the guide have been completed. If you need help with any of the steps, let me know.

@mobrovac Pull request for the changes you recommended above is here: https://github.com/schana/recommendation-api/pull/1

After merge, I'll begin the process to get it moved to Gerrit and follow the guide for deploying that you linked to.

Change 354451 had a related patch set uploaded (by Nschaaf; owner: Nschaaf):
[mediawiki/services/recommendation-api/deploy@master] Initial import of recommendation-api

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

Change 354451 merged by Mobrovac:
[mediawiki/services/recommendation-api/deploy@master] Initial import of recommendation-api

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

schana closed this task as Resolved.Jun 1 2017, 5:30 PM
schana removed a project: Patch-For-Review.
schana moved this task from For Review to Done on the Recommendation-API board.

The service has been ported to node and is hosted in gerrit.