Skip to content

Fix OpenBLAS pow_dd unresolved symbol error, update Emscripten CI testing#9343

Merged
bashtage merged 11 commits intostatsmodels:mainfrom
agriyakhetarpal:emscripten-ci-updates
Sep 3, 2024
Merged

Fix OpenBLAS pow_dd unresolved symbol error, update Emscripten CI testing#9343
bashtage merged 11 commits intostatsmodels:mainfrom
agriyakhetarpal:emscripten-ci-updates

Conversation

@agriyakhetarpal
Copy link
Contributor

@agriyakhetarpal agriyakhetarpal commented Sep 3, 2024 •
edited
Loading

  • closes #xxxx
  • tests added / passed.
  • code/documentation is well formatted.
  • properly formatted commit message. See
    NumPy's guide.

Description

This PR follows up on changes made in #9270. Here's a small, point-wise summary noting the changes:

  • It updates the Emscripten/Pyodide CI job added in Add Pyodide support and CI jobs for statsmodels #9270 and removes the workaround JavaScript test suite file, since we fixed the pow_dd OpenBLAS unresolved symbol coming from SciPy upstream, having updated SciPy in-tree.
  • The Pyodide xbuildenv can now be used to set up a virtual environment to run the tests in the same way as they were running before.
  • The pyodide-build tool has been unvendored from the Pyodide repository and now infers the Pyodide version from the releases.
  • At the time of writing, Pyodide 0.27alpha2 is used in the CI job; once 0.27 stable comes out and cibuildwheel updates to it, we can update cibuildwheel in Build and publish Pyodide wheels MacPython/statsmodels-wheels#161 and run the test suite there (if it makes sense to do so – since the test suite is on the slower side without parallelization).
  • The tests for the "Tukey's HSD" statistical test that were previously marked as xfailing due to issues with SciPy (more specifically, scipy.optimize) have been restored.

Additional context

Details

Notes:

  • It is essential that you add a test when making code changes. Tests are not
    needed for doc changes.
  • When adding a new function, test values should usually be verified in another package (e.g., R/SAS/Stata).
  • When fixing a bug, you must add a test that would produce the bug in main and
    then show that it is fixed with the new code.
  • New code additions must be well formatted. Changes should pass flake8. If on Linux or OSX, you can
    verify you changes are well formatted by running 
    git diff upstream/main -u -- "*.py" | flake8 --diff --isolated
    assuming flake8 is installed. This command is also available on Windows
    using the Windows System for Linux once flake8 is installed in the
    local Linux environment. While passing this test is not required, it is good practice and it help
    improve code quality in statsmodels.
  • Docstring additions must render correctly, including escapes and LaTeX.

This was referenced Sep 3, 2024
Open
Open
@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Sep 3, 2024 •
edited
Loading

test_tukeyhsd is displayed as failing, but it's a strict xpass. I'm not sure why the behaviour differs in the xbuildenv venv versus the Node.js runner. I restored it in 62ceeba. Most probably, updates to SciPy in-tree have fixed it (see pyodide/pyodide#4719, pyodide/pyodide#5012, pyodide/pyodide#5031, and more).

@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Sep 3, 2024

On that note, maybe the rest of the Tukey HSD tests in test_pairwise.py now work, too. I'll undo those skips/xfails and check.

@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Sep 3, 2024

Addressed in 316ca4d, the only failing tests under WASM in the entirety of the statsmodels test suite should now be those where threading/multiprocessing isn't available and the Matplotlib tests (which use a different backend). This is now ready for review, @bashtage, thank you!

@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Sep 3, 2024

Azure macOS test errors seem unrelated.

@bashtage
Copy link
Member

bashtage commented Sep 3, 2024

Agree. It is a random failure.

@bashtage bashtage merged commit 8c3fd00 into statsmodels:main Sep 3, 2024
@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Sep 3, 2024

Thanks for the speedy review and merge! :)

This was referenced Sep 4, 2024
Open
Merged
@agriyakhetarpal agriyakhetarpal deleted the emscripten-ci-updates branch September 5, 2024 21:44
@agriyakhetarpal agriyakhetarpal mentioned this pull request Sep 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@agriyakhetarpal @bashtage