Scheduler optimization #185
Conversation
achoum
commented
Jul 6, 2023
achoum
commented
- Release unused memory during evaluation
- Deterministic op evaluation order
- Release unused memory during evaluation - Deterministic op evaluation order
| self._internal_ordered_id = Operator.next_internal_id | ||
| Operator.next_internal_id += 1 |
There was a problem hiding this comment.
is this necessary? could we instead ensure that graph.operators is always the same order? e.g. by using an ordered set instead of a set in the graph's _operators, _nodes, etc. while building the schedule (ordered set in python is a dict with no values :) ). not sure if this would suffice?
There was a problem hiding this comment.
The alternative solution would be to replace some of the set-inputs in the API and intermediate functions by lists. I don't see that as ideal.
The other benefit of this change is that "id" is now a small number that is easier for people to read in error messages. Ideally, I would like to update all the "ids" with this mechanism.
The third benefit is that an "id()" is not guaranteed to be unique for two objects with non-overlapping lifetimes. This is not an issue for our current code, but this is a property that is not great for what we use it for.
| @@ -208,7 +208,8 @@ def build_schedule( | |||
| # Operators ready to be computed (i.e. ready to be added to "planned_ops") | |||
| # as all their inputs are already computed by "planned_ops" or specified by | |||
| # "inputs". | |||
| ready_ops: Set[Operator] = set() | |||
| ready_ops: List[Operator] = [] | |||
| ready_ops_set: Set[Operator] = set() | |||
There was a problem hiding this comment.
I think it's safe to remove ready_ops_set now we got ready_ops. I don't see its purpose.
There was a problem hiding this comment.
"ready_ops" and "ready_ops_set" contain the same data. Those two containers have different properties and costs (one is a list, the other is a set). The list is great for a FILO, while the set is great to check for presence of an item.
I think it's safe to remove ready_ops_set now we got ready_ops
You mean there is a situation where this code will not work? If so, can you give details?
| # Make evaluation order deterministic. | ||
| # | ||
| # Execute the op with smallest internal ordered id first. | ||
| ready_ops.sort(key=lambda op: op._internal_ordered_id, reverse=True) |
There was a problem hiding this comment.
This only affects the ops that depend directly on the inputs (current ops in ready_ops) and executes them in instantiation order, right?
I'm not sure why this is better for memory release.
There was a problem hiding this comment.
This sort ensure that the execution order of the operators is deterministic. This has not impact for the result. However, we have unit tests that check some of the internals that were affected by this non-deterministic evaluation. With this change, the unit tests are simpler :)
While the results are not impacted, the order of execution could change the speed and RAM usage of executing the graph. Making the order of execution deterministic reduces the risk of flakiness in resource constraints environment.
|
Thanks. Adding the open questions to the agenda of our next meeting. |
