Hey @Mholloway, can you tag the appropriate teams and add a description with links and what we need, with some approximate wanted date of deployment?

@Jhernandez I don't think there's anything to link to; this is just to give the Performance team a chance to look over the extension before initial deployment. I know @egardner wanted a chance to update some of the frontend JS code before that happens. I think we'll be ready to ping Performance by late this week or the beginning of next week.

Pinging @aaron about this since you worked on it last time (and it looks like Imarlier is the only current/former Performance person getting an email notification about it).

("last time" === the WikimediaEditorTasks performance review)

Performance reviews need to be requested before the quarter starts, so we can plan capacity for it and add it to our own OKRs. Sending an email to performance-team@ or filing a task like this one while tagging Performance-Team is enough to get that conversation started.

Also, it's in your best interest to involve us sooner than when everything is fully written. The latter performance issues are identified, the more likely they will be blocking or significantly delaying deployment. It seems like you were already working on this last quarter and we could have done a first pass then (again, requested before the beginning of that quarter), increasing the likelihood that the final review before deployment is a formality less likely to reveal big problems.

Thanks for the response, @Gilles. Sorry I didn't request this earlier; it seems I badly miscalculated when it would be appropriate to involve your team. My assumption was that it would not be worth your time to look at code that was under very active development. (I searched the wikis for some guidance on when and how to request a performance review but didn't find any; that would help a lot.) Where does this leave us, then?

I will take a first look this week, but if it turns out to be time-consuming I will have to pass the torch to someone else, since I'm OOO next week.

Thank you, @Gilles. The extension README should be up to date, but please let me know if you run into any setup issues.

FYI the Vagrant role for this extension fails to provision for me:

==> default: Warning: Unknown variable: '::mediawiki::wiki_name'. at /vagrant/puppet/modules/role/manifests/machinevision.pp:16:20
==> default: Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, 'regsubst' parameter 'target' expects a value of type Array or String, got Undef at /vagrant/puppet/modules/apache/manifests/site_conf.pp:37:19  at /vagrant/puppet/modules/role/manifests/machinevision.pp:15 on node vagrant.mediawiki-vagrant.dev
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.

Sorry about that. I'm testing a fix for the role now.

Change 545305 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MachineVision@master] build: Misc clean ups for new repo

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

Change 545307 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MachineVision@master] Avoid generic 'moduleID' global variable

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

Two minor patches. Not a blocker by any means. Ignore me :)

It seems like the Vagrant role is very partial in what it does, compared to all the instructions on README.md. Could you provide a fully set up Docker or Vagrant image to save some time?

Change 545305 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] build: Misc clean ups for new repo

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

Change 545307 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] Avoid generic 'moduleID' global variable

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

@Gilles After (a lot of) experimenting to find the most convenient way to set up all of the required dependencies and get the most usable result, I ended up creating a Docker-based environment for testing. Please clone https://github.com/mdholloway/machinevision-performance-review and follow the setup instructions at the top of the README file. (The setup script requires access to a separate private repo, to which I've added you as a collaborator.) Hopefully you will have better luck with that than with the Vagrant role.

All right. Thanks for taking the time to look again, @Gilles. I've tweaked the setup script so that whoever looks next is less likely to hit a permissions issue.

Whoever picks this up, please ping me for access to the required private repo.

Change 546141 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/extensions/MachineVision@master] [DNM] Various small performance review tweaks

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

Aside from the things mentioned in the above patch, the overall code looks OK to me.

Thanks very much for the review, @aaron. I'll address the feedback tomorrow.

Change 547688 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/MachineVision@master] [WIP] Performance: Execute labeling requests in a Job, not a DeferredUpdate

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

Change 546141 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] Various performance review tweaks and comments

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

Change 547688 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] Performance: Execute labeling requests in a Job, not a DeferredUpdate

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

The last remaining FIXME was resolved by https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MachineVision/+/548583/. Thanks again @aaron for the review!

Change 548924 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MachineVision@master] docs: Avoid class name strings in PHP, spelling of MediaWiki

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

Change 548914 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MachineVision@master] Avoid deprecated wfWikiID()

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

Change 548926 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MachineVision@master] resources: Scope RL package module to more specific directory

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

Change 548914 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] Avoid deprecated wfWikiID()

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

Change 548924 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] docs: Avoid class name strings in PHP, spelling of MediaWiki

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

Sorry for the confusion. The extension should not be difficult to install, per se, but there is a certain amount of work involved (as described in the README) in setting up its core functionality. Do you need to assess the entire label fetching and presentation flow, or do you just need to get the extension installed in order to, e.g, assess the effect of the additional JS modules on page load times?

From a user-facing perspective this is essentially just a Special page. It has no special operational metrics in place, but we'll be happy to add any you suggest.

In T230813#5648838, @Krinkle wrote:

In T230813#5599191, @Mholloway wrote:

Whoever picks this up, please ping me for access to the required private repo.

I thought maybe the ticket was missing some updates that were communicated elsewhere, but from what I understand our team was unable to install the extension. The review thus-far covered the backend and was based on static review of the code. Next steps is to figure out a minimal way to install (part of) it for frontend review.

Right, I only did backend CR. Are you doing the front-end review?

Change 548926 merged by jenkins-bot:
[mediawiki/extensions/MachineVision@master] resources: Scope RL package module to more specific directory

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

Following up from the retrospective, my understanding is that the ideal is to use the Beta Cluster as the review environment. The MachineVision BC configuration is currently quite different from production but I'll work on updating it to match production now.

Change 554630 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[operations/mediawiki-config@master] MachineVision: Update Beta settings to (mostly) match production

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

Change 554630 merged by jenkins-bot:
[operations/mediawiki-config@master] MachineVision: Update Beta settings to (mostly) match production

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

OK: https://commons.wikimedia.beta.wmflabs.org/wiki/Special:SuggestedTags and uploaded image labeling on Beta are now fully functional. Please let me know if there's anything else I can do to facilitate the remaining review.

I've done the minimal sanity check of frontend payload and see that the newly introduced RL module is correctly only loaded on the special page, making this very low-risk in terms of site-wide frontend performance regression. The feature itself on the special might have possible improvements, which I'll let @Krinkle look into, but this can roll out as-is.

I was a bit surprised to see that the Special page needs JS to work at all, but I don't know what the current policy about this is for a Special page. It would be nice to have at least some fallback text there for grade C browsers, currently the experience without JS is just a blank page.

Thank you, @Gilles. I'll make sure that some no-JS fallback text lands in wmf.10.