Page MenuHomePhabricator
Paste P2815

Meeting-E150-wikimedia-office.2016-03-23-21.03.log.txt
ActivePublic
Actions

Authored by • RobLa-WMF on Mar 24 2016, 10:45 PM.
Tags
Referenced Files
F3706243: Meeting-E150-wikimedia-office.2016-03-23-21.03.log.txt
Mar 24 2016, 10:51 PM
F3706203: Meeting-E150-wikimedia-office.2016-03-23-21.03.log.txt
Mar 24 2016, 10:45 PM
Subscribers
None
Copied from https://tools.wmflabs.org/meetbot/wikimedia-office/2016/wikimedia-office.2016-03-23-21.03.log.txt
21:03:20 <DanielK_WMDE_> #startmeeting RFC office hour
21:03:21 <wm-labs-meetbot> Meeting started Wed Mar 23 21:03:20 2016 UTC and is due to finish in 60 minutes. The chair is DanielK_WMDE_. Information about MeetBot at http://wiki.debian.org/MeetBot.
21:03:21 <wm-labs-meetbot> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
21:03:21 <wm-labs-meetbot> The meeting name has been set to 'rfc_office_hour'
21:03:34 <gwicke> next is #topic
21:03:40 <DanielK_WMDE_> #topc Service Locator for MediaWiki core https://phabricator.wikimedia.org/T124792
21:03:47 <DanielK_WMDE_> #link https://phabricator.wikimedia.org/T124792
21:03:54 <DanielK_WMDE_> \o/
21:03:55 <bd808> DanielK_WMDE_: spelling
21:03:57 <gwicke> #topic Service Locator for MediaWiki core https://phabricator.wikimedia.org/T124792
21:04:20 <gwicke> heh, you need to do it as the chair
21:04:22 <DanielK_WMDE_> bd808: hm?
21:04:25 <bd808> only DanielK_WMDE_ can do that unless he adds another admin
21:04:35 <bd808> DanielK_WMDE_: "#topc"
21:04:47 <DanielK_WMDE_> hehe
21:04:52 <DanielK_WMDE_> #topic Service Locator for MediaWiki core https://phabricator.wikimedia.org/T124792
21:04:57 <DanielK_WMDE_> how do i add an admin?
21:05:05 <DanielK_WMDE_> anyway. quick summary:
21:05:20 <DanielK_WMDE_> since the last meeting about this topic, I have been working on code experiments.
21:05:49 <DanielK_WMDE_> There is a patch that would introduce a serivce locator into core: https://gerrit.wikimedia.org/r/#/c/264403/
21:06:13 <DanielK_WMDE_> Some important points about the Service Locator (aka DI container):
21:06:17 <DanielK_WMDE_> * no auto-wiring
21:06:23 <MaxSem> +1115, -127 - not that bad :P
21:06:23 <DanielK_WMDE_> * no 3rd party lib
21:06:40 <DanielK_WMDE_> MaxSem: yea, well, there's more in the follow-ups
21:06:51 <DanielK_WMDE_> * closures in php files for wiring
21:07:07 <DanielK_WMDE_> * extensible: extensions can add, replace, and wrap services
21:07:25 <DanielK_WMDE_> Any questions/comments so far?
21:07:48 <Scott_WUaS> thanks, not yet
21:07:57 <RoanKattouw> DanielK_WMDE_: Are there usage examples anywhere?
21:07:57 <DanielK_WMDE_> i have one:
21:08:02 <SMalyshev> DanielK_WMDE_: so the clients interact with the container by calling static methods?
21:08:10 <DanielK_WMDE_> RoanKattouw: in the follow-up patches, yes
21:08:19 <RoanKattouw> I see
21:08:44 <DanielK_WMDE_> SMalyshev: one static method to get the main instance. application code should have the services injected though.
21:09:04 <DanielK_WMDE_> SMalyshev: only "static" code (like hook handlers) would access the main service locator through a static method
21:09:12 <SMalyshev> DanielK_WMDE_: ah, so ServiceLocator::getInstance()->doStuff() ?
21:09:16 <DanielK_WMDE_> RoanKattouw: the ticket has a rough overview of the patches
21:09:38 <DanielK_WMDE_> SMalyshev: MediaWikiServices::getInsteance()->getFooThings()->doFoo()
21:10:16 <DanielK_WMDE_> SMalyshev: but that's not typical. You'd rather see: new Thingy( MediaWikiServices::getInsteance()->getFooThing() )
21:10:59 <SMalyshev> DanielK_WMDE_: aha, I see. how would you make "getFooThing" extendable? i.e. add new "FooBarThing"?
21:11:09 <SMalyshev> would extension be able to do it?
21:11:23 <DanielK_WMDE_> SMalyshev: there are getters for well known services, and a generic one for all other services.
21:11:37 <DanielK_WMDE_> getFooThingy() is a shorthand for getService( 'FooThingy' )
21:11:45 <matt_flaschen> DanielK_WMDE_, what's the real benefit of the convenience getter, type-safety?
21:11:45 <SMalyshev> I see.
21:11:50 <DanielK_WMDE_> services defined by extensions wouldn't have conveniance methods
21:12:03 <DanielK_WMDE_> matt_flaschen: yes, type safety and typo safety
21:12:11 <RoanKattouw> DanielK_WMDE_: So the area I'm most interested in right now is ServiceWiring.php. Do you have an example of what the wiring would look like for a service that depends on another service?
21:12:55 <MaxSem> one thing current code is missing is teardown. this functionality should be present for tests
21:13:12 <matt_flaschen> RoanKattouw, MainConfig at https://gerrit.wikimedia.org/r/#/c/250150/20/ServiceWiring.php
21:13:23 <matt_flaschen> Depends on ConfigFactory.
21:13:23 <DanielK_WMDE_> RoanKattouw: sure, pretty much all wiring in the initial patch already depends on other services. The closure gets the service locator as a param, so it has access to other services.
21:13:29 <RoanKattouw> Oh I see
21:13:48 <matt_flaschen> It's pretty similar to how Flow uses Pimple. We just don't have the convenience getters.
21:14:00 <MaxSem> also, I don't like the concept of wiring files tht just get randomly included
21:14:33 <DanielK_WMDE_> MaxSem: "randomly included"?
21:14:55 <MaxSem> "randomly" is probably not the best word
21:15:03 <DanielK_WMDE_> a, i see. well, it's not very pretty, but i don't think it's a problem
21:15:09 <MaxSem> but do you realise you're just reimplementing autoloader?
21:15:14 <RoanKattouw> OK and the service objects are cached and lazy-instantiated
21:15:22 <matt_flaschen> MaxSem, how's that?
21:15:25 <DanielK_WMDE_> the alternatives are: make it all declarative (and come up with a DSL for this), like spring
21:15:32 <matt_flaschen> autoloader doesn't handle services depending on other services?
21:15:33 <RoanKattouw> Yeah, I was mostly trying to make sure that the nice parts from Pimple are present
21:15:50 <DanielK_WMDE_> or wrap all of this in closes. that means a lot of overhead, and potential performance issues
21:16:03 <DanielK_WMDE_> using plain arrays with plain closures means loading no classes and creating no objects at init time
21:16:36 <DanielK_WMDE_> MaxSem: this is very different from autoloader. autoloader is class -> file. This is name -> object.
21:16:39 <MaxSem> matt_flaschen, autoloader is class => file mapping. this wiring thingie is a service => file mapping
21:16:56 <DanielK_WMDE_> MaxSem: this doesn't map to files. it maps to instances.
21:17:26 <matt_flaschen> MaxSem, no it's not. It's a "how to make a service recipe". It's not about files.
21:17:40 <MaxSem> hmm oadWiringFiles( array $wiringFiles )
21:17:42 <DanielK_WMDE_> RoanKattouw: yea, it's all lazy init and caches. the caching is actually a problem, i'll come to that in a minute if there are no further questions.
21:17:49 <SMalyshev> yeah I don't think inventing another DSL is a good idea. We'll just end up having mini-PHP inside PHP
21:17:50 <MaxSem> *loadWiringFiles( array $wiringFiles )
21:17:57 <matt_flaschen> MaxSem, that's just how the recipes are loaded.
21:18:02 <DanielK_WMDE_> MaxSem: yes, extensions can add more wiring files. this was requested by tim last time.
21:18:36 <legoktm> DanielK_WMDE_: is there going to be actual code in these files or just arrays?
21:18:41 <DanielK_WMDE_> SMalyshev: indeed. dependency tracking would be the only advantage. i don't think it's worth the pain
21:19:11 <DanielK_WMDE_> legoktm: no code on the top level. just arrays of closures, and in each closure the code to instantiate a service. that may be trivial or not so much....
21:19:17 <MaxSem> DanielK_WMDE_, why not set it in extension.json?
21:19:31 <DanielK_WMDE_> Here's a later version of the wiring file, with more "meat" to it: https://gerrit.wikimedia.org/r/#/c/267692/28/ServiceWiring.php
21:19:40 <DanielK_WMDE_> most of the "meat" is actually B/C stuff with old style config
21:20:14 <DanielK_WMDE_> MaxSem: that's exactly what this is for. in extension.json, you register your wiring file by adding it to the global
21:20:15 <SMalyshev> DanielK_WMDE_: do you have an example of overriding service, e.g. for test mocking?
21:20:35 <DanielK_WMDE_> SMalyshev: yes, but it'S WIP
21:20:39 <DanielK_WMDE_> let me dig up the link...
21:21:04 <SMalyshev> that looks like very frequent use case for replacing services
21:21:45 <DanielK_WMDE_> SMalyshev: well, only for B/C. For new style code, you'd just inject the mock. no need to do anything with the global service locator
21:21:54 <DanielK_WMDE_> but sure, we will need a mechanism for this.
21:22:34 <DanielK_WMDE_> SMalyshev: https://gerrit.wikimedia.org/r/#/c/267692/28/tests/phpunit/MediaWikiTestCase.php
21:23:06 <DanielK_WMDE_> there I introduce MediaWikiTestCase::overrideMwServices
21:23:20 <DanielK_WMDE_> my version is still fialing some tests, but i'm getting there
21:23:25 <DanielK_WMDE_> it's not as bad as i feared
21:24:09 <SMalyshev> ah,I see, thanks
21:24:24 <DanielK_WMDE_> the problem is that services depend on each other, and we do not track how.
21:24:53 <DanielK_WMDE_> so if you replace one service, other services may use the old or the new instance.
21:25:08 <DanielK_WMDE_> this is rather tricky to get right. the only safe way to do this is to reset *everything*
21:25:31 <DanielK_WMDE_> I cose not to do this during testing (test code has to care about avoiding inconsistencies)
21:25:44 <DanielK_WMDE_> but in production, if any services needs resetting, all services have to be reset
21:26:04 <DanielK_WMDE_> and that's actually the biggest issue i'd like to discuss today:
21:26:50 <DanielK_WMDE_> during initialization, extensions max access services (like the config factory), but they may also change configuration. Which in turn would impact services. So initialization order matters.
21:27:47 <DanielK_WMDE_> One nasty case: an extension adds a new config in wgConfigRistry. But the ConfigFactory is already instantiated (the extension loader needs it, because it needs access to caches). So the change to wgConfigRegistry has no effect on the existing ConfigFactory.
21:28:19 <DanielK_WMDE_> For now, I solve this by resetting all services after the initialization phase. this is not very pretty though. Any ideas how to do this more nicely?
21:28:58 <RoanKattouw> Where can I read more about resetting services?
21:28:59 <DanielK_WMDE_> (look at "Accessing Services During Initialization" in the ticket for more info)
21:29:04 <RoanKattouw> Like, what is it, when is it used, why is it needed, etc
21:29:07 <RoanKattouw> Oh OK
21:29:11 <matt_flaschen> DanielK_WMDE_, if wgConfigRegistry is modified using extension.json, then the extension loader could mark that, and only do the full reset if it was actually modified. Though if this is always done it's a moot point.
21:29:32 <DanielK_WMDE_> RoanKattouw: also the section before that, called "Resetting the Service Locator".
21:30:01 <DanielK_WMDE_> matt_flaschen: but other configuration may affect other services, and we don't know which or how
21:30:25 <RoanKattouw> Interesting
21:30:35 <DanielK_WMDE_> SMalyshev: btw, the RFC also has a section called "Forcing Services During Testing" :D
21:30:43 <bd808> I doubt there is going to be a more graceful solution that to dump the instance cache once $wgFullyInitialised === true
21:31:06 <legoktm> [14:28:19] <DanielK_WMDE_> For now, I solve this by resetting all services after the initialization phase. this is not very pretty though. Any ideas how to do this more nicely? <-- don't allow extensions to add config late
21:31:11 <bd808> Until $wgFullyInitialised === true a lot of things are sketchy
21:31:36 <DanielK_WMDE_> bd808: well, i can think of a more craceful one, but that has other issues: fully declarative wiring. that way, you can track all dependencies, resetting only exactly what is needed. but it's maintenance hell, and not possible with legacy code.
21:31:58 <bd808> DanielK_WMDE_: crawl, walk, run :)
21:31:58 <DanielK_WMDE_> legoktm: yes, that is true. it's actually intentional.
21:32:12 <SMalyshev> declarative means no IDE help when configuring service ctors
21:32:15 <RoanKattouw> I hadn't considered these cases because they don't arise in Flow's use of DI, but they arise here because you're using DI for things like DB connections
21:32:25 <DanielK_WMDE_> legoktm: global state is evil. changing global state with the expectation to change behavior of pre existing objects is more evil.
21:32:33 <SMalyshev> i.e. if I add an argument, who knows if I add it in the same place in declarative config
21:33:14 <DanielK_WMDE_> SMalyshev: yea, i'm not a fan of declarative. well, in theory i am, actually. but not in practice. not here, not now. let's keep things simple and explicit: closures that return objects.
21:34:32 <DanielK_WMDE_> RoanKattouw: yea, DB connections are a fun can of worms, especially with the fun unittest fake database tables that are bound to a connection. if you close it, you have to re-create the tables... that kind of fun is why the patches are all > 1000 lines :/
21:35:16 <matt_flaschen> RoanKattouw, we do use container resets in a few places in Flow. Mostly just in tests, but also one trick we do with PurgeAction.
21:35:17 <legoktm> DanielK_WMDE_: so I don't see why it's a case we have to worry about...we should just explictly not support anything registering things late, nor should we permit things to access it early either
21:35:31 <DanielK_WMDE_> bd808: so, if we do reset all service instances once, should we track for which ones that would be particularly bad? I mean, we should really avoid re-connecting to the database, or the memcached server.
21:35:41 <DanielK_WMDE_> In the ticket, i proposed a coupld of possible solutions:
21:36:04 <DanielK_WMDE_> 1) Allow services to declare that they do not need to be re-created when configuration changes, perhaps by using a marker interface. Such services would need to use an entire Config object, or some kind of "promise" object, to keep track of changing configuration.
21:36:06 <DanielK_WMDE_> 2) Blacklist some services from being accessed during initialization. One way to do this is to have a separate service locator for "primordial" services, with a separate wiring file. This assumes that services that were not blacklisted can be kept alive through config changes. This is tricky, since it's already not true for the ConfigFactory (extensions can register new configurations), and pretty much everything depends on using
21:36:07 <DanielK_WMDE_> configuration.
21:37:04 <matt_flaschen> DanielK_WMDE_, could we blacklist all configs except 'main' during initialization?
21:37:14 <DanielK_WMDE_> matt_flaschen, RoanKattouw: forking maintenance scripts is another case that needs a full reset. and the installer! took me a while to figure that one out.
21:37:53 <DanielK_WMDE_> matt_flaschen: i suppose so. i'd rather want to blacklist LoadBalancer, though...
21:39:23 <DanielK_WMDE_> legoktm: "nor should we permit things to access it early either" <-- that doesn't work for things the extension loader itself needs. also, it will break compat with some extensions.
21:39:35 <bd808> DanielK_WMDE_: hmm.. yeah by the time that $wgFullyInitialised === true a lot of things will have been built. main cache, message cache, parser cache, sessionmanager, possibly db connections for authn, ...
21:39:40 <SMalyshev> DanielK_WMDE_: maybe not blacklist but whitelist? I.e. if we say we only should access some very basic services during init, maybe not allow to access others then
21:39:40 <DanielK_WMDE_> legoktm: it would be really nice if we had a clean init phase with no service access, but that's not how it is.
21:40:13 <DanielK_WMDE_> bd808: loading the message cache twice would suck. so maybe whitelist some services that could be kept?
21:41:05 <DanielK_WMDE_> SMalyshev: yea, perhaps. i see two kinds of whitelist: a list of things that it's ok to access, and a list of things that can be kept and doesn't need a reset.
21:41:14 <bd808> that might work. "locking" some services as soon as ExtensionRegistry is done loading maybe
21:41:20 <DanielK_WMDE_> actually, the latter should be a marker interface, i suppose
21:41:50 <MaxSem> DanielK_WMDE_, lists of exceptions are a sure ways to feet shot off
21:41:51 <SMalyshev> DanielK_WMDE_: yeah the second looks like anybody should be able to claim it
21:42:06 <DanielK_WMDE_> MaxSem: propose a better way
21:42:27 <DanielK_WMDE_> SMalyshev: i think i'll experiment with this, yea
21:42:29 <SMalyshev> i.e. if my service does not keep anything special in the instance, I should be able to say "just use the same object always"
21:43:04 <DanielK_WMDE_> SMalyshev: if it doesn't use anything special, re-creating it doesn't hurt
21:43:14 <DanielK_WMDE_> it's the edge cases that make the fun
21:43:14 <SMalyshev> DanielK_WMDE_: except performance :)
21:43:17 <DanielK_WMDE_> MessageCache...
21:43:35 <DanielK_WMDE_> SMalyshev: creating a trivial object is no problem. fetching stuff from the database twice is a problem
21:44:26 <MaxSem> and then someone overrides this trivial object with something that does stuff on init...
21:45:08 <DanielK_WMDE_> MaxSem: the override would thake effect after the init phase. nothing can override anything before the init phase.
21:45:17 <DanielK_WMDE_> anyway, we are reaching the last quarter of this... so let me ask:
21:45:26 <bd808> The problem I see with early init and locking is that until $wgExtensionFunctions are run you really don't know what the state of the global config is
21:45:27 <DanielK_WMDE_> who if you is going to be at the hackathon?
21:45:34 <DanielK_WMDE_> and who's interested in a session on this?
21:45:38 <bd808> and that happens very late in Setup.php
21:45:41 <MaxSem> not me:P
21:45:46 <DanielK_WMDE_> (and ho is NOT going to be at the hackathong, but still interested?)
21:46:13 <SMalyshev> I'll be in the hackathon and may be interested
21:46:32 <bd808> I'll be at the hackathon and willing to talk. I think the real proof of this is going to be more code though.
21:46:33 <SMalyshev> need to read more about it before it I guess :)
21:46:52 <DanielK_WMDE_> bd808: do you think this is the right place? https://gerrit.wikimedia.org/r/#/c/270020/25/includes/Setup.php
21:46:57 <DanielK_WMDE_> bd808: if not, please comment there
21:47:13 <DanielK_WMDE_> SMalyshev: that would be appreciated :)
21:47:26 <SMalyshev> I agree, the only way to know how it works is trying to write a code with it. The theory looks fine, but the devil is always in the details :)
21:47:51 <DanielK_WMDE_> bd808: there is already quite a lot of code, approaching 10k lines. I'd like a bit more fedback before i steal more hours from wmde working on this :P
21:48:49 <DanielK_WMDE_> bd808, SMalyshev: we (ArchCom) are considering setting up a "working group" to tackle this (and similar tasks). Would you be interested?
21:49:06 <SMalyshev> DanielK_WMDE_: yes, I think so
21:49:33 <bd808> DanielK_WMDE_: you're not going to trick me that easily ;)
21:49:40 <bd808> but yeah I'm interested
21:49:47 <DanielK_WMDE_> hehe :P
21:49:49 <DanielK_WMDE_> awesome
21:50:12 <DanielK_WMDE_> so... anyone else interested in joining a group like that?
21:50:23 * bd808 nominates legoktm
21:50:52 <legoktm> I'm interested
21:50:56 <DanielK_WMDE_> cool :D
21:51:08 <Scott_WUaS> (I/WUaS are not planning to go to the hackathon at this point)
21:51:26 <DanielK_WMDE_> one request to everyone: please review the patches and leave comments. the more i can do before the hackathon, the more we can get done at the hackathon.
21:51:40 <Scott_WUaS> :)
21:51:49 <DanielK_WMDE_> by "the patches" i mean the ones linked on https://phabricator.wikimedia.org/T124792
21:52:39 <DanielK_WMDE_> cool. so, now we have 5 minutes to go through the backlog and add #info and #action tags :D
21:52:49 <DanielK_WMDE_> any favorite lines to #info?
21:53:28 <gwicke> honestly, I find writing 1-2 paragraphs as a tl;dr on the task more useful
21:53:44 <DanielK_WMDE_> gwicke: go ahead, then :P
21:53:55 <gwicke> heh ;)
21:54:09 <gwicke> I missed half of the discussion, so that wouldn't be very accurate
21:54:19 <gwicke> any volunteers?
21:54:41 <Scott_WUaS> to do what specifically, gwicke?
21:54:43 <DanielK_WMDE_> i'll summarize tomorrow, one way or the other
21:55:08 <gwicke> DanielK_WMDE_: I could include a line or two in the update today
21:55:21 <DanielK_WMDE_> gwicke: that sounds good
21:55:48 <gwicke> please write them ;)
21:56:11 <DanielK_WMDE_> so my main take-aways for today is that i seem to be on the right track.
21:56:55 <DanielK_WMDE_> some experimentation/investigation is needed for resetting services after the init phase
21:57:10 <gwicke> "I think the real proof of this is going to be more code though."
21:57:46 <DanielK_WMDE_> gwicke: that's always true. i'd appreciate some help with that, though. would be nice if people could just play with the code, and try what it can or can't do
21:58:00 <DanielK_WMDE_> what issues they run into
21:58:08 <gwicke> yeah, could say that
21:58:33 <DanielK_WMDE_> matt_flaschen: would you be interested in a code experiment which replaces Pimple with MediaWikiServices?
21:58:41 <gwicke> General support, but a desire to write more code with it before making a final call.
21:58:43 <DanielK_WMDE_> just to see how it goes, if it's nice to use, and if anything is missing
21:58:58 <DanielK_WMDE_> gwicke: "more code" is a bit unspecific though
21:59:07 <DanielK_WMDE_> i'd be greatful for concrete suggestions
21:59:?? <gwicke> Tentative working group forming, aiming to discuss at Jerusalem Hackathon.
21:59:?? <DanielK_WMDE_> Anomie asked be to showcase refactoring LoadBalancer and friends. so i did - and learned a lot in the process.
21:59:?? <bd808> DanielK_WMDE_: what are the blockers to landing the main patch to introduce the components? If it is softly introduced into core it may be easier to hammer out some of the edge details.
21:59:?? <matt_flaschen> DanielK_WMDE_, it needs to be done, but I don't know if I'll be able to get to it. I can at least help with code review and troubleshooting, though.
22:00:?? <DanielK_WMDE_> bd808: the blocker is "someone has to review and merge it"
22:00:?? <bd808> classic problem :)
22:00:?? <DanielK_WMDE_> :P
22:00:?? <gwicke> https://etherpad.wikimedia.org/p/di-summary
22:01:?? <DanielK_WMDE_> ok, thanks everyone! this was very helpful!
22:01:?? <DanielK_WMDE_> #endmeeting
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits