Skip to content

Extend support for scalars and scalar lists in Value class #2271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

SS-JIA
Copy link
Contributor

@SS-JIA SS-JIA commented Mar 5, 2024

Summary:

Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the Value class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors VulkanBackend.cpp by introducing GraphBuilder which makes constructing a ComputeGraph from a serialized flatbuffer much clearer.

Differential Revision: D54561567

Copy link

pytorch-bot bot commented Mar 5, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/2271

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 1 Unrelated Failure

As of commit 066f119 with merge base c0167c5 (image):

NEW FAILURE - The following job has failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 5, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

SS-JIA added a commit to SS-JIA/executorch-1 that referenced this pull request Mar 6, 2024
)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Differential Revision: D54561567
@SS-JIA SS-JIA force-pushed the export-D54561567 branch from 352f6ce to 9d7b682 Compare March 6, 2024 03:35
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

SS-JIA added a commit to SS-JIA/executorch-1 that referenced this pull request Mar 6, 2024
)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Differential Revision: D54561567
@SS-JIA SS-JIA force-pushed the export-D54561567 branch from 9d7b682 to 6365deb Compare March 6, 2024 03:36
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

SS-JIA added a commit to SS-JIA/executorch-1 that referenced this pull request Mar 6, 2024
)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Differential Revision: D54561567
@SS-JIA SS-JIA force-pushed the export-D54561567 branch from 6365deb to 29b3792 Compare March 6, 2024 03:37
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

SS-JIA added a commit to SS-JIA/executorch-1 that referenced this pull request Mar 6, 2024
)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Differential Revision: D54561567
@SS-JIA SS-JIA force-pushed the export-D54561567 branch from 29b3792 to eaae93b Compare March 6, 2024 03:37
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

SS-JIA added a commit to SS-JIA/executorch-1 that referenced this pull request Mar 6, 2024
)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Differential Revision: D54561567
@SS-JIA SS-JIA force-pushed the export-D54561567 branch from eaae93b to 65bd2b4 Compare March 6, 2024 15:41
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Reviewed By: jorgep31415

Differential Revision: D54561567
SS-JIA added a commit to SS-JIA/executorch-1 that referenced this pull request Mar 6, 2024
)

Summary:

## Context

This changeset enables serialization and execution of Operators with arbitrary function signatures. Previously, only operators with a very specific schema were supported (2 inputs, 1 output).

This is achieved by extending the `Value` class (which is essentially a tagged union) to support all necessary types. All objects needed to execute an operator are now serialized/deserialized as a tagged union.

This changeset also refactors `VulkanBackend.cpp` by introducing `GraphBuilder` which makes constructing a `ComputeGraph` from a serialized flatbuffer much clearer.

Reviewed By: jorgep31415

Differential Revision: D54561567
@SS-JIA SS-JIA force-pushed the export-D54561567 branch from 65bd2b4 to 28c8554 Compare March 6, 2024 19:13
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

@SS-JIA SS-JIA force-pushed the export-D54561567 branch from 28c8554 to 066f119 Compare March 6, 2024 19:14
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D54561567

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 12fcfcf.

jorgep31415 added a commit that referenced this pull request Mar 13, 2024
In #2271, we already added
- IntList
- DoubleList
- BoolList
- ValueList

to the schema and the runtime's Value class. Their serialization was incomplete missing two components:
1. Receiving a list in `torch.fx.Node.args`.
2. Receiving a non-tensor in `torch.fx.Node`.

This change completes #1.


Also, this change fixes a bug where values type `bool` matches both types `bool` and `int` and hence were being added twice.

If our type support grows more complex, we can consider using our own types similar to the core Executorch runtime: https://github.com/pytorch/executorch/blob/689796499024fc4a133318d707f4c10db73da967/exir/emit/_emitter.py#L158-L166

Differential Revision: [D54708353](https://our.internmc.facebook.com/intern/diff/D54708353/)

[ghstack-poisoned]
jorgep31415 added a commit that referenced this pull request Mar 13, 2024
In #2271, we already added
- IntList
- DoubleList
- BoolList
- ValueList

to the schema and the runtime's Value class. Their serialization was incomplete missing two components:
1. Receiving a list in `torch.fx.Node.args`.
2. Receiving a non-tensor in `torch.fx.Node`.

This change completes #2.

Also, we introduce a specific handler for `getitem` nodes as it is required. Every `function_call` outputting non-tensor `torch.fx.Node` is followed by a special `getitem` `function_call`.

Differential Revision: [D54789099](https://our.internmc.facebook.com/intern/diff/D54789099/)

[ghstack-poisoned]
jorgep31415 added a commit that referenced this pull request Mar 13, 2024
In #2271, we already added
- IntList
- DoubleList
- BoolList
- ValueList

to the schema and the runtime's Value class. Their serialization was incomplete missing two components:
1. Receiving a list in `torch.fx.Node.args`.
2. Receiving a non-tensor in `torch.fx.Node`.

This change completes #2.

Also, we introduce a specific handler for `getitem` nodes as it is required. Every `function_call` outputting non-tensor `torch.fx.Node` is followed by a special `getitem` `function_call`.

Differential Revision: [D54789099](https://our.internmc.facebook.com/intern/diff/D54789099/)

ghstack-source-id: 218541429
Pull Request resolved: #2405
facebook-github-bot pushed a commit that referenced this pull request Mar 13, 2024
Summary:
bypass-github-export-checks

Pull Request resolved: #2404

In #2271, we already added
- IntList
- DoubleList
- BoolList
- ValueList

to the schema and the runtime's Value class. Their serialization was incomplete missing two components:
1. Receiving a list in `torch.fx.Node.args`.
2. Receiving a non-tensor in `torch.fx.Node`.

This change completes #1.

Also, this change fixes a bug where values type `bool` matches both types `bool` and `int` and hence were being added twice.

If our type support grows more complex, we can consider using our own types similar to the core Executorch runtime: https://github.com/pytorch/executorch/blob/689796499024fc4a133318d707f4c10db73da967/exir/emit/_emitter.py#L158-L166
ghstack-source-id: 218539049
exported-using-ghexport

Reviewed By: SS-JIA

Differential Revision: D54708353

fbshipit-source-id: 8641647b515e201ea63db67115c01c1532ad6566
facebook-github-bot pushed a commit that referenced this pull request Mar 13, 2024
Summary:
bypass-github-export-checks

Pull Request resolved: #2405

In #2271, we already added
- IntList
- DoubleList
- BoolList
- ValueList

to the schema and the runtime's Value class. Their serialization was incomplete missing two components:
1. Receiving a list in `torch.fx.Node.args`.
2. Receiving a non-tensor in `torch.fx.Node`.

This change completes #2.

Also, we introduce a specific handler for `getitem` nodes as it is required. Every `function_call` outputting non-tensor `torch.fx.Node` is followed by a special `getitem` `function_call`.
ghstack-source-id: 218541429
exported-using-ghexport

Reviewed By: SS-JIA

Differential Revision: D54789099

fbshipit-source-id: 1e58cbc0246a8c651d95e9ef6707bacb60066e2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants