Remove guard_size_oblivious from default contiguity python check, and add aten.sym_is_contiguous. #159197
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/159197
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit c42ba31 with merge base 316c188 ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This might cause some new DDEs on call sites that do not use is_contiguous_or_false but want to find those call sites to handle this properly by calling is_contiguous_or_false and not is_contiguous [ghstack-poisoned]
| is_nested_int, | ||
| ) | ||
|
|
||
| maybe_guard_or_false = guard_or_false if false_if_dde else guard_size_oblivious | ||
| maybe_guard_or_true = guard_or_true if false_if_dde else guard_size_oblivious | ||
| def eval_eager(x): |
There was a problem hiding this comment.
shall we just do
if false_if_dde:
return torch.ops.aten.sym_is_contiguous.default(input).guard_or_false here?
make debugging harder, but make impl simpler.
| @@ -313,7 +313,7 @@ void TensorImpl::throw_data_ptr_access_error() const { | |||
| c10::SymBool TensorImpl::sym_is_contiguous_custom( | |||
| at::MemoryFormat memory_format) const { | |||
| if (C10_UNLIKELY(matches_python_custom(SizesStridesPolicy::CustomStrides))) { | |||
| return pyobj_slot_.load_pyobj_interpreter()->is_contiguous( | |||
There was a problem hiding this comment.
calling this causes DDE. we need to delegate to sym_is_contiguous and define it in python for those that overrides it
| - func: sym_is_contiguous(Tensor self, MemoryFormat memory_format=contiguous_format) -> SymBool | ||
| variants: function | ||
| device_check: NoCheck | ||
| device_guard: False | ||
| tags: core | ||
| manual_cpp_binding: True |
There was a problem hiding this comment.
The operator seems fine to me. But:
- Why are there changes to pyinterpreter?
- Do you have more context over the how this solves the problem? It seems like there should have been a design somewhere
There was a problem hiding this comment.
ok some context before this PR i will add that to the summary.
we defined in this PR sym_is_contiguous to be the function computing contiguity for dynamic shapes. It returns a symbolic expression that represents contiguity and guaranteed not to throw a DDE.
when people call is_contiguous we do sym_is_contiguous().guard_bool()
when people call is_contiguous_or_false we do sym_is_contiguous().guard_or_false()
one issue not handled well was this path
c10::SymBool TensorImpl::sym_is_contiguous_custom(
at::MemoryFormat memory_format) const {
if (C10_UNLIKELY(matches_python_custom(SizesStridesPolicy::CustomStrides))) {
return pyobj_slot_.load_pyobj_interpreter()->is_contiguous(
this, memory_format);
}
return sym_is_contiguous_default(memory_format);
}
namely if we call sym_is_contiguous_custom but we have matches_python_custom(SizesStridesPolicy::CustomStrides) return true , then we used to call is_contiguous(this, memory_format);
This used to go through the load_pyobj_interpreter and end up calling the python is_contiguous call which used implicit size oblivious reasoning.
once we removed that implicit size oblivious reasoning, the right thing we want is to call
return pyobj_slot_.load_pyobj_interpreter()->sym_is_contiguous(this, memory_format);
otherwise we would get DDE even if the caller is doing sym_is_contiguous.
so I had to define it for pyinterpreter, and then I had to override it for nested tensors.
make sense?
Stack from ghstack (oldest at bottom):
This might cause some new DDEs on call sites that do not use is_contiguous_or_false() or sym_is_contiguous()
but want to find those call sites to handle this properly by calling is_contiguous_or_false() and not is_contiguous() explitly when appropriate.
I had to fix one issue after removing the implicit size oblivious reasoning. here is context
we defined in this #157472 sym_is_contiguous to be the function computing contiguity for dynamic shapes in c++. It returns a symbolic expression that represents contiguity and guaranteed not to throw a DDE.
when people call is_contiguous we do sym_is_contiguous().guard_bool()
when people call is_contiguous_or_false we do sym_is_contiguous().guard_or_false()
one issue not handled well was this path
namely if we call sym_is_contiguous_custom but we have matches_python_custom(SizesStridesPolicy::CustomStrides) return true , then we used to call is_contiguous(this, memory_format);
This used to go through the load_pyobj_interpreter and end up calling the python is_contiguous call which used implicit size oblivious reasoning.
once we removed that implicit size oblivious reasoning, the right thing we want is to call
return pyobj_slot_.load_pyobj_interpreter()->sym_is_contiguous(this, memory_format);
otherwise we would get DDE even if the caller is doing sym_is_contiguous.
so I had to define it for pyinterpreter, and then I had to override it for nested tensors.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela