Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-07-28
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 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:00 whoami-rajat sean-k-mooney, ah, does disabling it has any side effect?
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
11:40:39 stephenfin Maybe drop the type hints or put them in a separate patch in that case? :)
11:41:00 sean-k-mooney well we had this debate a few versions ago
11:41:35 sean-k-mooney that we did not want to have to write worse code with out type hint jsut because we wanted to backport
11:41:50 stephenfin fair point
11:42:27 sean-k-mooney so im hesitent to set that precident as if i do ill be asked to do it for every bugfix
11:43:14 sean-k-mooney at which point its much less motivating to ever write type hints and i used them while writing the patch a few times so they are really useful IMO
11:43:37 sean-k-mooney e.g. i forgot what i was passing an looked at the type hint
11:56:00 sean-k-mooney stephenfin: by the way im not going to do it in this patch but what are your feeling on doing ty.Dict vs importing Dict directly and just using it
11:56:12 sean-k-mooney stephenfin: you seamed to be doing ty.<whatever>
11:56:29 sean-k-mooney so that is what im doing but the ty. gets a little tedious after a while
11:57:27 sean-k-mooney most of the examples i have seen do "from typeing import Dict,List,..."
11:58:18 sean-k-mooney long term do you think we should stick with import typeing as ty or move over to the other from typing style
12:46:34 lyarwood stephenfin: so I've actually managed to hit the same issue with live_migration as pre_live_migration in my func tests btw, looks like I also need the fix to cover this case
12:48:20 lyarwood oh wait I borked the test
12:48:30 lyarwood OS_DEBUG++
12:53:09 sean-k-mooney OS_DEBUG is for what disabling monkeypatching or extra output
12:53:22 sean-k-mooney its the latter right?
12:54:08 sean-k-mooney when running test with tox
12:54:13 lyarwood latter, had a typo in a mock and was confused until I saw the extra output
12:54:36 sean-k-mooney ya i have used it once or twice
12:54:48 sean-k-mooney i normally got to pdb instead
13:00:19 jsuchome lyarwood: https://review.opendev.org/#/c/743220/ is green, hooray!
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: Add regression tests for bug #1889108 https://review.opendev.org/743289
13:22:52 openstack bug 1889108 in OpenStack Compute (nova) "failures during driver.pre_live_migration remove source attachments during rollback" [High,In progress] https://launchpad.net/bugs/1889108 - Assigned to Lee Yarwood (lyarwood)
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/743319
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: func: Add live migration rollback volume attachment tests https://review.opendev.org/743534
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: func: Add CinderFixture to _IntegratedTestBase https://review.opendev.org/743535
13:23:34 lyarwood jsuchome: ack, I'll sort the actual test for that out later this eveningn
13:32:55 jsuchome thanks
13:41:36 openstackgerrit Alexandre Arents proposed openstack/nova master: Make _rebase_with_qemu_img() generic https://review.opendev.org/743537

Earlier   Later