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

Add a sync feature to common, core, and tensor #893

Merged
merged 7 commits into from
Oct 24, 2023

Conversation

AlexErrant
Copy link
Contributor

Pull Request Template

Checklist

  • [?] Confirm that run-checks script has been executed.

Failed with the same error as last time, more or less.

Related Issues/PRs

Resolves #865.

Continuing the discussion from the above issue: After a truly embarrassing amount of spinning in circles, I don't think I can get Asyncify to work with wgpu. WGPU is, at its foundation, async, and critical apis don't have a synchronous version. Asyncify only works on wasm files. WGPU is async Rust, so asyncify can't "de-async" it for our consumption.

Changes

So instead I pull the same trick as last time and feature flag stuff. I add a sync feature that I can use to get sync behavior when targeting WASM.

Testing

Add

burn = {path = "../../burn", default-features = false, features = ["train-minimal", "ndarray-no-std", "sync"]}
getrandom = { version = "0.2", features = ["js"] }

to https://github.com/burn-rs/burn/blob/main/examples/mnist-inference-web/Cargo.toml, delete this await, and observe that ./build-for-web.sh ndarray builds.

@AlexErrant AlexErrant marked this pull request as draft October 23, 2023 22:48
@AlexErrant
Copy link
Contributor Author

Maybe this'll fix CI?

Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

Maybe we should rename the feature flag to something like wasm-sync since it exclusively modifies the behavior when targeting wasm. Another option is to introduce a feature flag called async, but it could be misleading, as most of the execution relies on asynchronous operations using channels.

Just the name sync is also misleading, since even when not enabled, the APIs are sync when not targeting wasm.

burn/Cargo.toml Outdated
Comment on lines 63 to 64
burn-common = { path = "../burn-common", version = "0.10.0", optional = true, default-features = false }
burn-tensor = { path = "../burn-tensor", version = "0.10.0", optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

I think I would use burn-core to pass the sync feature flag to those crates instead of adding them to burn.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@38e88a7). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #893   +/-   ##
=======================================
  Coverage        ?   84.99%           
=======================================
  Files           ?      469           
  Lines           ?    44443           
  Branches        ?        0           
=======================================
  Hits            ?    37773           
  Misses          ?     6670           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nathanielsimard nathanielsimard marked this pull request as ready for review October 24, 2023 18:31
@nathanielsimard nathanielsimard merged commit 9f2bc59 into tracel-ai:main Oct 24, 2023
8 checks passed
@AlexErrant
Copy link
Contributor Author

Oh hah I did not expect that to fix CI.

@AlexErrant AlexErrant deleted the wasmsync branch October 24, 2023 18:40
@nathanielsimard
Copy link
Member

@AlexErrant It was a setting problem, so just a trigger would fix it!

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.

train-minimal doesn't work on wasm after async read
2 participants