| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2018-12-13 | |||
| 23:37:12 | kmalloc | there we go | |
| 23:39:17 | kmalloc | hmm. this is why i usually use settr() instead of = for setting values, especially on python structures. | |
| 23:45:07 | ianw | so the original decorator wrapped the function, which is actually in this case a bound method | |
| 23:45:16 | kmalloc | right. | |
| 23:45:33 | kmalloc | which means that __get__ is looking at .. i think something else here. | |
| 23:45:33 | ianw | now it uses "decorate" | |
| 23:45:52 | kmalloc | yeah, i'm not super familiar with decoratte, that might eat things. | |
| 23:46:01 | ianw | i think the __get__ is right -- that's returning the bound method | |
| 23:46:11 | ianw | e.g. if we're calling self.list_users() | |
| 23:46:19 | ianw | it's the list_users() method of self | |
| 23:46:50 | kmalloc | right. but we're failing when it comes to doing the .set = set_ | |
| 23:47:12 | ianw | then it's essentially calling "cache_on_arguments(self.list_users)" | |
| 23:47:13 | kmalloc | so, with decorate module we might be getting something else back from the __get__ that is resulting on the failure. | |
| 23:47:35 | ianw | well it hasn't got to that point yet | |
| 23:47:47 | ianw | it's assuming it can set "func.set = ... something" | |
| 23:47:50 | kmalloc | the traceback i see is | |
| 23:47:52 | kmalloc | https://www.irccloud.com/pastebin/Dd2zuRog/ | |
| 23:47:54 | ianw | which you can't do on a bound method | |
| 23:48:02 | kmalloc | right. | |
| 23:48:19 | kmalloc | hmm.. i wonder. | |
| 23:48:27 | ianw | ok, we're talking about the same traceback at least :) | |
| 23:49:09 | ianw | splitting it up helps i think | |
| 23:49:11 | ianw | http://paste.openstack.org/ | |
| 23:49:23 | kmalloc | yeah | |
| 23:50:04 | kmalloc | i think i see what is going on | |
| 23:50:14 | ianw | in the old decorator, it would use @wraps(fn) on the incoming user function | |
| 23:50:25 | kmalloc | yes | |
| 23:50:48 | kmalloc | dogpile makes the broad assumption it is doing the wrapping | |
| 23:51:03 | kmalloc | not getting magic instantiated things | |
| 23:51:06 | kmalloc | bound methods. | |
| 23:51:17 | ianw | yes ... probably a fair assumption :) | |
| 23:51:30 | kmalloc | we're probably doing it wrong. | |
| 23:52:37 | ianw | sorry i've got to step out for about 30 minutes for a meeting. i'll be back | |
| 23:52:41 | kmalloc | yeah np. | |
| 23:54:54 | kmalloc | ianw: the solutions are pretty straightforward (cc Shrews, mordred): 1) dogpile reverts, 2) we stop wrapping on-demand, and we lean on something similar to a "should_cache_fn", or always wrap and just lean on the null backend by default, 3) same as 2, but we use should_cache_fn to control if caching is enabled. | |
| 23:55:19 | kmalloc | i think dogpile is probably making the sane assumption it should be allowed to control wrapping. | |
| 23:56:11 | kmalloc | i think we should just always wrap, and let dogpile do that, instead of wrap the bound method. | |
| #openstack-sdks - 2018-12-14 | |||
| 00:06:09 | kmalloc | Shrews: out of curiosity, why do we have our own ... on-demand cache-on-arguments that does it independantly of import time? this seems odd... is it just to override the self-being-needed in the .invalidate? | |
| 00:31:58 | ianw | https://review.openstack.org/#/c/171456/ is where it seems to have come in | |
| 03:15:44 | ianw | I have updated https://storyboard.openstack.org/#!/story/2004605 and have a small test-case in an upstream issue https://github.com/sqlalchemy/dogpile.cache/issues/140 | |
| 03:22:15 | kmalloc | weird... | |
| 03:22:24 | kmalloc | this is just odd | |
| 03:25:25 | kmalloc | ianw: i can't log into github atm, or i'd comment on your issue | |
| 03:25:39 | kmalloc | ianw: i think dogpile is doing the sane thing and openstacksdk is doing something very weird. | |
| 03:26:31 | kmalloc | ianw: i know it's a behavior change but i wonder why we didn't just raise up the invalidate instead of adding another layer of wrapping | |
| 03:29:09 | ianw | kmalloc: at this point i dunno, i'm going a bit cross-eyed from staring at it :) gonna switch gears to look at the pin, because we kind of need that to unblock other stuff | |
| 03:29:39 | ianw | we have a circular dependency in fixing nodepool jobs | |
| 03:34:49 | openstackgerrit | Ian Wienand proposed openstack/openstacksdk master: Avoid dogpile.cache 0.7.0 https://review.openstack.org/624485 | |
| 03:37:45 | openstackgerrit | Ian Wienand proposed openstack/openstacksdk master: Avoid dogpile.cache 0.7.0 https://review.openstack.org/624485 | |
| 03:57:09 | kmalloc | ianw: ++ | |
| 03:57:21 | kmalloc | ianw: i'll poke at mordred and Shrews i'm sure they know | |
| 12:53:21 | openstackgerrit | melissaml proposed openstack/openstacksdk master: Change openstack-dev to openstack-discuss https://review.openstack.org/625247 | |
| 13:52:17 | Shrews | kmalloc: i think the intent was to have an invalidate function (i don't know the history of dogpile.cache here, but latest code seems to always have an invalidate func) and to eventually support per-resource caches | |
| 13:53:09 | Shrews | kmalloc: i'm not familiar enough with dogpile to know how to properly get rid of our custom decorator and have things still work | |
| 13:57:05 | Shrews | kmalloc: i tried https://review.openstack.org/624767 but I can't remember what on earth i saw wrong with that to cause me to abandon it. i think it was not being able to support per-resource caches | |
| 13:57:22 | Shrews | which we don't appear to be doing anyway | |
| 13:57:59 | Shrews | my head was so confused trying to unravel this :/ | |
| 14:39:27 | kmalloc | Shrews: it's always had an invalidate function | |
| 14:39:33 | kmalloc | Shrews: as long as we've been using it | |
| 14:39:45 | kmalloc | Shrews: it does require passing the "Self" to the invalidate explicitly | |
| 14:40:14 | kmalloc | Shrews: *shrug*. i think the quick fix is unwrap the extra layer of wrapping | |
| 14:40:41 | kmalloc | Shrews: brie has surgery this morning, but this afternoon i should be able to spin up and chase down a fix that undoes the wrapping | |
| 14:41:14 | Shrews | kmalloc: then i'm not sure what the purpose of https://review.openstack.org/171456 was then. i can't remember that far back | |
| 14:41:22 | kmalloc | yeah, maybe mordred knows | |
| 14:41:28 | Shrews | kmalloc: k, thx for helping | |
| 14:41:50 | kmalloc | Shrews: also i could see a order of operation issue with configuring the dogpile cache region | |
| 14:42:13 | Shrews | unpossible that there are more bugs | |
| 14:42:23 | kmalloc | well i mean with unwrapping it :P | |
| 14:42:38 | kmalloc | sorry it's super early here and i've been up 3 times to walk the dog already | |
| 14:42:45 | Shrews | oh! that's what was wrong with my attempt to unwrap it | |
| 14:42:53 | kmalloc | :) | |
| 14:42:54 | Shrews | i didn't have the region to make the wrapper call | |
| 14:42:58 | kmalloc | AHAH | |
| 14:43:21 | kmalloc | oslo_cache has a lot of mechanisms to make sure we have a region. | |
| 14:44:14 | kmalloc | anyway, i can help chase this down after brie is back from the dr. and she's settled sleeping off the anesthesia | |
| 14:44:22 | Shrews | ++ | |
| 14:44:58 | kmalloc | Shrews: also.. "YAY YEARLY COMPLIANCE TRAINING" | |
| 14:45:01 | kmalloc | :P | |
| 15:37:49 | kmalloc | mordred: ^ cc (so you know i haven't forgotten it) :P | |
| 15:38:11 | kmalloc | Shrews: i'll be back sometime post noon pacific | |
| 15:38:24 | kmalloc | Shrews: so i'll get to work on SDK things unless someone else has hit it first. | |
| 18:57:55 | kmalloc | mordred: ^ do you know the reasoning for the original extra layer wrap for cache_on_arguments ? | |
| 18:58:18 | kmalloc | mordred: it seems superfluous as it just re-implements invalidate, which was already added? | |
| 18:58:26 | kmalloc | By dogpile* | |
| 18:59:26 | kmalloc | It's the source of the issue and exception because dogpile now expects to own the wrapping, won't work with bound methods (as method.xxx = thing doesn't work on bound methods) | |
| 18:59:37 | kmalloc | Nor would setattr | |
| 21:45:24 | kmalloc | Shrews: ok... so part of the issue with dogpile is the chicken-egg issue. we're dynamically building on instantiation of the mixin the dogpile region | |
| 21:46:05 | kmalloc | that is... something we can do on import. we'll just need to run the configuration at __init__ time for the region(s) | |
| 21:48:47 | kmalloc | Shrews: oh gah. the per-resource cache is ... the problem. | |
| 21:48:58 | kmalloc | maybe we can do it with a metaclass? | |
| 21:52:53 | slaweq | mordred: hi | |
| 21:53:04 | slaweq | mordred: I just opened story https://storyboard.openstack.org/#!/story/2004618 for sdk and osprofiler | |
| 21:53:36 | slaweq | mordred: please take a look, maybe You will have some quick idea how we can solve this issue - I don't know this parts too much TBH :) | |
| 22:19:10 | kmalloc | Shrews: unfortunately the whole "undecorate" for ease of debugging is also part of the issue. | |
| 22:19:55 | kmalloc | i'm trying to make this work with a metaclass, but since connection already has a meta, this is going to be weird... i need to look into the MRO and class traversal with super() in the cases of metaclasses. | |
| 22:20:20 | kmalloc | really we should stop trying to be extra dynamic and just build cache regions that make sense | |
| 22:21:00 | kmalloc | we can dynamically configure them at __init__ time with values, but the whole "only make some cache regions and decorate/undecorate" is way overly complex. | |
| 22:21:03 | kmalloc | mordred: ^ cc | |
| 22:25:39 | kmalloc | aha, i think i found the simplest solution... | |
| 22:52:31 | openstackgerrit | Morgan Fainberg proposed openstack/openstacksdk master: Fix dogpile.cache 0.7.0 interaction https://review.openstack.org/625370 | |