mozilla / taar-lite

A lightweight version of the TAAR service intended for specific deployments with reduced feature visibility.
Mozilla Public License 2.0
2 stars 6 forks source link

Discussed code re-organization #41

Closed birdsarah closed 6 years ago

birdsarah commented 6 years ago

Fixes #38, #37

Marking as fixes #37, because guidception has been moved to experimental.

birdsarah commented 6 years ago

Tests are failing because dockerflow is not a requriement of srgutil, which is where the logging comes from. I believe that's where the dependency should live. Thoughts @crankycoder?

This has been addressed and updates pushed to srgutil.

birdsarah commented 6 years ago

@crankycoder, pending the test failure, which I will fix, this is ready I think.

There's a couple of API niggles (e.g. I'm thinking about tie breaking as a step like treatments). @dzeber is working on a follow-on PR to:

  1. clean-up treatment code
  2. add metrics to recommender class

@dzeber, you'll see i've added an analysis directory and some instructions on README. Contrary to what I said earlier you can start Jupyter from wherever.

cc @mlopatka

birdsarah commented 6 years ago

@crankycoder I would probably do a squash merge due to so many incremental (and failing) commits along the way here. (But I generally prefer squash merge, so up to you)

crankycoder commented 6 years ago

Sorry for the long delay in completing the review. It wasn't clear to me that adding the treatment graph code modified the range of addon-ids that taar-lite would respond to.

For other people's sake - I used the following URL when running taar-api-lite to test end to end functionality.

http://localhost:8000/taarlite/api/v1/addon_recommendations/html5-video-everywhere@lejenome.me/

The previous GUID I happened to be using "{a0f88751-df67-4902-8da4-543cc4ce3d48}" wasn't in the treatment graph so I was surprised to see an empty list of results come back.

crankycoder commented 6 years ago

It's friday. I'm just going to merge this in.