Skip to content

Java Tensor and EValue serialization #6620

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 21, 2024
Merged

Java Tensor and EValue serialization #6620

merged 2 commits into from
Nov 21, 2024

Conversation

kirklandsign
Copy link
Contributor

@kirklandsign kirklandsign commented Nov 1, 2024 •
edited
Loading

Add serialization and deserialization for EValue (except string) and Tensor.

Summary

Fixes #6569

Test plan

cd extension/android_test/
./gradlew build

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Nov 1, 2024 •
edited
Loading

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

❌ 7 New Failures

As of commit 8750e17 with merge base f40daea (image):

NEW FAILURES - The following jobs have failed:

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 Nov 1, 2024
@kirklandsign kirklandsign force-pushed the android-java branch 6 times, most recently from c61a10b to 74be7a5 Compare November 21, 2024 01:27
@kirklandsign
Java Tensor and EValue serialization
44283a2 
Add serialization and deserialization for EValue (except string) and
Tensor.

RFC: #6569
@kirklandsign kirklandsign added the release notes: misc Miscellaneous label Nov 21, 2024
davidlin54
davidlin54 reviewed Nov 21, 2024
View reviewed changes
extension/android/src/main/java/org/pytorch/executorch/EValue.java
.put((byte) TYPE_CODE_STRING)
.put(toString().getBytes())
.array();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will we add list support as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Jacob, list is an internal dtype within ET runtime. Maybe we should totally get rid of list in java layer. I could double check with the team

davidlin54
davidlin54 reviewed Nov 21, 2024
View reviewed changes
extension/android/src/main/java/org/pytorch/executorch/Tensor.java Outdated
int dtypeSize = 0;
byte[] tensorAsByteArray = null;
if (dtype() == DType.FLOAT) {
dtypeSize = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you reference the consts on L56 and add the missing ones? (uint8, int8)

davidlin54
davidlin54 reviewed Nov 21, 2024
View reviewed changes
extension/android/src/main/java/org/pytorch/executorch/Tensor.java Outdated
Comment on lines 755 to 756
byte scalarType = buffer.get();
byte numberOfDimensions = buffer.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use same constants as the serialize function? dtype and shapeLength?

davidlin54
davidlin54 reviewed Nov 21, 2024
View reviewed changes
extension/android_test/src/test/java/org/pytorch/executorch/EValueTest.java Outdated
Comment on lines 25 to 30
private static final int TYPE_CODE_NONE = 0;
private static final int TYPE_CODE_TENSOR = 1;
private static final int TYPE_CODE_STRING = 2;
private static final int TYPE_CODE_DOUBLE = 3;
private static final int TYPE_CODE_INT = 4;
private static final int TYPE_CODE_BOOL = 5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can just expose the constants in EValue as public instead of copying this here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really implementation detail and I want to avoid exposing it. Honestly we shouldn't test it as well.

davidlin54
davidlin54 approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@davidlin54 davidlin54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could consider using switch statements instead of if, but looks good

@kirklandsign
Copy link
Contributor Author

could consider using switch statements instead of if, but looks good

Will follow up :)

@kirklandsign kirklandsign merged commit 6ac19cc into main Nov 21, 2024
32 of 39 checks passed
@kirklandsign kirklandsign deleted the android-java branch November 21, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidlin54 davidlin54 davidlin54 approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: misc Miscellaneous
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFC: Java layer EValue serialization
3 participants
@kirklandsign @davidlin54 @facebook-github-bot