Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update primitive options type #52

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Conversation

jyu00
Copy link
Contributor

@jyu00 jyu00 commented Oct 20, 2023

No description provided.

Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jyu00 ! dataclasses are great. I'm not sold that the following is the best idea, but since I already did it, I figured I might as well post it here. The idea is to put a thin layer of magic (blegh) on top of dataclasses so that they are typed in a way we might like, and to support serialization/deserialization of nested options more naturally:

from dataclasses import asdict, dataclass, field, replace
from typing import Optional, get_type_hints

class OptionsType(type):
    """Metaclass for options classes.
    
    Ensures all options have defaults. This is done to prevent non-locality of default values.
    """
    def __new__(cls, name, bases, attrs):
        # ensure an annotations dict exists
        annotations = attrs["__annotations__"] = attrs.get("__annotations__", {})

        # demand default values for all attributes
        for attr in annotations:
            if attr not in attrs:
                raise ValueError(f"'{name}.{attr}' must be assigned a default value")

        # allow sub-options to forgo specifying an annotation or using field() attributions   
        for attr in list(attrs):
            val = attrs.get(attr)
            if isinstance(val, OptionsType):
                 attrs[attr] = field(default_factory=val)
                 annotations[attr] = val
            elif isinstance(type(val), OptionsType):
                # just val.copy would copy any changes made to val between now and construction
                attrs[attr] = field(default_factory=val.copy().copy)
                annotations[attr] = type(val)

        # construct the dataclass and return
        return dataclass(super().__new__(cls, name, bases, attrs))
    
class Options(metaclass=OptionsType):
    def copy(self):
        return replace(self)
    
    def to_dict(self):
        # note: we could do something more complicated that skips default values, but then we run
        # the risk of getting out of sync with another version's defaults if this dict is serde'd.
        return asdict(self)
    
    @classmethod
    def from_dict(cls, options_dict):
        if options_dict is None:
            return cls()
        hints = get_type_hints(cls)
        return cls(**{
            attr: hints[attr].from_dict(val) if isinstance(hints.get(attr), OptionsType) else val 
            for attr, val in options_dict.items()
        })

With example:

class TwirlingOptions(Options):
    gates: bool = False
    measure: bool = False

class TranspilationOptions(Options):
    optimization_level: int = 3
    coupling_map: None = None

class ExecutionOptions(Options):
    max_circuits: Optional[int] = 900
    shots: Optional[int] = 4096
    samples: Optional[int] = None
    shots_per_sample: Optional[int] = None
    interleave_samples: bool = False

class EstimatorOptions(Options):
    twirling = TwirlingOptions
    transpilation = TranspilationOptions
    execution = ExecutionOptions(shots=3)

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you speak to the motivation of why "options" should be included in the required interface of a primitive, and not entirely left to implementations? There's no individually defined options, so what is actually being gained; a user must consult the documentation of their implementation already, to know what each supported value means, because it's not defined here (and imo that's a good thing). The more that's defined, the less free implementations are to make things that are better; this is the current problem you're describing with the Qiskit Options class, which the current primitives require exists.

An interface should typically require only the absolute minimum set of methods/attributes that all implementers must have, and their semantics. Here, defining the Python type of options without specifying the options doesn't actually achieve very much; the semantics are still undefined. This RFC attempts to enforce various things such as "'options' should autocomplete", "'options' should behave under dir(options)", "'options' should behave under copy.{copy,deepcopy}", but those are the standard behaviours of well written Python objects (which Options isn't, really). It doesn't (and cannot) actually make the options transferable from one implementation of a primitive to another in a meaningful way, which is the purpose of an abstract interface.

If Aer and IBM Runtime / other IBM offerings want to define some shared options that they'll both accept and define the semantics of those (so there's a subset that you can transfer), that can be done separately to the base primitives interface in a different package / documentation entirely. You're also then much freer to upgrade that shared specification over time; you don't need to worry about the compatibility of other implementers, so we're not holding back IBM development.

@jakelishman
Copy link
Member

jakelishman commented Oct 24, 2023

Fwiw, I'm not at all opposed to defining a helper class somewhere that makes it easier to define nested dataclasses, I just don't think that the use of it should be enforced by the primitives interface.

My (very much from the outside) view is that there's nothing Runtime or Aer are doing that isn't already served by standard usage of the implementations in one of dataclasses, attrs or pydantic, so I'm not sure there's any new generic code that needs to be written. I would also like to see Qiskit's Options class eventually disappear for all the reasons given in this RFC.

@jyu00
Copy link
Contributor Author

jyu00 commented Oct 24, 2023

Can you speak to the motivation of why "options" should be included in the required interface of a primitive, and not entirely left to implementations?

An interface should typically require only the absolute minimum set of methods/attributes that all implementers must have, and their semantics.

This RFC attempts to enforce various things such as "'options' should autocomplete", "'options' should behave under dir(options)", "'options' should behave under copy.{copy,deepcopy}",

This whole RFC can be summarized in one sentence - let's not use qiskit.providers.Options as the return type for primitive.options. For all the reasons you described above - it gives the implementers less freedom to do what we want. The rest of the RFC just lists out what an implementer might want to do that is currently blocked by having qiskit.providers.Options as the type (such as auto-complete); not what the base primitives should have.

But if we don't use qiskit.providers.Options, we'd either need an alternative (this RFC proposes a dataclass), or drop options property entirely. Frankly I'm fine with either, as either would unblock an implementer such as QRT from adding the features we want.

@jakelishman
Copy link
Member

we'd either need an alternative (this RFC proposes a dataclass), or drop options property entirely

My whole spiel boils down to justifying "'drop options entirely' is better" - if that's on the table, I'd vote that way.

@ikkoham
Copy link

ikkoham commented Oct 25, 2023

+1 Pedantic https://docs.pydantic.dev.

@1ucian0 1ucian0 added the RFC proposal New RFC is proposed label Oct 27, 2023
@jyu00
Copy link
Contributor Author

jyu00 commented Nov 6, 2023

My whole spiel boils down to justifying "'drop options entirely' is better" - if that's on the table, I'd vote that way.

While I didn't feel too strongly about this, @ElePT brought up a good point that application developers who may encounter different implementation of the primitives would want a standard way to set/get options.

@jakelishman
Copy link
Member

I would support this view if there were any standardised options with defined semantics and types. As far as I'm aware, there's no plan for this, though, which means that the Options objects must all be specially dispatched on depending on the particular implementer already. Adding standardised getters and setters in the abstract, without requiring any particular options or structure in them to exist, doesn't actually make it any more possible to do anything in the abstract.

@ihincks
Copy link
Contributor

ihincks commented Nov 6, 2023

@jakelishman I assume you agree that most primitive implementations have need for some kind of optioning. I think we can also agree, as you argue, that they won't have standardized option names or values. Despite this, I think that there's still value to user experience in homogenizing how these options are exposed. It's very frustrating when one implementation's options have a copy() method, and another one's doesn't, and one has an update, and the other doesn't, and one has subscript lookup, and the other doesn't, and one has set_option() methods on the class, and the other doesn't etc, etc. Sure, one can go and look it up in the docs, but that doesn't address the usability. If you can just tab-complete to find the options you remember being there, it doesn't feel like a big deal (when you're hacking together a notebook, which is the most common user experience I expect) that the options for various implementations are different.

@jakelishman
Copy link
Member

jakelishman commented Nov 6, 2023

I assume you agree that most primitive implementations have need for some kind of optioning.

I do, but that is true of almost all objects. The point of an interface is to abstract over differences between implementations for users. There's no concrete option being added here, so saying "the getters and setters are concrete" is no good, because they cannot be used in a concrete manner with only the abstract interface as the allowed names and the semantics of the options are not configurable. It constrains implementations into the future without enabling users, which means that when implementations want to do something that isn't representable well within dataclasses, they're going to move the option into their primitive implementation state, making Options even less reliable for users.

It's very frustrating when one implementation's options have a copy() method, and another one's doesn't, and one has an update, and the other doesn't, and one has subscript lookup, and the other doesn't, and one has set_option() methods on the class, and the other doesn't etc, etc.

I agree that that's frustrating, but this RFC does not make it any simpler to update options between instances because it does not define the semantics of any options, nor what other state a primitive implementation may have. If anything, it makes it easier to silently do the wrong thing, by making it seem like the options can be accessed abstractly as part of the interface, when they cannot.

Put this way: if you say that Options must have a copy() method, will you also require in the interface that an Estimator instance is fully defined by its base class type object and its Options alone? If not, then what can a user actually rely on by being able to do Options.copy()? Can they reliably always do

estimator1 = Estimator(...)
estimator2 = Estimator(dataclasses.asdict(estimator1.options))

and have estimator2 have exactly the same semantics as estimator1? What if (e.g.) estimator1 manages a connection to a session that gets dropped before estimator2 is constructed?

The point is that, as best as I can tell, specific implementations of Estimator are going to have options that are not in Options. The IBM Runtime currently has backend and session, for example. What if an implementation decides that all its configuration will just be part of the object state (which is natural for a Python class)? Because there's no definition of what constitutes an "option", there's nothing stopping an implementation from doing that, so a user cannot rely on Options.update to actually do anything, and Options.copy will copy some unknown amount of semantics, but they're still going to have to consult the documentation to know what else (if anything) they need to copy.

If an interface is meant to enable particular user behaviours, it's not really enough to expect that all implementers will do what "feels natural" to us here; if we want a behaviour to be possible in the abstract (e.g. "a user should be able to construct a new version of a primitive in the abstract"), then the whole semantics of what that means need to be defined by the interface. Just doing bits of it (e.g. the Options.copy method) without the rest of the semantics doesn't provide any guarantees that a user can rely on - what's enough for the IBM Runtime might not be enough for Aer.

If you can just tab-complete to find the options you remember being there, it doesn't feel like a big deal [...] that the options for various implementations are different.

For the "algorithms designer" persona mentioned above as justification for the abstract options, I'd argue that this actually is the whole ballgame. It doesn't matter if you know the syntax for setting an option in a library if you don't know the semantics; it's still unsettable. The worst-case scenario is actually that it does set, then without warning does something different to what you expected. (Note I'm not at all suggesting making things deliberately different between implementations to avoid this, just in favour of not having an interface require that "similar" things look abstract when they're not.)

When we talk about "[a user] hacking together a notebook" for tab completion, then we're not talking about a user of the abstract interface any more. I'd argue that tab completion is not something an abstract interface should be requiring. It's a suggestion for good usability, but requiring that everybody uses a base dataclasses.dataclass to enforce it is (imo) unnecessarily heavy-handed; all well-behaved Python objects should really provide that already.

@ihincks
Copy link
Contributor

ihincks commented Nov 7, 2023

This is not a response, just a comment: all of @jakelishman 's arguments apply just as well to BaseEstimator/Sampler as they do to BasePrimitive. So the argument is essentially not to have an options attribute on any Base* class.

This is not an argument against unifying how various implementations choose to implement their (inevitable) options attributes, or, as @jakelishman said, "defining a helper class somewhere that makes it easier to define nested dataclasses".

@blakejohnson
Copy link
Contributor

@jakelishman what if we want to make the minimal promise that users can define some options at instantiation of an Estimator or Sampler and then override those options when calling run on those objects? At minimum we have some examples in the wild now where an algorithm wants to an accept an instantiated Sampler and/or Estimator and then override the shots option in various phases of the algorithm.

@jakelishman
Copy link
Member

jakelishman commented Nov 7, 2023

If we define semantics for specific options and the effect that implementations must cause them to have, then I'm totally happy. If not, then we're still in a situation where a user can't pass shots to either the constructor or run for an abstract implementation because that implementation might not accept the shots argument (or might use it to mean something totally different). My argument in that case is that we shouldn't include it in the base interface, but still should suggest that implementations do it if it makes sense for them.

I think the idea of "scoping" options between the object state and run method is good, though as it stands, this PR isn't addressing how that would look - would you have to pass a complete Options instance to run, for example, or would you be able to override individual options? If the latter, how do you override a option in a nested dataclass?


Just to be clear: I think the idea of getting rid of the current qiskit.Options from the interface is a great idea and absolutely should be done - it's not a good object at all. I personally think the API will be clearer on what's actually required for both users and implementers if there's no mandated Options in the base abstract interfaces, though I'm not at all against IBM's specific offerings following the same design document to make their user experiences feel similar between them.

I should say too, that at this point in the process, if I couldn't talk you round to my view with my above comments, and I'm not reading anything so far that causes me to change my opinion right now, I don't necessarily think that ought to be a blocker on accepting the RFC as-is. I don't think it's the best possible interface, but I do think it's an acceptable interface, and I don't think there's any large technical risk from it - the worst case scenario to me is mostly that options is actually just a bit of a useless attribute from an abstract perspective, but otherwise the primitives function give-or-take how we all want.

Copy link
Contributor

@blakejohnson blakejohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not ignoring Jake's comments, but it seems like the majority view is still to have an options attribute on the base class. So, let's proceed.

@blakejohnson blakejohnson merged commit b05a0a8 into Qiskit:master Nov 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC proposal New RFC is proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants