Add a utility function to filter out elements that are not of a certain type by naoyam · Pull Request #267 · csarofeen/pytorch

naoyam · 2020-08-04T23:34:17Z

This adds a utility function that I think can reduce repetition. It's not yet completed, but I'd like to have feedback before going further.

We often have code like this:

std::vector<Val*> values;
for (auto x: values) {
  // do something only for TensorView vals
  if (x->getValType() == ValType::TensorView) {
   TensorView* x_tv = x->as<TensorView>();
   ...
  }
}

This PR adds ir_utils::filterVals, which can be used to simplify code patterns like the above:

for (auto x: filterVals<TensorView>(values)) {
  // x is TensorView*
 ... 
}

For this, I added static constexpr ValType type to the Val classes (not yet completed; only added to IterDomain and TensorView for now).

Let me know if you have any feedback. Thanks!

type

tlemo · 2020-08-05T00:49:13Z

+Container<FilterType*> filterVals(const Container<ElementType*>& container) {
+  Container<FilterType*> out;
+  for (auto& s : container) {
+    if (s->getValType() == FilterType::type) {


since you're interested in a better notation, would you be interested in adding another building block to the IR root class? ie. add this to Statement:

template<class T> bool IsA() const { return dynamic_cast<const T*>(this) != nullptr; }

this is a counterpart to Statement::as that I have on my todo list, and would allow us to say:

if (s->isA<FilterType>()) { ... }

... and also eventually eliminate the need for ValType altogether.

That looks nice, but one thing I didn't want to change is that the type checking is currently always done with getValType(). I think we can replace that with something like IsA, but I wonder whether ValType values and classes have one-to-one relationship. For example, would there be a case where there could be two different classes that are a subclass of Val AND have the same ValType value. I don't think that's the case now, but is this something that could happen in the future?

I think switch to a more consistent IsA makes sense to me (uniformed API).
Can't we have specialization of isA so that we cannot necessarily do dynamic cast? that seems to also resolve the restriction on the one-to-one mapping.

template<class T> bool IsA() const { return dynamic_cast<const T*>(this) != nullptr; } template<ValType T> bool IsA() const { return getValType() == T; }

Using IsA<>() with the non-mapping filter and return would still require us to add an extra template parameter in the filterVals, (one for filter criterion used in the IsA, one to cast the return type as). Might bother people 🤷

@naoyam: a potential mismatch between the tag (ValType) and the actual type is one of the reasons I'm wary of using tags , especially when they duplicate the type hierarchy 1:1 (like we do today). Since we're using a hierarchy to model the taxonomy of nodes, I think it's both easier and safer to use the types themselves. Another benefit of using the types (through a notation like isA<T>), is that you can query for abstract (non-leaf) types too, ex. isA<Expr> or isA<Val>

@jjsjann123 : as I explained above, the main benefit of isA<T> is not the notation itself, but the fact that it avoids tags altogether. So a version which tries to mix tags and types seems confusing at best and I'd recommend against it.

I'm not suggesting we should go as far as eliminating tags in this change. It seems an opportunity for adding isA<T> since it fits well with the proposed utility, but it's not really critical to this PR.

I agree that we could get rid of the tag and just use the runtime type information would be more robust. However, I'd prefer to keep using the tag for type checking at least in this PR as changing that is not the purpose. I'll create a separate issue to consider.

jjsjann123

I like the updated version a lot 👍

jjsjann123 · 2020-08-05T13:03:52Z

+class FilterValContainer {
+ public:
+  using value_type = FilterType*;
+  using const_iterator = FilterIterator<FilterType, InputIt>;


nitpick: Wouldn't we also want a non-const iterator?

Do we want to modify the original container through the filtered view?

There may be some use cases, but I suspect we don't need that at this time. I left a related comment for a future extension.

jjsjann123 · 2020-08-05T14:18:53Z

+Container<FilterType*> filterVals(const Container<ElementType*>& container) {
+  Container<FilterType*> out;
+  for (auto& s : container) {
+    if (s->getValType() == FilterType::type) {


I think switch to a more consistent IsA makes sense to me (uniformed API).
Can't we have specialization of isA so that we cannot necessarily do dynamic cast? that seems to also resolve the restriction on the one-to-one mapping.

template<class T> bool IsA() const { return dynamic_cast<const T*>(this) != nullptr; } template<ValType T> bool IsA() const { return getValType() == T; }

jjsjann123 · 2020-08-05T14:21:18Z

+Container<FilterType*> filterVals(const Container<ElementType*>& container) {
+  Container<FilterType*> out;
+  for (auto& s : container) {
+    if (s->getValType() == FilterType::type) {


Using IsA<>() with the non-mapping filter and return would still require us to add an extra template parameter in the filterVals, (one for filter criterion used in the IsA, one to cast the return type as). Might bother people 🤷

tlemo · 2020-08-05T17:39:08Z

+};
+
+template <typename FilterType, typename InputIt>
+class FilterValContainer {


We should probably rename this to say "view" to better reflect what it is (it's not really a container since it doesn't own the values). FilteredView ?

Also, InputIt -> just Iterator? (technically it's not an input iterator but rather a forward iterator)

Thanks for the suggestion. Renamed to FilteredView.

tlemo · 2020-08-05T17:41:38Z

+class FilterValContainer {
+ public:
+  using value_type = FilterType*;
+  using const_iterator = FilterIterator<FilterType, InputIt>;


Do we want to modify the original container through the filtered view?

tlemo · 2020-08-05T17:42:40Z

+  }
+
+ private:
+  InputIt input_it_;


const for the data members?

Use more precise names for the iterators: begin_ and end_ ? (in particular, last != end so we should avoid using that name)

Thanks. Done.

tlemo · 2020-08-05T17:43:39Z

+
+template <typename FilterType, typename ContainerType>
+auto filterVals(const ContainerType& inputs) {
+  return filterVals<FilterType>(inputs.begin(), inputs.end());


nit: cbegin, cend

tlemo · 2020-08-05T17:45:47Z

+  return out;
+}
+#else
+template <typename FilterType, typename InputIt>


InputIt -> IteratorType or just Iterator ?

Renamed to Iterator.

tlemo · 2020-08-05T17:47:11Z

+  }
+
+ private:
+  InputIt input_it_;


please separate the data members from private functions into separate private sections

tlemo · 2020-08-05T17:51:24Z

+
+ private:
+  InputIt input_it_;
+  InputIt last_;


rename input_it_ -> current_ ?

as I mentioned in another comment, we should rename last_ -> end_

tlemo · 2020-08-05T17:56:43Z

+  }
+
+  bool operator==(const FilterIterator& other) const {
+    return input_it_ == other.input_it_ && last_ == other.last_;


I'd actually just TORCH_INTERNAL_ASSERT(last_ == other.last_) - if someone is trying to compare fitered iterators coming from different filtered views it's most likely an error and we should raise it as such

That's a good idea. Done.

We eventually do casting so dynamic_cast-based type checking seems more appropriate

naoyam · 2020-08-05T19:49:46Z

I actually changed the type checking to use dynamic_cast instead of getValType() as it eventually does dynamic_cast. I think it makes more sense.

jjsjann123

LGTM

tlemo · 2020-08-05T22:20:47Z

+    return (*current_)->template as<FilterType>();
+  }
+
+  FilterType* operator->() const {


nice, good catch!

tlemo · 2020-08-05T22:21:26Z

+  }
+
+  FilterIterator operator++(int) {
+    auto before_increment = *this;


nit: const ?

tlemo · 2020-08-05T22:22:41Z

+    return !(*this == other);
+  }
+
+ private:


please place the private state section last (public methods, private methods, private state)

naoyam · 2020-08-05T23:08:56Z

+    This doesn't compile
+  ../test/cpp/jit/test_gpu.cpp:800:51: error: no matching function for call to ‘std::vector<torch::jit::fuser::TensorView*>::vector(torch::jit::fuser::ir_utils::FilteredView<torch::jit::fuser::TensorView, __gnu_cxx::__normal_iterator<torch::jit::fuser::Val* const*, std::vector<torch::jit::fuser::Val*> > >::const_iterator, torch::jit::fuser::ir_utils::FilteredView<torch::jit::fuser::TensorView, __gnu_cxx::__normal_iterator<torch::jit::fuser::Val* const*, std::vector<torch::jit::fuser::Val*> > >::const_iterator)’
+       ir_utils::filterVals<TensorView>(vals).end());
+                                                   ^
+In file included from /usr/include/c++/8/vector:64,
+                 from ../c10/util/StringUtil.h:11,
+                 from ../c10/util/Exception.h:5,
+                 from ../c10/core/Device.h:5,
+                 from ../c10/core/Allocator.h:6,
+                 from ../aten/src/ATen/ATen.h:7,
+                 from ../torch/csrc/jit/ir/attributes.h:2,
+                 from ../torch/csrc/jit/ir/ir.h:3,
+                 from ../test/cpp/jit/test_base.h:5,
+                 from ../test/cpp/jit/test_gpu.cpp:3:
+/usr/include/c++/8/bits/stl_vector.h:543:2: note: candidate: ‘template<class _InputIterator, class> std::vector<_Tp, _Alloc>::vector(_InputIterator, _InputIterator, const allocator_type&)’
+  vector(_InputIterator __first, _InputIterator __last,
+  */
+#if 0
+  std::vector<TensorView*> tvs(
+      ir_utils::filterVals<TensorView>(vals).begin(),
+      ir_utils::filterVals<TensorView>(vals).end());


@tlemo Do you know why this doesn't work?

Look at constructor (5) in https://en.cppreference.com/w/cpp/container/vector/vector
Your const_iterator needs to satisfy LegacyInputIterator... So you would need difference_type and bunch of that...

tlemo · 2020-08-06T00:20:28Z

+  TORCH_CHECK(ints.size() == 2);
+  TORCH_CHECK(ints[0] == scalar1);
+  TORCH_CHECK(ints[1] == scalar2);
+  return;


nit: return not needed

tlemo · 2020-08-06T00:21:38Z

+  auto scalar1 = new Int(0);
+  auto scalar2 = new Int(1);
+
+  std::vector<Val*> vals = {tv0, scalar0, tv1, scalar1, scalar2};


can you please make vals const?

tlemo · 2020-08-06T00:23:30Z

+  TORCH_CHECK(floats.size() == 1);
+  TORCH_CHECK(floats[0] == scalar0);
+
+  std::vector<Int*> ints(


one more test case please: a filtering which returns an empty list, ex:

for (auto ptr : ir_utils::filterVals<Expr>(vals)) { TORCH_CHECK(false, "Not expecting any results"); }

naoyam · 2020-08-06T00:46:58Z

I renamed the filter function to filterByType as it is actually no longer specific to Val. It just checks the runtime type of each element and filters out anything that is of that type.

The example I gave first still looks mostly the same. We replace this:

std::vector<Val*> values;
for (Val* x: values) {
  // do something only for TensorView vals
  if (x->getValType() == ValType::TensorView) {
   TensorView* x_tv = x->as<TensorView>();
   ...
  }
}

with:

for (TensorView* x: filterByType<TensorView>(values)) {
  // x is TensorView*
 ... 
}

naoyam · 2020-08-06T01:01:24Z

@tlemo Sorry, I missed your last comments. I'll update the test with a separate PR.

…ty. (#267) (pytorch#89315) Summary: pytorch#89122 introduces internal compatibility issues with torchdeploy. However, GetPythonFramesFunction() never worked with torchdeploy, so this PR simply reverts to the original behavior of skipping the function if torchdeploy is used as a forward fix. Test Plan: Running failed tests in T128123281 ``` buck2 test @//mode/opt //multipy/runtime:test_deploy -- --exact 'multipy/runtime:test_deploy - TorchpyTest.TaggingRace' --run-disabled buck2 test mode/dev //multipy/runtime/testdev:test_deploy_from_python -- --exact 'multipy/runtime/testdev:test_deploy_from_python - multipy.runtime.testdev.test_deploy_from_python.TestDeployFromPython: test_deploy_from_python' ``` Differential Revision: D41414263 Pull Request resolved: pytorch#89315 Approved by: https://github.com/kurman

Naoya Maruyama added 2 commits August 4, 2020 16:23

Add a utility function to filter out elements that are not of a certain

7cf8c5f

type

Fix clang-tidy

f7b613d

tlemo reviewed Aug 5, 2020

View reviewed changes

Comment thread torch/csrc/jit/codegen/cuda/ir_utils.h Outdated

Naoya Maruyama added 2 commits August 4, 2020 21:55

Custom container-based version

7b7c7d7

clang-format

7925794

jjsjann123 reviewed Aug 5, 2020

View reviewed changes

tlemo reviewed Aug 5, 2020

View reviewed changes

Remove deprecated code

fe73373

tlemo reviewed Aug 5, 2020

View reviewed changes

Comment thread torch/csrc/jit/codegen/cuda/ir_utils.h

Naoya Maruyama added 6 commits August 5, 2020 11:25

review feedback

9726ee6

Add a comment on non-const iterator

87441d4

Use find_if

d0dfc7e

Add more overloads and switch to use dynamic_cast instead of the tag

9e2e0e3

We eventually do casting so dynamic_cast-based type checking seems more appropriate

Use filterVals

f8c3b0d

Remove static type field

b322c4d

jjsjann123 approved these changes Aug 5, 2020

View reviewed changes

naoyam marked this pull request as ready for review August 5, 2020 20:11

tlemo approved these changes Aug 5, 2020

View reviewed changes

Naoya Maruyama added 2 commits August 5, 2020 15:55

cleanup based on review feedback

89bc6c3

WIP: unit test

9a2bf32

naoyam commented Aug 5, 2020

View reviewed changes

Naoya Maruyama added 2 commits August 5, 2020 16:44

Expand the unit test

451cdfb

Make FilterIterator compatible as std::forward_iterator

f166eb9

tlemo approved these changes Aug 6, 2020

View reviewed changes

naoyam changed the title ~~[WIP] Add a utility function to filter out elements that are not of a certain type~~ Add a utility function to filter out elements that are not of a certain type Aug 6, 2020

Rename filterVals to filterByType as it is no longer specific to Val

d870273

naoyam merged commit 1850edd into csarofeen:20_7_6_devel Aug 6, 2020

naoyam mentioned this pull request Aug 6, 2020

Minor follow up update #271

Merged

Conversation

naoyam commented Aug 4, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jjsjann123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naoyam commented Aug 5, 2020

Uh oh!

jjsjann123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!