Skip to content
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

Specify MLTensor #787

Merged
merged 11 commits into from
Nov 26, 2024
Merged

Specify MLTensor #787

merged 11 commits into from
Nov 26, 2024

Conversation

a-sully
Copy link
Contributor

@a-sully a-sully commented Nov 15, 2024 •
edited by pr-preview bot
Loading

This PR specifies key methods related to MLTensor, while hand-waving over much of the juicy details, such as how to actually specify the MLContext's timeline (#529)

My hope is that this is a mostly-straightforward improvement which is "good enough" to allow us to remove MLContext.compute() as well as providing a more concrete starting point for more detailed discussions about e.g. specifying timelines in follow-up PRs

Preview | Diff

@a-sully
Copy link
Contributor Author

a-sully commented Nov 15, 2024 •
edited
Loading

There's one remaining trivial bikeshed error which I intend to fix tomorrow. Please don't let that stop you from reviewing this PR :)

Thanks!

Update: This error is now fixed 🟢

a-sully
a-sully commented Nov 15, 2024
View reviewed changes
index.bs
::
Whether {{MLTensor}}.{{MLTensor/destroy()}} has been called. Once destroyed, the {{MLTensor}} can no longer be used.

: <dfn>\[[data]]</dfn> of an [=implementation-defined=] type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered making this a Data Block but I haven't fully thought through the implications of that yet, so... I went for maximal hand-waviness for now

https://tc39.es/ecma262/multipage/ecmascript-data-types-and-values.html#sec-data-blocks

inexorabletash
inexorabletash reviewed Nov 15, 2024
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@a-sully
Copy link
Contributor Author

a-sully commented Nov 16, 2024

Apologies for the accidental force-push 🤦

Thanks for the feedback @inexorabletash! I believe I've addressed all your comments

inexorabletash
inexorabletash reviewed Nov 16, 2024
View reviewed changes
Copy link
Member

@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.

Thanks! Most updates LGTM.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
huningxin
huningxin reviewed Nov 18, 2024
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.

Thanks @a-sully !

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
huningxin
huningxin reviewed Nov 19, 2024
View reviewed changes
index.bs Show resolved Hide resolved
@huningxin huningxin mentioned this pull request Nov 19, 2024
Merged
@anssiko anssiko mentioned this pull request Nov 20, 2024
Open
huningxin
huningxin reviewed Nov 21, 2024
View reviewed changes
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
huningxin
huningxin approved these changes Nov 22, 2024
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!

@a-sully
Copy link
Contributor Author

a-sully commented Nov 22, 2024

Thanks for the review! Feel free to merge at your convenience 🙏

@huningxin huningxin requested a review from fdwr November 22, 2024 06:07
@huningxin
Copy link
Contributor

@inexorabletash @reillyeon Any further comments? Also add @fdwr , PTAL.

inexorabletash
inexorabletash reviewed Nov 22, 2024
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@a-sully a-sully mentioned this pull request Nov 22, 2024
Closed
inexorabletash
inexorabletash approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@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! (I can't mark comment threads as resolved)

@a-sully a-sully mentioned this pull request Nov 22, 2024
Open
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 23, 2024
@a-sully
webnn: Use USVString as key for MLNamedTensors
134e98d 
See discussion at:
webmachinelearning/webnn#787 (comment)

Change-Id: I9b00c041b2a12e72d4a7147d6b31dc4ad02d8b87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6044441
Auto-Submit: Austin Sullivan <[email protected]>
Reviewed-by: Joshua Bell <[email protected]>
Commit-Queue: Joshua Bell <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1387182}
fdwr
fdwr reviewed Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@fdwr fdwr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Minor comments, else LGTM. Thanks for including in the spec.

@RafaelCintron, any thoughts? Rafael skimmed and messaged me.

index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
@fdwr fdwr merged commit dd5cbdc into webmachinelearning:main Nov 26, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 26, 2024
@a-sully @github-actions
Specify MLTensor (#787)
b50be14 
SHA: dd5cbdc
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@a-sully a-sully deleted the mltensor-spec branch November 26, 2024 23:37
@a-sully a-sully mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inexorabletash inexorabletash inexorabletash approved these changes

@reillyeon reillyeon reillyeon left review comments

@fdwr fdwr fdwr approved these changes

@huningxin huningxin huningxin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@a-sully @huningxin @reillyeon @inexorabletash @fdwr