| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-06-27 | |||
| 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 | gibi | bauzas pointed out that the sentinel + kwargs is widely used in the nova rpc apis | |
| 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: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 | |
| 12:00:08 | sean-k-mooney | that actully a error in any path where new_az is not _sentinel | |
| 12:00:36 | bauzas | well, the reference to _sentinel is unique | |
| 12:00:49 | bauzas | you can check its id | |
| 12:01:21 | bauzas | that's why this pattern exists in Python to verify whether this parameter was called | |