Re: Notes on Generating Python signatures for QMP RPCs

[Daniel P . Berrangé]

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 :|