[engine] Move storage out of scheduler to engine
Review Request #3554 - Created March 10, 2016 and submitted
|kwlzn, patricklaw, stuhood|
Storageis part of the execution so it belongs to
Schedulernow it sees only
state_keys (Except for one special case: Noop introduced for cyclic deps). Translations (both from key to subjects/states and the other way around) happen during
Engine, and rename from
storage, since it also stores
- Add serializable's
Key, this allows 1) validate deserialized object matches its retrieval key's type 2) scheduler needs
State's type, this comes in handy
- Move subject_key to subject look up from scheduler to
- Add state_key to state look up to
- Add state to state_key conversion also in
StepResult.(state_key|dependencies), just the information needed by scheduler, given type is encoded in the key
Encounted two pickling errors:
- Need to define
__setstate__sicne we defined
- ConflictingProducersError not pickable due to python bug (
TypeError: ('__init__() takes exactly 4 arguments (2 given)')
Can drop this comment: it's effectively on by default now.
This is pretty awkward (and seems to be repeated in all of the tests).
execution_requestmethods should move to factory functions on BuildRequest that take the Engine (and the scheduler if need be)?
Please make this a factory function instead.
ConflictingProducersError.create(s, p, m)
I don't think this should be a method on the StepContext... rather,
Step.__call__can use it directly.
The only reason something should be on the StepContext is because the Node.step method can use it.
Backfill means something else... maybe: "Convert dependency keys into dependency content"?
Can you add a TODO here? We can't think of the right way to solve this now, but we should keep trying.
Also, it might be more explicit for the isinstance check to check for
excessive amount of parens here probably. will be
Instead, probably display the type only if you don't have a string for it.
Apparently using the latest pickling version allows for pickling objects with
__slots__... would you mind trying that out here instead? If it doesn't work, can leave this with a comment about pickling.
Can probably do:
key = Key.create(blob, type(obj), str(obj) if self._debug else None)
lgtm, handful of quick comments
any reason for this to not just be a public attribute, e.g.
self.storage = storage?
type checks should probably all use
==since we're comparing identity vs value.
also consider ditching the temp
state_typevar in favor of:
if state_key.type in [...]: ... elif state_key.type is Waiting: ...
... if state_key.type is Noop
_nameform in the call signature is a little confusing because upon first glance it appears that you're trying to make the arguments private - but it seems its just to avoid conflicts with the builtins
the idiomatic python approach to resolving conflicts with builtins/keywords as documented in PEP-8 would be to suffix with an underscore, like:
def __init__(self, digest, type_, hash_, string): ...
which is a little clearer. see: https://www.python.org/dev/peps/pep-0008/#function-and-method-arguments
Address Kris and Stu's comments.
Revision 2 (+270 -191)
Commited as 396e8df75087ba3318db068805c442726d0e19a1