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.

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

Remove the MLContext.compute() method #795

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

a-sully
Copy link
Contributor

@a-sully a-sully commented Nov 27, 2024

Fixes #791. Made possible by #787 which specified dispatch()

The changes proposed in #786 have also been rolled into this PR, since they can't really be decoupled from compute() removal at this point

I've tried to keep this PR minimal and focused on removing algorithms and IDL definitions which are no longer relevant. The spec still contains lots of normative text which was written with compute() in mind which doesn't map as well to dispatch() - specifically text which contains assumptions about how graph execution relates to compute devices. I've lightly updated some of this text where necessary to match the dispatch() paradigm, but my intention is to avoid opening the can of worms that is #749 in this PR :)


Preview | Diff

index.bs Show resolved Hide resolved
@a-sully
Copy link
Contributor Author

a-sully commented Nov 27, 2024

@huningxin @fdwr PTAL?

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

This does make me realize that as part of #529 we'll likely need to define queuing for operations on the timeline that share tensors, to avoid racy behavior. With ArrayBuffers transferred away synchronously during compute(), there wasn't the possibility for multiple compute()s to compete for access to the buffer or script to read/write the contents.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
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 with a comment to example, thanks!

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, thanks!

@a-sully
Copy link
Contributor Author

a-sully commented Dec 2, 2024

Thanks for the reviews, @huningxin and @inexorabletash

@fdwr PTAL?

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.

👍 LGTM Mr. Sully.

@fdwr fdwr merged commit 4e80205 into webmachinelearning:main Dec 2, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Dec 2, 2024
SHA: 4e80205
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@a-sully a-sully deleted the delete-compute branch December 2, 2024 19:52
caugner pushed a commit to mdn/browser-compat-data that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove MLContext.compute()
4 participants