Page MenuHomePhabricator

Code Review & Merge - Improved Property Suggester/Recommender service patches
Closed, ResolvedPublic8 Estimated Story Points

Description

Over the past months a team at Vrije Universiteit Amsterdam have been working on an improved backend for PropertySuggester.
In order to prepare for deployment we want to have a final review of the involved code changes to PropertySuggester and get them merged.

All patches can be found at https://gerrit.wikimedia.org/r/q/owner:marta.jansone99%2540gmail.com and involve changes to PropertySuggester and also some EventLogging schema.

There is a test service that you can hook up to on CloudVPS.
The build and setup instructions for that can be found at https://github.com/martaannaj/RecommenderServer#setting-up-on-cloud-vps
Example call curl -d '{"properties":["http://www.wikidata.org/prop/direct/P31"],"types":[]}' https://recommender.wmcloud.org/recommender

Acceptance Criteria:

  • All needed code patches are merged, or +1ed and approved ready for merge
  • The existing default behaviour of PropertySuggester remains the same
  • The new backend can be enabled using configuration and enabled using config variables

Event Timeline

I just wanted to say while I love to get this into production, I highly object to get the code in production while trying to read the cloud service in the backend (i.e. deploying the code without deploying the service to production). We tried to do this for ores several years ago and it turned out that it is not a good idea.

  • The reason production doesn't speak to the outside is security, we are exposing a big vector attack. That service can be taken over somewhat easier.
  • The defense in depth principle being applied in production implies anything we depend on must have a security review. This effectively bypasses that principle.
  • Production and Cloud have way different SLOs. The service can go down for any reason for hours or more, degrading a production functionality or worse causing exceptions and possibly an outage in production.

TLDR this ticket is about code review of these things, not about deploying this thing to production right now.


I just wanted to say while I love to get this into production, I highly object to get the code in production while trying to read the cloud service in the backend (i.e. deploying the code without deploying the service to production). We tried to do this for ores several years ago and it turned out that it is not a good idea.

So this is primarily for an A/B test, and after the A/B test is complete we can make the decision around 1) full production deployment including the service in production or 2) keep using the existing thing.
There is little point in multiple teams investing work on getting something deployed into product, that in theroy after a month of so we then just need to undeploy and unwire etc.

The reason production doesn't speak to the outside is security, we are exposing a big vector attack. That service can be taken over somewhat easier.

We can control and "lock down" the service in cloud VPS as much as we and other see fit.
No remotely private data is sent to the service by the application.
Any data read from the application can also be sanitized / checked for validity, and again this would be a very small attack vector if any (this is something that should come up in CR of the patches linked by this ticket.

The defense in depth principle being applied in production implies anything we depend on must have a security review. This effectively bypasses that principle.

This area can indeed have a security review of some sorts etc.
As mentioned in the story time call, there is an ongoing discussion with WMF around if and how we can do this.
This ticket only relates to the code that would make something like this possible, not about the deploying to production bit, which would come after.
Even if we don't end up deploying this to production and talking to labs, this code is still needed in the extension.

Production and Cloud have way different SLOs. The service can go down for any reason for hours or more, degrading a production functionality or worse causing exceptions and possibly an outage in production.

This is also something that should come up in code review of the patches.
We should be ready for the service to not be there, and to gracefully fallback to the other solution, or whatever product @Lydia_Pintscher is happy with in said situation.

I’ve started looking at the patches, but I know almost nothing about the EventLogging parts.

I’ve started looking at the patches, but I know almost nothing about the EventLogging parts.

It's highly likely that we will not be the folks to +2 the schemas themselves for EventLogging, but we should ensure that the code submitting events appears to lineup with the schemas that are defined.

[...]
Even if we don't end up deploying this to production and talking to labs, this code is still needed in the extension.
[...]

Could you elaborate on this? If it is decided to not deploy that new suggester backend somewhere and talk to it, then I'm not sure what value these patches add and what they are needed for.
Or is this intended to be also used in other Wikibase installations?

Could you elaborate on this? If it is decided to not deploy that new suggester backend somewhere and talk to it, then I'm not sure what value these patches add and what they are needed for.
Or is this intended to be also used in other Wikibase installations?

For deployment on beta for example this code is still needed (or any other wikibase)

Could you elaborate on this? If it is decided to not deploy that new suggester backend somewhere and talk to it, then I'm not sure what value these patches add and what they are needed for.
Or is this intended to be also used in other Wikibase installations?

For deployment on beta for example this code is still needed (or any other wikibase)

Ok, so the idea is that even if we cannot evaluate it on production because of security concerns, we would still want to evaluate it on beta and other places. Gotcha. Thanks.

Indeed, and I have a note written down regarding production deployment to write a phabricator ticket for that soon.
We also have a call / more emails with WMF around the topci scheduled for next week.

I removed the token as somehow I had thought the third party server was only for testing, not for the actual update of the feature.

If we deploy new version of recommendation system, it worths a new service in WMF production instead of a Cloud VPS instance.

The more important problem of a student project is maintanence - https://www.mediawiki.org/wiki/Google_Summer_of_Code/Past_projects#Wikidata_Entity_Suggester was a GSoC project and is not deployed to the production in favor of the current PropertySuggester; WikibaseQuality was also unmaintained for several years.

Where is the analysis and design documentation for the Improved Property Suggester?

Found it at SchemaTree: Maximum-Likelihood Property Recommendation for Wikidata

If we deploy new version of recommendation system, it worths a new service in WMF production instead of a Cloud VPS instance.

Indeed, this will be detailed in T285098: Production A/B test deployment - Improved Property Suggester/Recommender

Although I do not have a screenshot of the new suggester, it would be great if they work on T111154: [Story] Entity suggester should also suggest values/

UI-wise nothing changes with the new suggester. It's just supposed to give better suggestions for Properties compared to the current one. I'll bring up value suggestions as something we might want to have in the future.

the 2 code patches are now merged, pending figuring out what to do with the other 1/2 of the event logging stuff

Addshore claimed this task.