On Thu, Aug 30, 2012 at 09:25:21PM -0000, Gary Lasker wrote:
> > On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> > > Gary Lasker has proposed merging lp:~gary-lasker/software-center
> > /recommended-installed-feedback into lp:software-center with lp:~gary-lasker
> > /software-center/update-to-latest-recommender-client as a prerequisite.
> >
> > Thanks for working on this branch. I'm very happy to see this feature
> > coming.
> >
> > [..]
> > > Note that this branch depends on the lp:~gary-lasker/software-center/update-
> > to-latest-recommender-client branch as that branch adds the new recommender
> > client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-
> > recommender-client. Also note that I made one small change to sreclient to fix
> > the validate decorator for the pkgname argument to implicit_feedback so that
> > it matches the validator for the other calls that use pkgname as an argument.
> >
> > Thanks for this, if you fixed the sreclient_pristine.py, please also
> > send the diff to the server team (if you haven't done already) so that
> > they add it to their
> > lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
> > branch.
>
> Yep, I proposed a branch for this and it has been merged by Lukasz.
Great, thanks for this.
> > > Finally, I added a unit test for the new call in test_recagent.py, however
> > like a few of the other tests there, the test is not currently working with
> > the server. Therefore, it remains disabled as the other tests are. I'll look
> > at this tomorrow to try to see what the issue is with these tests.
> > [..]
> >
> > Maybe the staging server had problems. I just tested enabling them and
> > its working, but that may well be that its now fixed. While looking at
> > the test code I noticed that there is a bit of duplication in
> > there, I pushed a branch at
> > lp:~mvo/software-center/recagent-test-cleanup that removes most of
> > that.
> >
> > I also noticed that the branch is missing a test for the new
> > functionality, it would be nice to add one to
> > tests/gtk3/test_recommendations_widget.py. Something like activate a
> > app, trigger a install transaction and verify that on finish the
> > self.recommender_agent.post_implicit_feedback() call was done.
> >
>
> I added a thorough test for this functionality in test_catview.py (this is where the other recommendations panel tests are, and imo it's consistent because the tests for the other lobby panels (What's New and Top Rated) are also located here. While in test_catview.py I did some cleanup and consolidation of duplicated code as well.
Thanks for adding the test and for the cleanup there, that looks very
good!
[..]
> > Should self.recommended_apps_viewed we cleared when we get new data,
> > e.g on _on_app_recommendations_agent_refresh (and the equivalent
> > function for the categories). Or should this be persistent? I'm don't
> > know the details of the spec well enough :) I guess it comes down to
> > if we should consider it a recommendation that are reached
> > "indirectly", e.g. click on it "app A", move back to main view search
> > for something else and then reach "app A" again later (without
> > following a recommendation) if installing that should be counted or
> > not. I don't really mind either way, but would like to know.
> >
>
> Ok, so what happens is that when a person clicks on a recommended item, we remember that they did that for that item for as long as the current USC session is active (it is now, however, persisted between sessions, I think would be a mistake). I believe the way it works currently is optimal because I think that a person clicking on a recommended item probably became aware of that item *because* of the recommendation, and so we still want to report if that item is installed even if the install did not occur immediately after clicking through the recommendation. In other words, if the user looks at a recommended item, then navigates around a bit before revisiting (and installing) that recommended item, I think that should be considered a successful recommendation.
>
> That's why it's designed as it is. If we decide this isn't exactly what we want, we can change it easily.
Yeah, that is ok, I just wanted to raise it as a point of
consideration.
> > > + def _on_transaction_started(self, backend, pkgname, appname, trans_id,
> > > + trans_type):
> > > + if (trans_type != TransactionTypes.INSTALL and
> > > + pkgname in self.recommended_apps_viewed):
> > > + # if the transaction is not an installation we don't want to
> > > + # track it as a recommended item
> > > + self.recommended_apps_viewed.remove(pkgname)
> >
> > Could the check for the transaction type simply be folded into the
> > _on_transaction_finished() ? So that there is only a single signal
> > type to watch?
>
> I wanted to do that! Definitely it would be preferable. However, I could not because (strangely) the transaction-finished signal does *not* include the transaction type. So, I had to catch the transaction-started signal also so that I could capture the type of the transaction.
Indeed its not send! Its comming from
softwarecenter/backend/installbackend_impl/aptd.py so we could extend
it. But I guess its fine as it is.
> >
> > > + def _on_transaction_finished(self, backend, result):
> > > + if result.pkgname in self.recommended_apps_viewed:
> > > + self.recommended_apps_viewed.remove(result.pkgname)
> > > + if network_state_is_connected():
> > > + # no need to monitor for an error, etc., for this
> > > + self.recommender_agent.post_implicit_feedback(
> > > + result.pkgname,
> > > + RECOMMENDATIONS_FEEDBACK_INSTALLED)
> >
> > If the error condition does not matter, then we can probably also skip
> > the network check.
>
> Yeah, it's just an added check to avoid the exception. No need to try to hit the server if we aren't connected, so I think it's ok. No?
Yeah, its ok t leave it there.
One tiny bit (that I can just changed during the merge) is that
"rec_item_tile.on_press()/rec_item_tile.on_release()" can be written
as "rec_item_tile.clicked()".
Its all looking good now, but I discovered a issue during my final
testing, I added:
=== modified file 'softwarecenter/backend/recagent.py'
--- softwarecenter/backend/recagent.py 2012-08-30 00:08:39 +0000
+++ softwarecenter/backend/recagent.py 2012-08-31 08:15:41 +0000
@@ -220,6 +220,7 @@ "SoftwareCenterRecommenderAPI", "recommend_top")
On Thu, Aug 30, 2012 at 09:25:21PM -0000, Gary Lasker wrote: installed- feedback into lp:software-center with lp:~gary-lasker center/ update- to-latest- recommender- client as a prerequisite. recommender- client branch as that branch adds the new recommender pristine. py, please also
> > On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> > > Gary Lasker has proposed merging lp:~gary-lasker/software-center
> > /recommended-
> > /software-
> >
> > Thanks for working on this branch. I'm very happy to see this feature
> > coming.
> >
> > [..]
> > > Note that this branch depends on the lp:~gary-lasker/software-center/update-
> > to-latest-
> > client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-
> > recommender-client. Also note that I made one small change to sreclient to fix
> > the validate decorator for the pkgname argument to implicit_feedback so that
> > it matches the validator for the other calls that use pkgname as an argument.
> >
> > Thanks for this, if you fixed the sreclient_
> > send the diff to the server team (if you haven't done already) so that
> > they add it to their
> > lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
> > branch.
>
> Yep, I proposed a branch for this and it has been merged by Lukasz.
Great, thanks for this.
> > > Finally, I added a unit test for the new call in test_recagent.py, however test_recommenda tions_widget. py. Something like activate a r_agent. post_implicit_ feedback( ) call was done.
> > like a few of the other tests there, the test is not currently working with
> > the server. Therefore, it remains disabled as the other tests are. I'll look
> > at this tomorrow to try to see what the issue is with these tests.
> > [..]
> >
> > Maybe the staging server had problems. I just tested enabling them and
> > its working, but that may well be that its now fixed. While looking at
> > the test code I noticed that there is a bit of duplication in
> > there, I pushed a branch at
> > lp:~mvo/software-center/recagent-test-cleanup that removes most of
> > that.
> >
> > I also noticed that the branch is missing a test for the new
> > functionality, it would be nice to add one to
> > tests/gtk3/
> > app, trigger a install transaction and verify that on finish the
> > self.recommende
> >
>
> I added a thorough test for this functionality in test_catview.py (this is where the other recommendations panel tests are, and imo it's consistent because the tests for the other lobby panels (What's New and Top Rated) are also located here. While in test_catview.py I did some cleanup and consolidation of duplicated code as well.
Thanks for adding the test and for the cleanup there, that looks very
good!
[..] d_apps_ viewed we cleared when we get new data, recommendations _agent_ refresh (and the equivalent
> > Should self.recommende
> > e.g on _on_app_
> > function for the categories). Or should this be persistent? I'm don't
> > know the details of the spec well enough :) I guess it comes down to
> > if we should consider it a recommendation that are reached
> > "indirectly", e.g. click on it "app A", move back to main view search
> > for something else and then reach "app A" again later (without
> > following a recommendation) if installing that should be counted or
> > not. I don't really mind either way, but would like to know.
> >
>
> Ok, so what happens is that when a person clicks on a recommended item, we remember that they did that for that item for as long as the current USC session is active (it is now, however, persisted between sessions, I think would be a mistake). I believe the way it works currently is optimal because I think that a person clicking on a recommended item probably became aware of that item *because* of the recommendation, and so we still want to report if that item is installed even if the install did not occur immediately after clicking through the recommendation. In other words, if the user looks at a recommended item, then navigates around a bit before revisiting (and installing) that recommended item, I think that should be considered a successful recommendation.
>
> That's why it's designed as it is. If we decide this isn't exactly what we want, we can change it easily.
Yeah, that is ok, I just wanted to raise it as a point of
consideration.
> > > + def _on_transaction _started( self, backend, pkgname, appname, trans_id, s.INSTALL and d_apps_ viewed) : d_apps_ viewed. remove( pkgname) _finished( ) ? So that there is only a single signal finished signal does *not* include the transaction type. So, I had to catch the transaction-started signal also so that I could capture the type of the transaction.
> > > + trans_type):
> > > + if (trans_type != TransactionType
> > > + pkgname in self.recommende
> > > + # if the transaction is not an installation we don't want to
> > > + # track it as a recommended item
> > > + self.recommende
> >
> > Could the check for the transaction type simply be folded into the
> > _on_transaction
> > type to watch?
>
> I wanted to do that! Definitely it would be preferable. However, I could not because (strangely) the transaction-
Indeed its not send! Its comming from backend/ installbackend_ impl/aptd. py so we could extend
softwarecenter/
it. But I guess its fine as it is.
> > _finished( self, backend, result): d_apps_ viewed: d_apps_ viewed. remove( result. pkgname) state_is_ connected( ): r_agent. post_implicit_ feedback( _FEEDBACK_ INSTALLED)
> > > + def _on_transaction
> > > + if result.pkgname in self.recommende
> > > + self.recommende
> > > + if network_
> > > + # no need to monitor for an error, etc., for this
> > > + self.recommende
> > > + result.pkgname,
> > > + RECOMMENDATIONS
> >
> > If the error condition does not matter, then we can probably also skip
> > the network check.
>
> Yeah, it's just an added check to avoid the exception. No need to try to hit the server if we aren't connected, so I think it's ok. No?
Yeah, its ok t leave it there.
One tiny bit (that I can just changed during the merge) is that tile.on_ press() /rec_item_ tile.on_ release( )" can be written tile.clicked( )".
"rec_item_
as "rec_item_
Its all looking good now, but I discovered a issue during my final /backend/ recagent. py' backend/ recagent. py 2012-08-30 00:08:39 +0000 backend/ recagent. py 2012-08-31 08:15:41 +0000
" SoftwareCenterR ecommenderAPI" , "recommend_top")
testing, I added:
=== modified file 'softwarecenter
--- softwarecenter/
+++ softwarecenter/
@@ -220,6 +220,7 @@
def post_implicit_ feedback( self, pkgname, action):
spawner. parent_ xid = self.xid
+ print "++++++++++ implicit", pkgname, action
# build the command
spawner = SpawnHelper()
and clicked on a random item in the "Whats new" section of the
lobby (in my case ggcov) and I got on the terminal:
++++++++++ implicit ggcov installed
The same for items in the lobby in the top-rated section. Its 100%
reproducable for me. Please have a look if you see this as well.
Cheers,
Michael