Skip to content

Allow tensors for graph constants. #830

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 8 commits into from
Apr 22, 2025
Merged

Allow tensors for graph constants. #830

merged 8 commits into from
Apr 22, 2025

Conversation

@bbernhar
Copy link
Contributor

@bbernhar bbernhar commented Mar 19, 2025 •
edited by pr-preview bot
Loading

As noted in the issue, sharing weights between graphs is a effective way to enable reuse of device buffers, allowing web developers to manage weight data in graphs similarly to other input or output buffers.

@fdwr @huningxin @reillyeon

Based on the work in Chromium by Austin Sullivan.

Resolves #760

Preview | Diff

inexorabletash
inexorabletash reviewed Mar 19, 2025
View reviewed changes
huningxin
huningxin reviewed Mar 20, 2025
View reviewed changes
@bbernhar
Copy link
Contributor Author

bbernhar commented Mar 20, 2025

Thanks @inexorabletash and @huningxin! Addressed comments.

inexorabletash
inexorabletash reviewed Mar 20, 2025
View reviewed changes
inexorabletash
inexorabletash reviewed Mar 20, 2025
View reviewed changes
inexorabletash
inexorabletash suggested changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just some annoying nitpicks. I ran the lint tool locally over the PR to avoid surprises later.

huningxin
huningxin reviewed Mar 21, 2025
View reviewed changes
@inexorabletash inexorabletash mentioned this pull request Mar 21, 2025
Merged
@bbernhar bbernhar force-pushed the constant_tensor branch 2 times, most recently from 5dae5e3 to 673ac96 Compare March 21, 2025 23:12
@anssiko
Copy link
Member

anssiko commented Mar 25, 2025

(FYI - I enabled the CI workflow for this PR and all the tests passed, including the newly added lint rules from #831)

inexorabletash
inexorabletash reviewed Mar 25, 2025
View reviewed changes
inexorabletash
inexorabletash approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your patience.

reillyeon
reillyeon reviewed Mar 26, 2025
View reviewed changes
fdwr
fdwr approved these changes Mar 27, 2025
View reviewed changes
Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

👍

@reillyeon
Copy link
Member

reillyeon commented Mar 28, 2025

Thank you for the discussion here. I'm starting to come around to the idea of constness being just another property of an MLTensor like readable or writable and the debugging story is compelling. This does make me wonder as Dwayne did about whether it's worthwhile to have a separate interface name. After all, we have similar requirements on readTensor() and writeTensor() for the readable and writable properties without creating additional types.

huningxin
huningxin reviewed Mar 31, 2025
View reviewed changes
bbernhar added 2 commits March 31, 2025 11:51
@bbernhar
Allow tensors for graph constants.
673b9cd 
As noted in the issue, sharing weights between graphs is a effective way
to enable reuse of device buffers, allowing web developers to manage
weight data in graphs similarly to other input or output buffers.

Resolves webmachinelearning#760
@bbernhar
Add [[constantTensor]] slot to check if destroyed before build.
18fb614
fdwr
fdwr reviewed Apr 18, 2025
View reviewed changes
fdwr
fdwr reviewed Apr 18, 2025
View reviewed changes
fdwr
fdwr approved these changes Apr 18, 2025
View reviewed changes
Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

👍

reillyeon
reillyeon approved these changes Apr 18, 2025
View reviewed changes
inexorabletash
inexorabletash approved these changes Apr 18, 2025
View reviewed changes
Copy link
Contributor

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

One tiny nit

@bbernhar
Copy link
Contributor Author

bbernhar commented Apr 21, 2025

@fdwr Feel free to merge when ready. Thanks reviewers for the help.

fdwr
fdwr approved these changes Apr 21, 2025
View reviewed changes
Copy link
Collaborator

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Deferring to Ningxin for final review, approval, and merging.

huningxin
huningxin approved these changes Apr 22, 2025
View reviewed changes
Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM

@huningxin huningxin merged commit d98e86d into webmachinelearning:main Apr 22, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@huningxin huningxin huningxin approved these changes

@fdwr fdwr fdwr approved these changes

+2 more reviewers

@reillyeon reillyeon reillyeon approved these changes

@inexorabletash inexorabletash inexorabletash approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support building graphs from MLTensor containing constants

6 participants

@bbernhar @anssiko @reillyeon @inexorabletash @huningxin @fdwr