Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Glitch typing #2
Conversation
petevg
and others
added some commits
Oct 11, 2016
johnsca
approved these changes
Oct 17, 2016
I really like the type hinting!
Pete also made the move into matrix/plugins/glitch/ at my behest in the other branch, so you might want to check against the original branch to make sure you didn't miss any other changes of note.
| + @wraps(func) | ||
| + async def wrapped(model, objects, **kwargs): | ||
| + for obj in objects: | ||
| + await func(model, obj, **kwargs) |
johnsca
Oct 17, 2016
Owner
I think it would be better to have the actions explicitly accept sets of objects, rather than implicitly converting them by calling the action multiple times, because some implementations (e.g., juju run or juju run-action) can handle multiple objects natively, which would be more efficient.
However, since the calls will be dispatched async, it's not a big enough deal to consider a blocker.
bcsaller
Oct 17, 2016
Contributor
I was hoping to avoid that all action include the boilerplate for doing set iteration. In addition when we add things like inflight, interval and max_retries I didn't want each action to have to carry support for those, Being able to manage the individual options seems the fix for that.
That said we can change the calling protocol to include (set, item in set) for action and support a StopIteration return that says "I dealt with the entire set"
johnsca
Oct 17, 2016
Owner
That's a fair point. I'm fine with sticking with the current approach to ease single-point-of-handling of those options. That functionality is clearly not compatible with the actions handling multiple items themselves.
| +# Define your actions here | ||
| +# | ||
| +@action | ||
| +async def reboot(model: Model, unit: Unit): |
johnsca
Oct 17, 2016
Owner
Note for self: If an action can accept multiple types (e.g., units or applications), it can use Union (e.g., Union[Unit, Application]).
| + | ||
| + | ||
| +# XXX: this most likely will need an async def | ||
| +# depending on libjuju |
johnsca
Oct 17, 2016
Owner
I don't think so. All this is doing is accessing attributes on the model. The model is updated in the background by a watcher, so accessing the attributes doesn't make any calls to the API and is entirely synchronous.
bcsaller
Oct 17, 2016
Contributor
Yeah, I was thinking if things like health checks would be different or not, but we can ask that they populate the model the same as the watchers do perhaps.
petevg
reviewed
Oct 17, 2016
Other than some quibbles with naming conventions, which might just be a style difference (I prefer to be as verbose as I can be, while still keeping lines to 80 characters), and what I think is an error in the loop in main.select, this looks good to me.
| + return wrapped | ||
| + | ||
| +# Public singleton | ||
| +Actions = _Actions() |
petevg
Oct 17, 2016
Collaborator
Probably should be actions or ACTIONS, esp. since the class is basically just a dict in a Singleton's clothing; If we're going to get rid of the classmethods, might as well get rid of the ugly camel case, too. :-)
bcsaller
Oct 17, 2016
Contributor
I agree, uppercase is fine for what is really a global. Also making change s/Selectors/SELECTORS/
| +def default_resolver(model, kind, name): | ||
| + if kind not in ["application", "unit", "model", "controller", "relation"]: | ||
| + return None | ||
| + entities = getattr(model, kind + "s") |
petevg
Oct 17, 2016
Collaborator
I think that, whenever you are tempted to call any of the *attr methods, you should think carefully about the approach, as it tends to mean that you are about to write code that is difficult to reason about, or that is going to do surprising things with unexpected input (you do guard against this in the line above).
I can see why it is compelling to have some automagic to translate some strings to objects in the model (though this doesn't address the needs of the units selector, which might want to act on multiple applications). It does start us down the path of treating the plan file as a DSL, and parsing it into executable code.
bcsaller
Oct 17, 2016
Contributor
I think maybe a lookup table rather than a str manipulation is better here, but the basic issue remains.
| + args = [model] | ||
| + # This can raise many an exception | ||
| + for selector in selectors: | ||
| + data = selector.copy() |
petevg
Oct 17, 2016
Collaborator
I think that you should feel free to just do selector = selector.copy() here. We don't need the old reference, and it makes the code below more readable if the person reading it doesn't have to keep track of what "data" is.
| + # This can raise many an exception | ||
| + for selector in selectors: | ||
| + data = selector.copy() | ||
| + m = Selectors.get(data.pop('selector')) |
petevg
Oct 17, 2016
Collaborator
Would prefer the original .func, or .get_func, just as a semantic reminder that you are getting an executable. Makes the code easier to review and contribute to :-)
| + data[k] = o | ||
| + | ||
| + #print(m, args, data) | ||
| + cur = m(*args, **data) |
petevg
Oct 17, 2016
Collaborator
You need to update *args to include objects here, correct?
... or get rid of the generic args argument, and explicitly pass in model and objects.
johnsca
Oct 17, 2016
Owner
Not objects, but cur. But I'm +1 to making it explicit, as cur = m(model, cur, **data)
johnsca
Oct 17, 2016
Owner
To enable making the call explicit, the selectors would need to change to always accept the "working set" param, even for "top-level selectors" like units.
petevg
Oct 17, 2016
Collaborator
johnsca: Yes. I was thinking in terms of last week's pass at this, where I was abusing the dynamic typing of Python to slip in the "nounits" arg, and keep all the selectors consistent in terms of the number of positional arguments that they accept. Since we're being more formal here, the dancing about with args bit makes sense.
| + | ||
| + #print(m, args, data) | ||
| + cur = m(*args, **data) | ||
| + args = [model, cur] |
petevg
Oct 17, 2016
•
Edited 1 time
-
petevg
Oct 17, 2016
Collaborator
-
Oct 17, 2016
Edit: nm. my eyes had missed the indentation level (I thought that we were returning after this statement).
This code doesn't do anything. Did you initially intend to return args here, instead of just returning cur?
johnsca
Oct 17, 2016
Owner
It does do something: it adds the working set to the positional params. Currently, "top-level" selectors like units don't actually accept an initial working set; thus, the objects list here is ignored. I think making the working set explicit, required, and initialized by objects is a good idea.
petevg
Oct 17, 2016
Collaborator
johnsca: aha. I missed where we were in the nesting. You're correct.
| + """ | ||
| + rule.log.info("Starting glitch") | ||
| + | ||
| + output_filename = DEFAULT_PLAN_NAME.format(time.time()) |
petevg
Oct 17, 2016
Collaborator
If we run a lot of tests in a row, we'll wind up spamming the temp dir with executed plans. I think that I'd rather just write out a single plan file, and leave it up to the tester/tooling to make backups of past runs if they desire.
| + | ||
| +class _Selectors(dict, metaclass=Singleton): | ||
| + def decorate(self, f): | ||
| + wrapper = enforce.runtime_validation(f) |
petevg
Oct 17, 2016
Collaborator
I'm always wary when I see type checking in Python -- it feels like an argument for not using Python :-)
In this case, I had already begun to add some loose "guarantees" to the code, and this does so more formally, without needing to trust the programmer as much. I think that it makes sense, even if it does make me want to just port the project to Scala :-)
johnsca
Oct 17, 2016
Owner
I don't understand the sentiment that adding type checking implies we should give up Python entirely. As you mentioned, we were already starting to do ad-hoc type checking, and there are plenty of other reasons to use Python over Scala.
Type hints are now a standard feature of Python, albeit optional, and it we should use it when it makes sense. One of Python's strengths has always been the willingness to adopt ideas from other languages and paradigms when they make sense, while still allowing flexibility to not use them when they don't.
| + # if there are string names being passed (from a serialized plan for | ||
| + # example) we must resolve them relative to the current model. This is | ||
| + # pluggable using a resolver object which takes a model, | ||
| + cur = None |
johnsca
Oct 17, 2016
•
Edited 1 time
-
johnsca
Oct 17, 2016
Owner
-
Oct 17, 2016
I think Pete's right, and this should be initialized to (or just replaced entirely with) objects
johnsca
Oct 17, 2016
•
Edited 1 time
-
johnsca
Oct 17, 2016
Owner
-
Oct 17, 2016
Also, I'd like to push for calling this var working_set instead of cur or objects.
bcsaller
Oct 17, 2016
Contributor
I don't understand the first comment here, let's circle back on this.
| + | ||
| + rule.log.info("Writing glitch plan to {}".format(output_filename)) | ||
| + with open(output_filename, 'w') as output_file: | ||
| + output_file.write(yaml.dump(glitch_plan)) |
johnsca
Oct 17, 2016
Owner
I'd like to push for writing the plan to disk just after validating it, before executing it.
bcsaller commentedOct 16, 2016
Before I take this too far I was to see if people like the approach. It attempts to move from an ad-hoc typing system to an officially supported one. Currently it is using runtime type validation but we might want to look at switching the type checking to mypy and hook in its static analysis at the time the plan is formed. That said that is still runtime check, so I am fine with this now.
This also moved the test under the top level test directory. I like to separate the tests from the runtime as they sometimes have different deps and testing deps won't be packaged with the release.