Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-06-27
09:54:35 gibi on the test side, I agree, that could be factored out
09:54:53 sean-k-mooney just stypictlly i like constnats to eb UPPERCASE_WITH_UNDERSCORES
09:55:10 sean-k-mooney *stylisticly
09:56:50 sean-k-mooney Uggla: so realisticaly you do not need to change anything it just not an established pattern in our code base to have sentenial like this so it looks odd because its different then our normal pattern
09:57:28 gibi bauzas: ^^ look sean-k-mooney also feels it is not an established pattern :D
09:57:59 gibi sean-k-mooney: context: we had a bit of back and forth with bauzas around the sentinel
09:58:36 sean-k-mooney we dont currntly have the equivalent of std:optional in python/nova
09:59:10 sean-k-mooney i would kind of perfer to have that class but i know other recoile at c++ conventions
09:59:10 gibi bauzas pointed out that the sentinel + kwargs is widely used in the nova rpc apis
09:59:31 sean-k-mooney we use none as the sentinal there no?
10:00:31 sean-k-mooney i dont really like that the custom sentinal we use is not "truthy" so we can just use if directly
10:00:46 gibi I think we use kwargs not to send a value even
10:02:58 sean-k-mooney we use None however as the sentinal https://github.com/openstack/nova/blob/c53ec4e48884235566962bc934cbf292ad5b67b8/nova/compute/manager.py#L11017-L11019=
10:03:11 sean-k-mooney so what Uggla is doing is very differnt
10:04:31 sean-k-mooney the use of None as a sential for kwargs is idiomatic python
10:04:33 Uggla sean-k-mooney, fyi bauzas provided this as a sentinel example: https://github.com/openstack/nova/blob/master/nova/scheduler/rpcapi.py#L152 and https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L145 so I try to respect that pattern.
10:05:37 sean-k-mooney i see
10:05:42 sean-k-mooney i still dont like that
10:05:47 Uggla :)
10:05:53 sean-k-mooney its kind of like raw gotos
10:06:05 sean-k-mooney we have if while ectra for a reason
10:06:42 gibi we cannot use None in the current case as None has a different meaning than the missing param
10:07:17 gibi as we want to be able to signal either unpin or no-change
10:07:20 sean-k-mooney yes im aware
10:07:32 gibi so we needed an extra value
10:07:49 sean-k-mooney yep that is what std:optional does in c++
10:08:00 sean-k-mooney it give you a value that is outside the normal set
10:08:03 sean-k-mooney same with ovo
10:08:11 sean-k-mooney the field have an addtional unset value
10:08:37 gibi yeah, I don't like the ovo way of missing fields, as that breaks the class invariant for me
10:08:56 sean-k-mooney well i prefer it to this
10:09:13 sean-k-mooney the ovo way at least work with in and is encapulated
10:10:04 gibi I create an ovo class with predefined fields, and then I have to check at each access if the field exists, that seem against the fact that we predefine the fields
10:10:25 sean-k-mooney i dont like that we have to rememeber that this is not just an az its a union of an az and a sentinal
10:10:26 gibi anyhow I go back to review the unshelve patch
10:10:51 gibi sean-k-mooney: so you would add an extra param to the call instead? like az_provided ?
10:11:01 gibi that way we can avoid the sentinel
10:11:31 gibi new_az can be None to unpin or a valid AZ name, and az_provided boolean can be set if no AZ field is provided in the REST request
10:11:36 sean-k-mooney perhaps or wrap the parmters in a class
10:12:22 sean-k-mooney i would think that az_provided would be more readable
10:13:07 sean-k-mooney so ideally even if we had a sentinil i would like use to do that test once at the top of the function and defien a local az_provided varible
10:13:27 sean-k-mooney that better expresses the intent
11:37:58 opendevreview Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126
11:40:36 bauzas Uggla: sean-k-mooney: gibi: sorry folks, I was cooking and lunching with my kid
11:42:00 bauzas sean-k-mooney: not sure I understand why you unlike the sentinel pattern
11:42:13 bauzas sean-k-mooney: and why you ask for a new parameter
11:42:29 sean-k-mooney bauzas: its very very error prone
11:42:38 bauzas ?
11:42:49 sean-k-mooney you need to keep the context that the vraiable is a union of a sential and the az
11:42:55 sean-k-mooney and everywhere you use it test for both
11:43:15 sean-k-mooney its very easy to miss that
11:43:17 bauzas sean-k-mooney: not if you provide a doc
11:43:24 sean-k-mooney no one reads docs
11:43:31 bauzas a doc comment I mean
11:43:36 bauzas like I said
11:43:47 bauzas :param az: (optional) blah
11:43:52 sean-k-mooney yep i hate those
11:44:29 bauzas and so, you prefer to have pypi parameters...
11:44:33 sean-k-mooney if we have a sential i would prefer use to do the test once at the top of the function and never refernce the centinal again
11:44:53 sean-k-mooney well thos are at least testable by mypy so yes
11:44:59 sean-k-mooney but that is missign the point
11:45:26 bauzas sean-k-mooney: well, if we have two tests, one for verifying a sepific value for new_az and another one for verifying to not use the parameter, I wouldn't see why it could be a problem
11:45:30 sean-k-mooney you are adding extra context that you need to keep in mind
11:45:49 bauzas sean-k-mooney: but we have done that since I'm working in Nova
11:46:01 sean-k-mooney in very limits places
11:46:16 sean-k-mooney i really dont want o see us doing this more in the code
11:46:17 bauzas lemme look at Uggla's change
11:49:01 bauzas sean-k-mooney: in general within RPC methods
11:49:18 bauzas so, I briefly looked at Uggla's code
11:49:47 bauzas I don't see why people wouldn't understand this is an optional parameter when looking at the call https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/api/openstack/compute/shelve.py#106
11:50:07 bauzas and in https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/compute/api.py#4467 Uggla added docstrings
11:51:33 bauzas but I understand your point
11:51:40 bauzas maybe we could use PEP661 then https://peps.python.org/pep-0661/
11:52:21 bauzas and using the sentinels module
11:52:39 bauzas lemme see if py3.8 supports it
11:53:09 bauzas mmm, not stdlib
11:53:46 bauzas oh my bad, PEP661 is a drafy
11:53:50 bauzas draft*
11:54:19 sean-k-mooney bauzas: its not the call site
11:54:36 sean-k-mooney its the use site within the function i find distasteful
11:54:58 bauzas you mean, the caller or the called method ?
11:55:07 sean-k-mooney the use in the called method
11:55:14 bauzas the if conditional ?
11:55:18 sean-k-mooney yes
11:55:21 bauzas hah
11:55:31 sean-k-mooney the fact you have to check the sential with an is check explictly
11:55:39 bauzas well, then we could have one single conditional that would set a value or not
11:55:55 sean-k-mooney right which is what i actully was askign for
11:56:09 bauzas would you then prefer something like
11:56:13 sean-k-mooney i didnt leave review feedback yet because i was in the midel of other thngs
11:56:48 sean-k-mooney bauzas: also https://peps.python.org/pep-0661/ is basically the same as cpp's std:optional
11:57:00 sean-k-mooney at least in inteded usage
11:57:05 sean-k-mooney so yes i would prefer that
11:57:36 sean-k-mooney that said i dont se how its used
11:58:13 bauzas sean-k-mooney: https://paste.opendev.org/show/bLmJ5uKGsufcYjXAIda3/
11:58:34 bauzas would you prefer this pattern ?
11:59:03 bauzas we only set an internal value if the parameter was provided
11:59:21 bauzas so we only check the sentinel value once at the top of the method
11:59:31 sean-k-mooney not quite
11:59:41 sean-k-mooney the problem with that is we can refernce undeined varbles

Earlier   Later