19:15:08 <bcoca> #startmeeting ansible core irc public meeting
19:15:09 <zodbot> Meeting started Tue Jan  7 19:15:08 2020 UTC.
19:15:09 <zodbot> This meeting is logged and archived in a public location.
19:15:09 <zodbot> The chair is bcoca. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:15:09 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:15:09 <zodbot> 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 <bcoca> hmm .. no tickets?
19:15:52 <bcoca> so no issues?
19:16:00 <nitzmahone> everything is awesome (TM)
19:16:06 <bcoca> #topic open floor
19:16:07 <sdoran> Nothing to see here.
19:16:11 <bcoca> @nitzmahone dream on
19:16:16 <nitzmahone> lol
19:16:30 <sdoran> 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 <sdoran> I need to add tags, sorry.
19:16:57 <bcoca> he, im in middle of building new ticket  ..
19:17:00 <sdoran> s/tags/labels
19:18:13 <bcoca> so requesteros either gave up or not aware we restarted meetings, postponing those issues till thrusday
19:18:33 <agaffney> I've got a PR that I'd like some feedback on. give me a minute to find it
19:18:59 <bcoca> and if no one has any reqeusts i'll end meeting in 5
19:19:01 <agaffney> https://github.com/ansible/ansible/pull/66102
19:20:28 <bcoca> ^ nitzmahone VarWithSources should be close enough to your wrapper for deprecations
19:20:58 <bcoca> 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 <nitzmahone> 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 <agaffney> 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 <bcoca> 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 <agaffney> nitzmahone: you didn't run into any issues with JSON encoding/decoding when using a var proxy class for bool?
19:23:26 <agaffney> I ran into all sorts of them in https://github.com/ansible/ansible/pull/41811
19:23:31 <bcoca> nitzmahone: did you create PR , even WIP would be nice to point at
19:24:13 <nitzmahone> 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 <bcoca> external dep ?
19:25:21 <nitzmahone> Well, we could vendor it- the part we need is very small
19:25:23 * sdoran was thinking the same thing
19:25:33 <nitzmahone> Similar in size to six
19:25:35 <sdoran> <nod>
19:26:12 <bcoca> 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 <agaffney> the issues I ran into weren't obvious until I ran it through CI
19:26:31 <nitzmahone> 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 <bcoca> then we should try to go for dep, C extension makes huge diff in yaml case
19:27:29 <bcoca> but testing would confirm that for this case
19:27:54 <bcoca> 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 <bcoca> and he seems motivated to push this forward
19:28:38 <agaffney> don't count on my motivation :)
19:29:08 <bcoca> i see you scratching, i infer itch
19:29:15 <nitzmahone> 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 <agaffney> the itch is often boredom
19:29:29 <bcoca> per var per host
19:29:49 * nitzmahone thought it was per container, but just skimming the PR
19:29:51 <bcoca> well, per var  x scope, host is really only muliplier, other vars are constant
19:30:13 <bcoca> er .. s/cconstant/limited to one global/
19:30:17 <agaffney> it's per var *after* accounting for scope, since it's only applied after merging
19:30:58 <bcoca> we do get_vars at diff scopes, play/task/host
19:31:03 <nitzmahone> *sigh* so many optimizations we could make there, so little time
19:31:05 <bcoca> so it will create copies for each
19:31:09 <bcoca> nitzmahone: agreed
19:31:21 <bcoca> i really want to just build a 'data server' that the template engine queries
19:31:30 <bcoca> stop making/passing 1000s of copies around
19:31:41 <agaffney> 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 <nitzmahone> I want varmgr to hold nested immutable matryoshka
19:32:05 <bcoca> agaffney: meth addicted psycotic dragons with rabies
19:32:20 <agaffney> ah, florida dragons...
19:32:31 <bcoca> nitzmahone: it currently holds nested mutable recursive matryoshka siamise twins
19:32:48 <nitzmahone> bcoca: (that were placed in a blender ;) )
19:33:05 <bcoca> get_vars = blender
19:33:54 <agaffney> and not even a good blender
19:34:09 <bcoca> nope, leaves big chunks
19:34:10 <nitzmahone> 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 <bcoca> nitzmahone: agree on 'off by default', this would be nice with ANSIBLE_DEBUG=1
19:34:47 <bcoca> and 'tech preview' label
19:35:02 <nitzmahone> 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 <agaffney> no matter how variable source is tracked, what I put together is a reasonable way to present the information
19:36:00 <agaffney> 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 <bcoca> see  adrian's PR  that does 'full tracking' .. it works as most people would want, but performance was just horrible
19:38:06 <agaffney> this approach is probably a good middle ground for performance, even if it's not hidden behind ANSIBLE_DEBUG=1
19:38:13 <agaffney> even if it's not "complete"
19:38:35 <agaffney> wrapping/proxying every var is going to have a lot of overhead
19:39:26 <bcoca> https://github.com/ansible/ansible/pull/28477
19:41:14 <bcoca> 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 <nitzmahone> 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 <sdoran> +1
19:43:59 <nitzmahone> (on part b of what bcoca said)
19:44:45 <sdoran> I'm for the feature but agree it should be off by default.
19:45:58 <bcoca> maybe define 2 diff 'combine_vars/get_vars' on debug true/false
19:46:05 <bcoca> probably easiest way to keep rest of code same
19:46:30 <nitzmahone> or just have it ignore the value instead of shoving it in (avoids the conditional dispatch at the call site)
19:47:07 <agaffney> 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 <bcoca> but still creates overhead of wrapper
19:47:22 <nitzmahone> nope
19:47:41 <bcoca> ?
19:48:16 <nitzmahone> 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 <bcoca> 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 <bcoca> when disabled
19:48:39 <nitzmahone> Yeah, so long as the "off" case has no appreciable overhead, I'm +1
19:49:04 <bcoca> +0.5 .. would really prefer the fuller feature, but i'll take what i can get
19:49:35 <agaffney> 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 <bcoca> if you avoid new code by default, there should be no diff
19:50:04 <bcoca> only use wrapper and pass data on debug
19:50:17 <bcoca> why i suggested redefining the functions as 'least intrusive' way
19:50:28 <bcoca> but there are 20+ ways to implement
19:51:50 <nitzmahone> 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 <nitzmahone> (that runs when the feature is disabled)
19:52:28 <agaffney> 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 <nitzmahone> So if you swap that VarsWithSources dict for a regular dict when the feature is off, it's "free"
19:53:01 <nitzmahone> I'd just make combine take the new arg and ignore it if the feature's off
19:53:46 <nitzmahone> You could do things like monkeypatching/impl-swapping, but ... bleh
19:54:12 <nitzmahone> (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 <bcoca> k, going to end meeting here, we can discuss more details in devel
19:55:29 <bcoca> thanks all for attending
19:55:32 <bcoca> #endmeeting