-
Notifications
You must be signed in to change notification settings - Fork 122
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
refactor: cache signature structure in ops.testing state classes #1499
refactor: cache signature structure in ops.testing state classes #1499
Conversation
…ml or charmcraft.yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, clever and simple solution, and a nice performance win.
I was briefly confused when tracing the new vs old logic, because required_args
in __new__
stores required (as args -- not kwargs), while cls._init_required_args
stores required (as args or kwargs). Idk if there's a clearer short name for _init_required_args
though, and it seems easy enough to follow when not trying to go side by side (so fine for future readers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level: the time saved is marginal, but then code is rather straightforward and I'm not sure what would the alternative be, maybe custom types where the inspected bits are inlined or hardcoded?
Anyway +1 on this.
).parameters | ||
cls._init_kw_only = { | ||
name | ||
for name in tuple(parameters)[cls._max_positional_args :] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this was auto-formatted by ruff, the dangling :]
is kinda funny :]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was, yeah - I lazily just wrote it out on one line and let tox -e fmt
clean up.
Does the trailing happy face bother you? I could do something like move tuple(parameters)
out to a separate line to get the dict comprehension to be on a single line, if that would be better.
The Scenario code that limits the number of positional arguments (needed in Python 3.8 since the dataclasses there do not have
KW_ONLY
) is quite expensive. We have a method of providing this functionality in 3.8, but it is unfortunately expensive, and therefore a regression from older versions of Scenario.I considered 4 options here:
__init__
signatures (as we had at one point in the original PR). I'd rather not for the same reasons we made the decision at that time.dataclasses.KW_ONLY
with modern Python and only use our custom code on older Python. I spent some time looking into this, and couldn't figure out a way to do it where the code still seemed readable and maintainable enough.The most expensive parts of the
__new__
method are the ones that work withinspect
:inspect.signature
in particular, but also getting the default value of the parameters. If we assume that no-one is run-time altering the signature of the class (I believe no-one should be) then the values of these never actually change, but we are currently calculating them every time an instance of the class is created. This PR changes that to cache those three values the first time they are needed.There's one additional performance tweak in the branch that doesn't make a significant difference, but is trivial to do: when checking if the YAML files exist, skip the filesystem
exists()
call if we just created an empty temporary directory a few lines above, since we know that it will never exist.A drive-by: I happened to notice while working on this branch Harness referring to
options.yaml
, which does not exist (any more?) as far as I know, so a docs tweak to address that.Timing (best of 3):
Refs #1434