On Wed, Jan 26, 2022 at 01:58:19PM -0500, John Snow wrote: > (1) QAPI types the return of many commands as an empty object. That's > literally indeed what happens on the wire, and it makes sense in that > if these commands were ever to return anything, it is a "compatible > evolution" to include new fields in such an object. In Python, this > does not make much sense, though; as this is somewhat hard to > annotate: > > async def stop() -> Literal[{}]: ... > > The more pythonic signature is: > > async def stop() -> None: ... > > I feel like it's spiritually equivalent, but I am aware it is a > distinct choice that is being made. It could theoretically interfere > with a choice made in QAPI later to explicitly return Null. I don't > think we'd do that, but it's still a choice of abstraction that > reduces the resolution of distinct return signatures.
As you mention though, bear in mind that a command returning nothing today, might return something tomorrow. IOW, we'd be going from a empty dict to a non-empty dict. If you use "None" then it'd be gonig from None to a non-empty dict. I think you can argue both of these approaches are backwards compatible. The python app is not likely to be looking at the return type at all initially - unlike C, errors get raised as exceptions, so you don't need to look at return type to distinguish succes from failure. So I'd consider it merely a documentation problem to say that a "None" return type might later change to a dict. Dunno how you represent that in python type annotations, but I presume there's a way. > (1.5) Do we have a formal definition for what we consider to be a > "compatible evolution" of the schema? I've got a fairly good idea, but > I am not sure how it's enforced. Is it just Markus being very > thorough? If we add more languages to the generator, we probably can't > burden Markus with knowing how to protect the compatibility of every > generator. We might need more assertions for invariants in the > generator itself ... but to protect "evolution", we need points of > reference to test against. Do we have anything for this? Do we need > one? Should I write a test? It isn't enforced through any technical measures. For example just recently we accidentally broke back compat of query-sgx by deleting a field. >From the POV of defining "compatible evolution" I guess I'd ask what constraints you envisage being important from your Python code generator POV ? We do allow fields to be deleted, which is a *non-compatible* evolution, but they MUST have been marked as deprecated for 2 cycles first. Because the C generated code is only used inside QEMU, when we have intentional non-compatible changes, we merely update the callers in QEMU as needed. If you are anticipating the python generated code to be a publically consumable API this becomes a bit more complex problem as you can't rely on fixing callers. > (3) Over the wire, the order of arguments to QMP commands is not > specified. In generating commands procedurally from introspection > data, I am made aware that there are several commands in which > "optional" arguments precede "required" arguments. This means that > function generation in Python cannot match the stated order 1:1. > > That's not a crisis per se. For generating functions, we can use a > stable sort to bubble-up the required arguments, leaving the optional > ones trailing. However, it does mean that depending on how the QAPI > schema is modified in the future, the argument order may change > between versions of a generative SDK. I'd like to avoid that, if I > can. I'd say sorting required vs optional arguments is doomed as a strategy. Stuff that is mandatory today can be made optional tomorrow and I think that's reasonable to want todo as we evolve an API design. Consider for example a command for querying something about a CPU. It might take a mandatory CPU ID number as its input parameter today. It could then be changed to accept either a CPU ID or a QOM Path, and both would be marked optional but at least one would need to set. This is backwards compatible from POV of callers because anyone passing a CPU ID today can carry on passing a CPU ID. New callers can choose to use QOM path instead. So I think however you express API params in python needs to cope with this scenario, which means not sorting args based on optional vs required. Effectively need to assume that all args are potentially optional on a long enough timeframe. > One trick I have available to me in Python is the ability to stipulate > that all (QAPI) "optional" arguments are keyword-only. This means that > Optional parameters can be re-ordered arbitrarily without any breakage > in the generative python API. The only remaining concern is if the > *mandatory* arguments are re-ordered. > > (In fact, I could stipulate that ALL arguments in Python bindings are > keyword-only, but I think that's going overboard and hurts usability > and readability.) I don't think you have any choice - they must all be keyword only if you want protection from future schema changes. > Marc-Andre has mentioned this before, but it might be nice to actually > specify a canonical ordering of arguments for languages that require > such things, and to make sure that we do not break this ordering > without good reason. Declaring a canonical ordering is not unreasonable as a concept on the surface. Essentially this is akin to fixing the order of fields in a C struct and making it append-only. None the less if you rely on this for positional arguments in python callers are still going to break when we *intentionally* delete fields/parameters (after a deprecation cycle). WIth strictly keyword only args, if the caller was not using the deleted field they won't be affected by the deletion as they won't be position sensitive. > (4) StrOrNull is a tricky design problem. > > In Python, generally, omitted arguments are typed like this: > async def example_command(arg: Optional[int] = None) -> None: ... > > Most Python programmers would recognize that signature as meaning that > they can omit 'arg' and some hopefully good default will be chosen. > However, in QAPI we do have the case where "omitted" is distinct from > "explicitly provided null". This is ... a bit challenging to convey > semantically. Python does not offer the ability to tell "what kind of > None" it received; i.e. unlike our generated QMP marshalling > functions, we do not have a "has_arg" boolean we can inspect. > > So how do we write a function signature that conveys the difference > between "omitted" and "explicitly nulled" ...? > > One common trick in Python is to create a new sentinel singleton, and > name it something like "Default" or "Unspecified" or "Undefined". Many > programmers use the ellipsis `...` value for this purpose. Then, we > can check if a value was omitted (`...`) or explicitly provided > (`None`). It is very unlikely that these sentinels would organically > collide with user-provided values (Unless they were trying to > explicitly invoke default behavior.) > > However, `...` isn't supported as a type and using it as the default > value invalidates the typing of the field. As far as I can tell, it > CANNOT be typed. We could create our own sentinel, but IMO, this > creates a much less readable signature: > > async def example_command(arg: Union[int, qmp.Default] = qmp.Default) > -> None: ... > > This probably doesn't communicate "This parameter is actually > optional" to a casual Python programmer, so I think it's a dead end. It sounds like you need a wrapper type. This StrOrNull scenario is QMP's "alternate" type IIUC, but you're trying to avoid expressing the existance fo the "alternate" type in the API ie you're trying to support example_command("foo") example_command(None) example_command() but that's impossible as the last 2 can't be distinguished If you explicitly huave an "Alternate" object type, which is a wrapper for any other type, then you can do example_command(Alternate("foo")) example_command(Alternate(None)) example_command() and now be able to distinguish the different scenarios. > (5) Generating functions from introspection data is difficult because > all of the structures are anonymous. The base type for most objects > becomes `Dict[str, Any]` but this isn't very descriptive. For Python > 3.8+, we can do a little better and use `Dict[Literal["name", "node"], > Any]` to help suggest what keys are valid, but we don't have access to > an in-line definition that pairs key names with values. Yep, we've also taken advantage of this to rename structs periodically as while it affected the generated C code inside QEMU, it didn't affect any QMP clients. I think it is not unreasonable to expose the struct names on introspection though, and just accept that it ties our hands a little more to avoid renaming structs. I don't think we rename frequently enough that this matters. > (6) Dealing with variants is hard. I didn't get a working > implementation for them within one day of hacking, so I stubbed them > out. There's no major blocker here, just reporting that I still have > to finish this part of the experiment. I'm pretty happy that Markus > simplified the union types we have, though. To my knowledge, I got > everything else working perfectly. Variants feels like it is the same kind of problem space as the StrOrNone scenario earlier. > (7) I have no idea what to do about functions that "may not return". > The QGA stuff in particular, I believe, is prone to some weirdness > that violates the core principles of the QMP spec. Maybe we can add a > "NORETURN" feature flag to those commands in the schema so that > clients can be aware of which commands may break the expectation of > always getting an RPC reply? A "NORETURN" flag sounds like a reasonable idea. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|