19:15:08 #startmeeting ansible core irc public meeting 19:15:09 Meeting started Tue Jan 7 19:15:08 2020 UTC. 19:15:09 This meeting is logged and archived in a public location. 19:15:09 The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:15:09 Useful Commands: #action #agreed #halp #info #idea #link #topic. 19:15:09 The meeting name has been set to 'ansible_core_irc_public_meeting' 19:15:26 * nitzmahone lurks between meetings 19:15:37 * sdoran waves 19:15:48 hmm .. no tickets? 19:15:52 so no issues? 19:16:00 everything is awesome (TM) 19:16:06 #topic open floor 19:16:07 Nothing to see here. 19:16:11 @nitzmahone dream on 19:16:16 lol 19:16:30 There's a few items on the agenda. I just made a January 2020 ticket: https://github.com/ansible/community/issues/516 19:16:39 I need to add tags, sorry. 19:16:57 he, im in middle of building new ticket .. 19:17:00 s/tags/labels 19:18:13 so requesteros either gave up or not aware we restarted meetings, postponing those issues till thrusday 19:18:33 I've got a PR that I'd like some feedback on. give me a minute to find it 19:18:59 and if no one has any reqeusts i'll end meeting in 5 19:19:01 https://github.com/ansible/ansible/pull/66102 19:20:28 ^ nitzmahone VarWithSources should be close enough to your wrapper for deprecations 19:20:58 agaffney: i understand the want, but i think the first thing people will ask is actual source, the precedence level is not enough in most cases when you are trying to find out where it was defined 19:21:12 Yeah, I had var provenance tracking as one of the "other potential uses", and at least in my testing, it was working fine with bool/int in JSON encode/decode 19:21:59 bcoca: it's definitely a bit of a hack, but even providing this much information if quite useful, and extending it could be done later 19:22:37 i dont disagree, i would just not use as default, pretty sure the performance impact will become steep once the data starts growing 19:23:03 nitzmahone: you didn't run into any issues with JSON encoding/decoding when using a var proxy class for bool? 19:23:26 I ran into all sorts of them in https://github.com/ansible/ansible/pull/41811 19:23:31 nitzmahone: did you create PR , even WIP would be nice to point at 19:24:13 It's been a few months since I played with it, but I specifically tested that case- I haven't created a PR, would have to see if I can even find the branches, but I was using wrapt instead of a hand-rolled proxy, which has extremely high type fidelity 19:25:00 external dep ? 19:25:21 Well, we could vendor it- the part we need is very small 19:25:23 * sdoran was thinking the same thing 19:25:33 Similar in size to six 19:25:35 19:26:12 fine, just needs to be a consideration, though i see it most repos i checked from older versions, so might be fine as dep also (someoene check RHEL5?) 19:26:13 the issues I ran into weren't obvious until I ran it through CI 19:26:31 There's an optional C extension that makes parts of it a lot faster, but I haven't done performance testing on a large enough in-circuit version to know if it'd matter for the parts we're using (I think most of the C extension gains are for wrapping methods/functions) 19:27:06 then we should try to go for dep, C extension makes huge diff in yaml case 19:27:29 but testing would confirm that for this case 19:27:54 nitzmahone: if you can push WIP pr, im pretty sure it would give agaffney something to poke at and continue since you probably wont have time 19:28:06 and he seems motivated to push this forward 19:28:38 don't count on my motivation :) 19:29:08 i see you scratching, i infer itch 19:29:15 If I'm skimming the PR correctly, I think the data size concerns here are pretty minor, since it's only one string annotation per host, but I might be reading it wrong 19:29:26 the itch is often boredom 19:29:29 per var per host 19:29:49 * nitzmahone thought it was per container, but just skimming the PR 19:29:51 well, per var x scope, host is really only muliplier, other vars are constant 19:30:13 er .. s/cconstant/limited to one global/ 19:30:17 it's per var *after* accounting for scope, since it's only applied after merging 19:30:58 we do get_vars at diff scopes, play/task/host 19:31:03 *sigh* so many optimizations we could make there, so little time 19:31:05 so it will create copies for each 19:31:09 nitzmahone: agreed 19:31:21 i really want to just build a 'data server' that the template engine queries 19:31:30 stop making/passing 1000s of copies around 19:31:41 what's in this PR is definitely just a first pass. however, doing more starts digging into VarsManager innards, and there be dragons and such 19:31:52 I want varmgr to hold nested immutable matryoshka 19:32:05 agaffney: meth addicted psycotic dragons with rabies 19:32:20 ah, florida dragons... 19:32:31 nitzmahone: it currently holds nested mutable recursive matryoshka siamise twins 19:32:48 bcoca: (that were placed in a blender ;) ) 19:33:05 get_vars = blender 19:33:54 and not even a good blender 19:34:09 nope, leaves big chunks 19:34:10 IMO if we did this, it needs to be off by default, and probably place some caution tape around the implementation and access saying we don't guarantee it won't go away or change 19:34:27 nitzmahone: agree on 'off by default', this would be nice with ANSIBLE_DEBUG=1 19:34:47 and 'tech preview' label 19:35:02 I def see the need for it (and a lot more), esp when you start thinking about more interactive debugging scenarios, which is what worries me about us getting boxed into a corner 19:35:05 no matter how variable source is tracked, what I put together is a reasonable way to present the information 19:36:00 the VarsWithSources class works in all cases that it needs to and the debug messages show up when any var is accessed from it 19:36:40 see adrian's PR that does 'full tracking' .. it works as most people would want, but performance was just horrible 19:38:06 this approach is probably a good middle ground for performance, even if it's not hidden behind ANSIBLE_DEBUG=1 19:38:13 even if it's not "complete" 19:38:35 wrapping/proxying every var is going to have a lot of overhead 19:39:26 https://github.com/ansible/ansible/pull/28477 19:41:14 again, not against the feature but a) would like the fuller implementation, but wont stop this waiting for the 'perfect' b) would put behind debug toggle so it wont affect 'normal' performance 19:42:34 Ditto- since the access is just "dump only" (ie you're not really defining any new user-visible API), there's a lot less worry about changing it later 19:43:55 +1 19:43:59 (on part b of what bcoca said) 19:44:45 I'm for the feature but agree it should be off by default. 19:45:58 maybe define 2 diff 'combine_vars/get_vars' on debug true/false 19:46:05 probably easiest way to keep rest of code same 19:46:30 or just have it ignore the value instead of shoving it in (avoids the conditional dispatch at the call site) 19:47:07 it would be easiest to just make the tracking part of combine_and_track() optional, as well as the use of the VarsWithSources class 19:47:07 but still creates overhead of wrapper 19:47:22 nope 19:47:41 ? 19:48:16 oh, you mean the dict wrapper? Could also just conditionally create that instead of a regular dict depending if the feature is on or not 19:48:21 in any case, that is implentation detail you can work out, i would just be happy to avoid overhead for this option 19:48:32 when disabled 19:48:39 Yeah, so long as the "off" case has no appreciable overhead, I'm +1 19:49:04 +0.5 .. would really prefer the fuller feature, but i'll take what i can get 19:49:35 I can wrap some of these bits in a check for debugging being enabled. I'm not sure how to measure the performance differences, though 19:49:56 if you avoid new code by default, there should be no diff 19:50:04 only use wrapper and pass data on debug 19:50:17 why i suggested redefining the functions as 'least intrusive' way 19:50:28 but there are 20+ ways to implement 19:51:50 Basically so long as there's no conditional code that's in a "hot" code path (which the vars dict definitely is), you should be good 19:52:03 (that runs when the feature is disabled) 19:52:28 making the VarsWithSources class optional is really easy. making _combine_and_track() optional isn't quite as easy, but still fairly easy 19:52:31 So if you swap that VarsWithSources dict for a regular dict when the feature is off, it's "free" 19:53:01 I'd just make combine take the new arg and ignore it if the feature's off 19:53:46 You could do things like monkeypatching/impl-swapping, but ... bleh 19:54:12 (then there's still two impls or nested combine impls, which, bleh) 19:54:38 * nitzmahone has to duck for WWG in a minute 19:55:23 k, going to end meeting here, we can discuss more details in devel 19:55:29 thanks all for attending 19:55:32 #endmeeting