Generate Dynamic Shapes by Krovatkin · Pull Request #37693 · pytorch/pytorch

Krovatkin · 2020-05-01T21:18:27Z

Yay!

dr-ci · 2020-05-01T21:43:18Z

💊 CI failures summary and remediations

As of commit 399704a (more details on the Dr. CI page):

2/3 failures possibly* introduced in this PR
- 1/2 non-CircleCI failure(s)
1/3 broken upstream at merge base 363a2d9 since May 19

1 job timed out:

binary_windows_libtorch_3_7_cpu_release_build

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet:

pytorch_macos_10_13_py3_test since May 19
- 🔁 rerun

ci.pytorch.org: 1 failed

Failed: pr/py3.6-clang7-rocmdeb-ubuntu16.04

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 33 times.

zdevito

Partial review: I didn't get to profiling_record.cpp/h yet, Ill try to that part tonight.

zdevito · 2020-05-04T21:54:57Z

-              push(stack, pttp->isSubtypeOf(expected));
+              const TypePtr& expected = af.types[inst.X];
+              auto expected_type = expected->cast<TensorType>();
+              bool pass = true;


This looks more complicated than it needs to be

agreed this can be simplified.

zdevito · 2020-05-04T21:58:49Z

    Function** functions;
    std::function<void(std::vector<IValue>&)>* profile_functions;
    TypePtr* types;
+    ShapeSymbolTable symbols2dims;


ActiveFrame is a way of keeping a bunch of pointers on the stack in the interpreter rather than reloading after every instruction. But here it is holding an entire hash table, which is destroyed every time we enter/exit a function. This symbol table should be local to Frame, not ActiveFrame, I believe.

ah yes... i think i actually moved it in one of the branches :-( but I guess I lost it in one of the rebases.

zdevito · 2020-05-04T22:04:00Z

+                pass &= bindSymbolicShapes(
+                    af, t.sizes(), expected_type->symbolic_sizes());
+                if (pass) {
+                  pttp = expected_type->merge(pttp, false);


I think the composition of bindSymbolicShapes, merging and isSubtypeOf here is not ideal. First, it is a bit hard to follow direct intent of tracking whether the guard matches. I am still not sure what merge, and false is suppose to be here. Second, this is in performance critical code. isSubtypeOf checks are slow, and there is a lot we can do here to combine the bindSymbolicShape, and isSubtypeOf together to make a single pass over the tensor. Finally, I am confused as to whether returning false from bindSymbolicShapes will cause the guard to fail. I think it should, since we found evidence of the program no longer passing its profiled model, but it doesn't directly lead to a push false, so I am not sure.

it doesn't directly lead to a push false, so I am not sure.

yes it's not too obvious that expected_type either contains static or symbolic symbols and isSubtypeOf will fail when comparing with pttp which only has static symbols unless they are either the same or we run bindSymbolicShapes . I explicitly added && bound_successfully && to push.

zdevito

This is getting there -- inline comments highlight a few places where I can't follow the data structures making the logic difficult to understand.

zdevito · 2020-05-05T03:35:55Z

+      GRAPH_DEBUG(symbol, " is now bound to ", new_size);
+      new_symbols.push_back(symbol);
+    } else {
+      if (symbol_table.getValue(symbol) == new_sizes[i]->static_size()) {


use new_size

zdevito · 2020-05-05T03:37:32Z

+c10::ShapeSymbol ShapeSymbolTable::toSymbol(
+    Dimension val,
+    std::map<Dimension, c10::ShapeSymbol>& dims2symbols,
+    ProfilingRecord* pr) {


pr is unused here

zdevito · 2020-05-05T03:37:44Z

+c10::ShapeSymbol ShapeSymbolTable::GetSymbolInSet(
+    Dimension new_size,
+    c10::ShapeSymbol set,
+    ProfilingRecord* pr) {


pr is unused here

zdevito · 2020-05-05T03:40:14Z

 namespace torch {
 namespace jit {

+c10::ShapeSymbol ShapeSymbolTable::toSymbol(


I don't understand the name toSymbol, getOrCreateSymbolForDim? Shouldn't this be a method on whatever owns dims2symbols? It is weird that this is taking the map as an argument.

I removed toSymbol and added a comment to getSymbolInSet

zdevito · 2020-05-05T03:46:22Z

  std::shared_ptr<Graph> profiled_graph_;
  std::mutex mutex_;
  size_t profiling_count_;
+  std::map<int64_t, std::map<Value*, TensorTypePtr>> profiled_types_per_frame_;


I can't follow this data structure in the code. There are no comments about what it stores, and it results in a lot of "x.first" and "x.second" confusion.

zdevito · 2020-05-05T03:46:53Z

+  void reset() {
+    sets_.clear();
+  };
+  std::map<c10::ShapeSymbol, std::map<Dimension, c10::ShapeSymbol>> sets_;


I don't understand what this stores, it is a nested map of ShapeSymbol to Dimension to Shape symbol?

I added a comment.
We are implementing the algorithm we discussed but the implementation merges by every dimension location, not by a whole set at once. It makes the implementation so much simpler, but it's a bit of a break from the conceptual algorithm we discussed.

// A helper structure used for partitioning // a set associated with ShapeSymbol into // subsets each associated with a unique // Dimension (dimension value) // if the same symbol is // assigned to multiple `Dimension` in one of the // profiling runs the set of dimension locations // associated with the set will be split into // two subsets each associated with the new Dimension // value struct ShapeSymbolSets {

zdevito

This looks good now. I have minor nits about the speed along the hot path but that can be done in a different PR since one seems functionally correct and the profiler is not on by default.

zdevito · 2020-05-13T04:10:49Z

+      continue;
+    }
+    auto symbol = *sym_shapes[i];
+    if (!isBound(symbol)) {


This is in the hot path, so be careful about hash table lookups, this is doing two lookups even though it only needs to be doing 1.

zdevito · 2020-05-13T04:11:19Z

+    TORCH_INTERNAL_ASSERT(!s.static_size());
+    data_[s] = v;
+  }
+  std::map<c10::ShapeSymbol, Dimension> data_;


unordered map? A red-black tree along the fast path is going to be pretty slow.

zdevito · 2020-05-13T04:12:28Z

+// to assign dimension values to ShapeSymbols
+// and fail a guard if the same symbol
+// is assigned more than one dimension value.
+struct ShapeSymbolTable {


A class used by the interpreter but not specific to profiling should probably go in a different file.

zdevito · 2020-05-13T04:22:04Z

+//   * For every two runs, we iterate over all `symbic_shapes_`  and compare their `ShapeSymbol`s in the same position. 
+//     * if we observe that for every dimension location that has the`ShapeSymbol S1`  in run #1 there is **only one** `ShapeSymbol S2` in the same dimension location in run #2, we conclude that the invariant holds.
+//     * However, if we observe some dimension locations in run #2 have `ShapeSymbol S2` and the other ones have `ShapeSymbol S3` we would like to partition the virtual set of dimension locations associated with `ShapeSymbol S1` into two new subsets, so the invariant holds.
+//     * The partitioning works by assigning a new symbol to the dimension locations (associated with `ShapeSymbol S1`) that have `ShapeSymbol S2` and another new symbol to the dimension locations that have `ShapeSymbol S3`. In other words,


Thanks for adding this comment, it makes it much clearer what is happening in the merging algorithm.

facebook-github-bot