GH-49995: [R][Wasm] Fix threading issue under Wasm/Emscripten#49996
GH-49995: [R][Wasm] Fix threading issue under Wasm/Emscripten#49996amoeba wants to merge 22 commits into
Conversation
|
@github-actions crossbow submit test-r-wasm |
|
Revision: dadf0f5 Submitted crossbow builds: ursacomputing/crossbow @ actions-9b0263bfa9
|
|
@github-actions crossbow submit test-r-wasm |
|
With dadf0f5, Parquet reading works with my local test. I'm testing the changes separately so I know what's actually fixing things here. |
|
Revision: abaacb8 Submitted crossbow builds: ursacomputing/crossbow @ actions-510511a107
|
|
I marked this as ready for review. The PR description hopefully explains all that's needed and I put together a demo at https://amoeba.github.io/arrow-r-wasm/ |
| bool CanRunWithCapturedR() { | ||
| #ifdef __EMSCRIPTEN__ | ||
| // Threading is not supported under Emscripten/WASM. Always take the | ||
| // synchronous path to avoid attempting pthread_create which will fail | ||
| // with "thread constructor failed: Not supported". | ||
| return false; | ||
| #else | ||
| return MainRThread::GetInstance().Executor() == nullptr; | ||
| #endif |
There was a problem hiding this comment.
Is it possible to add a test (or maybe it's that we actually need to run the tests under wasm(???) to catch this?
There was a problem hiding this comment.
It looks like maybe we can at least run some R code in the container if not run the test suite. I'll report back.
|
@github-actions crossbow submit test-r-wasm |
|
I'm testing trying to install, load the package, and run the test suite in CI here. I tried testing locally but I'm running into what appears to be some qemu errors since I'm running Docker on Mac so we'll see how this runs in CI. |
|
Revision: 19efaf3 Submitted crossbow builds: ursacomputing/crossbow @ actions-6a9811d919
|
|
@github-actions crossbow submit test-r-wasm |
|
Revision: 47738fa Submitted crossbow builds: ursacomputing/crossbow @ actions-8e088c6ac8
|
|
The job runs but we don't actually run any tests, More soon! |
|
@github-actions crossbow submit test-r-wasm |
|
Revision: 9f03a2a Submitted crossbow builds: ursacomputing/crossbow @ actions-d71cf926ef
|
|
I couldn't find any examples of people running their test suites under Wasm but I'm going to see if I can't get it working in 9f03a2a. If that doesn't work easily, I think it'd be good enough to just write a simple test script to run instead. |
|
That went shockingly well,
Here are the errors/failures: |
984f37e to
3866da3
Compare
| const server = http.createServer((req, res) => { | ||
| const filePath = path.join(repoDir, decodeURIComponent(req.url)); | ||
| if (!filePath.startsWith(path.resolve(repoDir))) { | ||
| res.writeHead(403); | ||
| res.end(); | ||
| return; | ||
| } |
| example_data | ||
| ) | ||
| skip_on_emscripten() | ||
| compare_dplyr_binding( |
| tz_path <- tzdb::tzdb_path("text") | ||
| packageStartupMessage("[configure_tzdb] tzdb path: ", tz_path) | ||
| packageStartupMessage("[configure_tzdb] path exists: ", dir.exists(tz_path)) | ||
| if (dir.exists(tz_path)) { | ||
| tz_files <- list.files(tz_path, recursive = TRUE) |
| bool CanRunWithCapturedR() { return MainRThread::GetInstance().Executor() == nullptr; } | ||
| bool CanRunWithCapturedR() { | ||
| #ifdef __EMSCRIPTEN__ | ||
| // Threading is not supported under Emscripten/WASM. Always take the |
| tz_path <- tzdb::tzdb_path("text") | ||
| packageStartupMessage("[configure_tzdb] tzdb path: ", tz_path) | ||
| packageStartupMessage("[configure_tzdb] path exists: ", dir.exists(tz_path)) | ||
| if (dir.exists(tz_path)) { | ||
| tz_files <- list.files(tz_path, recursive = TRUE) |
| packageStartupMessage( | ||
| "The tzdb package was available but failed to initialize: ", | ||
| e, | ||
| "[configure_tzdb] tzdb package available but failed to initialize: ", | ||
| conditionMessage(e) | ||
| ) |
| packageStartupMessage( | ||
| "The tzdb package is not installed. ", | ||
| "[configure_tzdb] tzdb package is NOT installed. ", | ||
| "Timezones will not be available to Arrow compute functions. ", | ||
| "If you get errors when using Arrow on datetimes, try running ", | ||
| "`install.packages('tzdb')` and trying again." |
| const server = http.createServer((req, res) => { | ||
| const filePath = path.join(repoDir, decodeURIComponent(req.url)); | ||
| if (!filePath.startsWith(path.resolve(repoDir))) { | ||
| res.writeHead(403); | ||
| res.end(); | ||
| return; | ||
| } | ||
| fs.readFile(filePath, (err, data) => { |
| tgz_file=$(ls "${arrow_r_dir}"/arrow_*.tgz 2>/dev/null | head -1) | ||
| if [ -z "${tgz_file}" ]; then | ||
| echo "ERROR: No arrow_*.tgz found in ${arrow_r_dir}" >&2 | ||
| exit 1 | ||
| fi |
| collect(), | ||
| example_data | ||
| ) | ||
| skip_on_emscripten() |
| skip_if_not_available("re2") | ||
| skip_on_emscripten() |
|
@github-actions crossbow submit test-r-wasm |
|
Revision: 4bf5669 Submitted crossbow builds: ursacomputing/crossbow @ actions-4d03e485b0
|
|
@github-actions crossbow submit test-r-wasm |
| packageStartupMessage( | ||
| "The tzdb package was available but failed to initialize: ", | ||
| e, | ||
| "Timezones will not be available to Arrow compute functions." | ||
| ) | ||
| } |
| arrow_r_dir="${1:-.}" | ||
|
|
||
| # Set up a fake CRAN-like repo so we can install the package | ||
| tgz_file=$(ls "${arrow_r_dir}"/arrow_*.tgz 2>/dev/null | head -1) |
| const server = http.createServer((req, res) => { | ||
| const filePath = path.join(repoDir, decodeURIComponent(req.url)); | ||
| if (!filePath.startsWith(path.resolve(repoDir))) { | ||
| res.writeHead(403); | ||
| res.end(); | ||
| return; | ||
| } |
|
@github-actions crossbow submit test-r-wasm |
|
Hey @DerThorsten do you want to take a look at this? You're probably one of the world experts on this :) |
Sure, but not sure if I manage today |
|
There's no urgency :) |
|
@github-actions crossbow submit test-r-wasm |
|
Revision: 9cc1264 Submitted crossbow builds: ursacomputing/crossbow @ actions-83127ccf1f
|
|
@amoeba you might want to rebase on this to get the |
|
Ah, I see you fixed the ncores issue in |
Rationale for this change
#49982 added a basic Wasm build but after doing some testing I ran into an issue with threading.
What changes are included in this PR?
CanRunWithCapturedRso it's always false under Emscripten. This change is the only change that was strictly needed to fix my issue. I'm not a pro with this part of the codebase so scrutiny welcome.options(arrow.use_threads = FALSE)when running under Wasm just in case since it makes sense to not use threads in a Wasm contextAre these changes tested?
Yes. Crossbow jobs show the build works and I have a demo at https://amoeba.github.io/arrow-r-wasm/.
Are there any user-facing changes?
No.