{"action":"created","issue":{"url":"https://api.github.com/repos/mozilla/telemetry-batch-view/issues/97","repository_url":"https://api.github.com/repos/mozilla/telemetry-batch-view","labels_url":"https://api.github.com/repos/mozilla/telemetry-batch-view/issues/97/labels{/name}","comments_url":"https://api.github.com/repos/mozilla/telemetry-batch-view/issues/97/comments","events_url":"https://api.github.com/repos/mozilla/telemetry-batch-view/issues/97/events","html_url":"https://github.com/mozilla/telemetry-batch-view/pull/97","id":167073606,"number":97,"title":"Train with addon ids","user":{"login":"Dexterp37","id":883721,"avatar_url":"https://avatars.githubusercontent.com/u/883721?v=3","gravatar_id":"","url":"https://api.github.com/users/Dexterp37","html_url":"https://github.com/Dexterp37","followers_url":"https://api.github.com/users/Dexterp37/followers","following_url":"https://api.github.com/users/Dexterp37/following{/other_user}","gists_url":"https://api.github.com/users/Dexterp37/gists{/gist_id}","starred_url":"https://api.github.com/users/Dexterp37/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Dexterp37/subscriptions","organizations_url":"https://api.github.com/users/Dexterp37/orgs","repos_url":"https://api.github.com/users/Dexterp37/repos","events_url":"https://api.github.com/users/Dexterp37/events{/privacy}","received_events_url":"https://api.github.com/users/Dexterp37/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2016-07-22T15:40:08Z","updated_at":"2016-08-09T11:15:01Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mozilla/telemetry-batch-view/pulls/97","html_url":"https://github.com/mozilla/telemetry-batch-view/pull/97","diff_url":"https://github.com/mozilla/telemetry-batch-view/pull/97.diff","patch_url":"https://github.com/mozilla/telemetry-batch-view/pull/97.patch"},"body":"This changes the addon recommender to fetch the addon database from addons.mozilla.org. We use the dumped information to prune commercial, unreviewed, privated or, in general, unlisted addons before training the model.\r\n\r\nWe also use the dumped information to get the addon name from the reported default locale, instead of relying on data coming from the clients.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/mozilla/telemetry-batch-view/97)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/mozilla/telemetry-batch-view/issues/comments/238523432","html_url":"https://github.com/mozilla/telemetry-batch-view/pull/97#issuecomment-238523432","issue_url":"https://api.github.com/repos/mozilla/telemetry-batch-view/issues/97","id":238523432,"user":{"login":"Dexterp37","id":883721,"avatar_url":"https://avatars.githubusercontent.com/u/883721?v=3","gravatar_id":"","url":"https://api.github.com/users/Dexterp37","html_url":"https://github.com/Dexterp37","followers_url":"https://api.github.com/users/Dexterp37/followers","following_url":"https://api.github.com/users/Dexterp37/following{/other_user}","gists_url":"https://api.github.com/users/Dexterp37/gists{/gist_id}","starred_url":"https://api.github.com/users/Dexterp37/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Dexterp37/subscriptions","organizations_url":"https://api.github.com/users/Dexterp37/orgs","repos_url":"https://api.github.com/users/Dexterp37/repos","events_url":"https://api.github.com/users/Dexterp37/events{/privacy}","received_events_url":"https://api.github.com/users/Dexterp37/received_events","type":"User","site_admin":false},"created_at":"2016-08-09T11:15:01Z","updated_at":"2016-08-09T11:15:01Z","body":"Ah, sorry for the mess. That's the first time I'm using this tool, I was assuming all things were resolved unless \"commented\".\n\n---\n\nReview status: 1 of 3 files reviewed at latest revision, 28 unresolved discussions.\n\n---\n\n*[a discussion](https://reviewable.io:443/reviews/mozilla/telemetry-batch-view/97#-KNleXuQ4kJwQ2EOmtmF:-KOiW3pPFbdijE2jpp8B:b-896fix) (no related file):*\n<details><summary><i>Previously, vitillo (Roberto Agostino Vitillo) wrote…</i></summary>\n> Please merge commit 8fc41ff in commit fe9cb50.\n</details>\nDone.\n\n---\n\n*[src/main/scala/com/mozilla/telemetry/ml/AddonRecommender.scala, line 243 \\[r4\\]](https://reviewable.io:443/reviews/mozilla/telemetry-batch-view/97#-KNX15sR7y950YDGi8F8:-KOiWATIJm2CvJ5Yy_zz:b-49m5t7) ([raw file](https://github.com/mozilla/telemetry-batch-view/blob/7ffa7437c422f82a7e211eb424fd8116b791769c/src/main/scala/com/mozilla/telemetry/ml/AddonRecommender.scala#L243)):*\n<details><summary><i>Previously, vitillo (Roberto Agostino Vitillo) wrote…</i></summary>\n> How is `default_locale` defined?\n</details>\nIt's a string containing the name of the default locale for the addon, e.g. \"it_IT\", as there can be addons with no English language. So we pick the name/info for the default locale, as they must be available.\n\n---\n\n*[src/main/scala/com/mozilla/telemetry/ml/AddonRecommender.scala, line 262 \\[r5\\]](https://reviewable.io:443/reviews/mozilla/telemetry-batch-view/97#-KNX28ijDq0sQQiThJmD:-KOiWjLtM_qH-BBKjkFn:b-fc2be4) ([raw file](https://github.com/mozilla/telemetry-batch-view/blob/0eae45ce2bb324fe9b5e777bedc63559b6c49f3c/src/main/scala/com/mozilla/telemetry/ml/AddonRecommender.scala#L262)):*\n<details><summary><i>Previously, vitillo (Roberto Agostino Vitillo) wrote…</i></summary>\n> Could you please verify that Spark doesn't have a better way of doing this natively and if not add a comment?\n</details>\nI tried to use the HDFS API there, but I trip on the same permission issues that we encountered using `model.write.overwrite().save(s\"file://$outputDir/als.model\")`\n\n---\n\n*[src/main/scala/com/mozilla/telemetry/ml/AddonRecommender.scala, line 266 \\[r5\\]](https://reviewable.io:443/reviews/mozilla/telemetry-batch-view/97#-KNX2LkD-fASiG3oE5h4:-KOiXIa8hMJz5F_tFmmL:b48leqf) ([raw file](https://github.com/mozilla/telemetry-batch-view/blob/0eae45ce2bb324fe9b5e777bedc63559b6c49f3c/src/main/scala/com/mozilla/telemetry/ml/AddonRecommender.scala#L266)):*\n<details><summary><i>Previously, vitillo (Roberto Agostino Vitillo) wrote…</i></summary>\n> Please use logging. When do you expect this to fail?\n</details>\nThis is expected to fail if there was some issue writing the the model to file or copying the model from HDFS to the local machine. I've added a comment there.\n\n---\n\n*[src/main/scala/com/mozilla/telemetry/ml/AMODatabase.scala, line 76 \\[r10\\]](https://reviewable.io:443/reviews/mozilla/telemetry-batch-view/97#-KNlfhXrpQgfzAPga1Bz:-KOimIO0I0TG2aFJTLMP:b-l8d7c1) ([raw file](https://github.com/mozilla/telemetry-batch-view/blob/8fc41ff7c6488d92f070ac00e793c9b51c9addaa/src/main/scala/com/mozilla/telemetry/ml/AMODatabase.scala#L76)):*\n<details><summary><i>Previously, vitillo (Roberto Agostino Vitillo) wrote…</i></summary>\n> This looks a bit weird. Please the constructor to fetch a copy of the AMO database. It probably makes sense to set a default argument for `outputDir` or remove the parameter entirely. Furthermore, how do you plan to handle cache invalidation? You might want to add a comment that explains that the \"read-from-disk\" functionality is needed only for testing purposes and so cache invalidation isn't really an issue.\n</details>\nGood points. I've removed the parameter and added a comment about the invalidation.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/mozilla/telemetry-batch-view/97#-:-KOiUQwsWnUMGJ96CSkM:b-vhnktp)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"Dexterp37"}}