Conversation
[ghstack-poisoned]
🔗 Helpful links
✅ No Failures (1 Pending)As of commit 48e1e46 (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
torch/_subclasses/fake_tensor.py
Outdated
| try: | ||
| yield | ||
| finally: | ||
| cpu_fallback_enabled = orig |
There was a problem hiding this comment.
Why not have this on the fake tensor mode itself?
There was a problem hiding this comment.
I guess... is this something that we view as temporary? If so, it might be nice not to commit it to the FakeTensorMode api. But I guess it's not that different either way.. I can move into the Mode
There was a problem hiding this comment.
Also there’s no clear way for that to interact with FakeTensors that are constructed from FakeTensor.from_tensor
There was a problem hiding this comment.
It is simpler and organizationally better for it to live on the mode, so yes I'd prefer it!
Re how tensors can get to it, that would be done by storing the mode on the tensor!
There was a problem hiding this comment.
One thing I don't love about storing the Mode on the Tensor is that you're creating a circular reference from
FakeTensor(a) -> FakeTensorMode -> FakeTensorConverter -> input Tensor -> FakeTensor(a). (cc @samdow )
There was a problem hiding this comment.
Converter should have a weak ref to the input tensor
torch/_subclasses/fake_tensor.py
Outdated
| r = run_function(func, types, args, kwargs) | ||
| except NotImplementedError as not_implemented_error: | ||
| if not cpu_fallback_enabled: | ||
| raise not_implemented_error |
There was a problem hiding this comment.
raise here is better as doing it this way will destroy the original backtrace info
There was a problem hiding this comment.
sorry, how is this different from one i'm doing now ?
torch/_subclasses/fake_tensor.py
Outdated
| r = func(*args , **kwargs) | ||
| except Exception: | ||
| # original error more orinformative | ||
| raise orig_not_implemented_exception |
There was a problem hiding this comment.
I think this will impede debugging. I suggest raising a fresh exception here but from orig_exception
There was a problem hiding this comment.
What does raising a fresh exception here but from orig_exception entail ?
There was a problem hiding this comment.
hmm I didn't think it through. How about raise orig_not_implemented_exception from e where e is the exception you caught here
torch/_subclasses/fake_tensor.py
Outdated
| if e in tensor_impls: | ||
| raise orig_not_implemented_exception | ||
|
|
||
| tree_map(throw_on_reused_impls, r) |
There was a problem hiding this comment.
This seems very insufficient and also the case being tested for here feels like it can be handled.
It's insufficient in that this fallback will not work for view operations, and you won't capture errors here because view operators will return a fresh tensor with shared storage. Testing for shared storage is better.
It's handleable in that if an output of the cpu operation is exactly the same as the input, we know we can just return the original meta in that case and it will be OK.
There was a problem hiding this comment.
if one were to have an operator which both
a) is unsupported on meta
and
b) changes the input metadata
then just returning the input meta value would not be sufficient
There was a problem hiding this comment.
Yes. But you can also have an operator that doesn't return the input but does mutate the metadata and you have no way of detecting this. Well, tags would help.
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
This adds the option to convert tensors to fallback to cpu if the operator is not supported on `meta` tensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs. [ghstack-poisoned]
|
@pytorchbot merge |
|
@pytorchbot successfully started a merge job. Check the current status here |
|
Hey @eellison. |
ghstack-source-id: 85603cf Pull Request resolved: pytorch#78522
Summary: Pull Request resolved: #78522 Approved by: https://github.com/ezyang Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/fe7a13496eb87c620ec404a0c37fb243bdbc7c01 Reviewed By: osalpekar Differential Revision: D37025735 Pulled By: eellison fbshipit-source-id: 806330725980fece8d43650d9fdeb8eb6003ffd7
Stack from ghstack (oldest at bottom):
This adds the option to convert tensors to fallback to cpu if the operator is not supported on
metatensors. The output of the cpu fallback operator must be a new-unaliased TensorImpl because the conversion to cpu and back will lose shared metadata between inputs and outputs.