| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-07-28 | |||
| 10:14:34 | sean-k-mooney | or the need to add a microversions | |
| 10:14:54 | sean-k-mooney | or treat it as optional in someother way so that older novas can work with it | |
| 10:16:10 | brinzhang_ | Yes, this will change the cyborg api, we need a new microversion to support project_id in cyborg arq api | |
| 10:16:41 | sean-k-mooney | i dont see that in https://review.opendev.org/#/c/738427/2 | |
| 10:17:38 | sean-k-mooney | in factr i dont see any microversion code in cyborg | |
| 10:17:48 | sean-k-mooney | i have not looked very hard | |
| 10:17:58 | brinzhang_ | I will ask him to add microversion, we have introduced microversion in v2 API, this must be done. | |
| 10:19:42 | sean-k-mooney | it looks like it will be the first microversion https://github.com/openstack/cyborg/blob/369abe8dd06aa6648298c3256f444a63ee6268d0/cyborg/api/controllers/v2/api_version_request.py#L49 | |
| 10:20:12 | sean-k-mooney | is this the first api change since v2 | |
| 10:20:30 | brinzhang_ | I would like this patch can add the microversion too https://review.opendev.org/#/c/698190/ | |
| 10:20:47 | brinzhang_ | I am trying my best.. | |
| 10:21:11 | sean-k-mooney | brinzhang_: i know | |
| 10:21:21 | sean-k-mooney | brinzhang_: its not just your responsiblity | |
| 10:21:29 | sean-k-mooney | all cyborg cores shoudl be reviewing for this | |
| 10:21:34 | sean-k-mooney | even non cores | |
| 10:21:46 | brinzhang_ | sean-k-mooney: yes, thanks | |
| 10:21:59 | brinzhang_ | hope you can leave comment inline too | |
| 10:22:32 | brinzhang_ | In contrast, you have more experience in this area, which is worth learning from. | |
| 10:22:34 | stephenfin | lyarwood: Reviewed those, btw | |
| 10:22:47 | sean-k-mooney | yes i can i was hoping to point them to a patch that previously raised the microversion so that they had an example but since they are the first i guess i cant | |
| 10:23:39 | sean-k-mooney | well nova does i try to avoid making api change when i can. its a lot less work if you dont have to modify the api to implemente a feature | |
| 10:23:43 | sean-k-mooney | :) | |
| 10:24:05 | sean-k-mooney | ill leave a comment on both patches shortly | |
| 10:24:29 | brinzhang_ | sean-k-mooney: yeah, I can understand, but the current two patches inevitably need to modify the API. | |
| 10:25:14 | brinzhang_ | Thanks, so that I can ask again^^ | |
| 10:31:37 | sean-k-mooney | brinzhang_: basically we need to copy https://docs.openstack.org/nova/latest/contributor/microversions.html into the cyborg docs and do a find and replace | |
| 10:32:21 | sean-k-mooney | brinzhang_: gmann and alex_xu are two of the experts on api microversioning | |
| 10:32:44 | brinzhang_ | sean-k-mooney: thanks, I will review this, and do a copy later | |
| 10:33:24 | brinzhang_ | yeah, if I have some problem, Iill ask them for help | |
| 10:37:49 | lyarwood | stephenfin: ta | |
| 10:56:13 | stephenfin | sean-k-mooney: Left comments on https://review.opendev.org/#/c/739131/. I have those type hints worked out locally if you want me to push them somewhere? | |
| 10:59:01 | sean-k-mooney | oh i just fixed the ones you commented on before i proablyshould have checked all the types | |
| 10:59:24 | sean-k-mooney | on the styple nit putting each paramater on its onw line is something im happy to fight you over | |
| 10:59:35 | sean-k-mooney | i hate that more then anything else | |
| 11:00:39 | sean-k-mooney | wasting vertical space like that is deeply wrong in my view. | |
| 11:01:24 | sean-k-mooney | im assumeing that is what you want here https://review.opendev.org/#/c/739131/8/nova/virt/libvirt/driver.py@6894 | |
| 11:22:04 | stephenfin | sean-k-mooney: I think putting it on its own line helps visually group the function definition stuff neatly together. It also allows for maximum width wrt annotating the return type (which can often be quite lengthy, as you're seeing) | |
| 11:23:15 | sean-k-mooney | right but i think your thinking about type hints wrong | |
| 11:23:24 | sean-k-mooney | we should not in all cases fully qualify the type | |
| 11:23:38 | sean-k-mooney | that is not how they are ment to be used | |
| 11:23:40 | stephenfin | why ever not? | |
| 11:23:52 | sean-k-mooney | have you ever used c++ templates | |
| 11:24:10 | sean-k-mooney | and tried to fully quallify a template that multipel template args | |
| 11:24:38 | stephenfin | no, I've barely touched C++ | |
| 11:24:40 | sean-k-mooney | type hints are most useful when they specify the interface of the type you expect | |
| 11:24:57 | stephenfin | right | |
| 11:25:13 | sean-k-mooney | you can be more specific too but our brains are not compilers so adding too much detail can be harmful | |
| 11:25:58 | sean-k-mooney | if you add too much detail to the type trait and you have multiple parmater it gets hard to keep all that context in your brain | |
| 11:26:21 | stephenfin | the point of the type hints is offload the context though | |
| 11:26:36 | sean-k-mooney | but you still need to read them | |
| 11:26:40 | stephenfin | you don't need to figure out what param_a is: it's written right there | |
| 11:26:58 | sean-k-mooney | yes we agree on this | |
| 11:27:08 | sean-k-mooney | but is ty.Dict[str,ty.Union[str, ty.Dict[str, ty.Union[str, ty.List[str], None]]]] | |
| 11:27:12 | sean-k-mooney | useful? | |
| 11:27:22 | sean-k-mooney | vs ty.Dict[str,ty.Union[str, ty.Dict]] | |
| 11:27:42 | stephenfin | No, not at all. That should be a typed dict | |
| 11:27:58 | stephenfin | you'll note in most places we have that I've just used ty.Dict[str, ty.Any] | |
| 11:28:04 | stephenfin | so we're on the same page there | |
| 11:28:23 | stephenfin | however, 'list' also isn't useful | |
| 11:28:37 | sean-k-mooney | well that is actully the fully correct type | |
| 11:28:51 | stephenfin | Unless it's truly a generic container for anything, then I want to know what's in it | |
| 11:28:53 | sean-k-mooney | its technically ty.Dict[str,ty.Union[str, ty.Dict[str, ty.Union[str, ty.List[str], None]]]] | |
| 11:29:27 | stephenfin | I don't think I've written anything approaching that level of detail anywhere | |
| 11:29:34 | sean-k-mooney | ty.Dict is an alis for dict which is treated as ty.Dict[Any, Any] | |
| 11:29:44 | sean-k-mooney | you havent | |
| 11:29:46 | whoami-rajat | hi #openstack-nova , is there a way to use pdb in n-cpu service (DEVSTACK), i tried starting the service manually on terminal but it doesn't stop on the pdb | |
| 11:29:57 | stephenfin | and I'm not suggesting we do that here either | |
| 11:30:06 | sean-k-mooney | but that is what i would hav too change https://review.opendev.org/#/c/739131/8/nova/virt/libvirt/driver.py@6909 too | |
| 11:30:41 | sean-k-mooney | whoami-rajat: that is because we use eventlets | |
| 11:31:00 | stephenfin | nah, I didn't say that. I just said what was there was wrong. 'ty.Dict[str, ty.Any]' would work perfectly fine | |
| 11:31:05 | sean-k-mooney | to use it with pdb you have to disable monkeypatching of the threading api | |
| 11:31:06 | stephenfin | or ty.Dict, if you prefer | |
| 11:32:00 | whoami-rajat | sean-k-mooney, ah, does disabling it has any side effect? | |
| 11:32:00 | sean-k-mooney | right so i have been using dict which is the same as ty.Dict which is ty.Dict[ty.Any, ty.Any] | |
| 11:32:21 | sean-k-mooney | whoami-rajat: unfortunetly yes, we actullly have a couple of infinet loops | |
| 11:32:37 | sean-k-mooney | we normally only exit them via yeilding | |
| 11:32:58 | sean-k-mooney | so in practic it is quite hard to debug n-cpu directly | |
| 11:33:12 | whoami-rajat | :( , is rpdb a solution here? sean-k-mooney | |
| 11:33:28 | sean-k-mooney | i have never used that | |
| 11:33:58 | stephenfin | sean-k-mooney: Okay, then I retract my request not to use 'dict' in that case then | |
| 11:34:04 | whoami-rajat | ack. thanks for the info sean-k-mooney | |
| 11:34:20 | stephenfin | I'd still rather we did use 'ty.Dict[str, ty.Any]', since that's at least a little more information than we might otherwise have | |
| 11:34:24 | stephenfin | but it's not necessary | |
| 11:35:03 | sean-k-mooney | oh am well we can run with remote debuging but basically you have to deal with either eventlet monkey patching which means breakpoint might not work or disable monkey patch whcih means you might not get to the code you want | |
| 11:35:22 | stephenfin | I think my comments on the other places that 'ty.Any' is still used are valid though, since they're easy to type and actually helpful | |
| 11:35:28 | sean-k-mooney | stephenfin: ill go make most of the change you asked for | |
| 11:35:38 | sean-k-mooney | and then when the type get a bit too long | |
| 11:35:45 | sean-k-mooney | ill do type erasure | |
| 11:36:00 | sean-k-mooney | i can do ty.Dict[str, ty.Any] | |
| 11:36:09 | stephenfin | sounds good to me | |
| 11:36:33 | sean-k-mooney | to me by the way needing to do ty.Dict[str, ty.Any] is kind fo a code smell | |
| 11:37:06 | sean-k-mooney | either we should be returning classes rather then dicts | |
| 11:37:30 | sean-k-mooney | or we shoudl be breaking down the function as it likely has too many differnt posable return types | |
| 11:37:41 | stephenfin | totally agree | |
| 11:38:27 | sean-k-mooney | am other then typing and the comment you left | |
| 11:38:33 | sean-k-mooney | are you happy with the change other then that | |
| 11:38:56 | sean-k-mooney | i am hoping this will be the last revision so i can move on to the numa in placemnt stuff for a while | |
| 11:39:25 | stephenfin | I do think we could fold some of the functions into each other, since the separation feels a bit artificial rn, but I've said as much in the review and it's easy fix later | |
| 11:39:32 | stephenfin | so yeah, lgtm otherwise | |
| 11:40:03 | sean-k-mooney | ok im also planning to backport this at least to train so i was trying to keep this relitivly small | |