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

ENH: Add OpenBLAS wrap #1734

Merged
merged 1 commit into from
Nov 3, 2024
Merged

ENH: Add OpenBLAS wrap #1734

merged 1 commit into from
Nov 3, 2024

Conversation

HaoZeke
Copy link
Contributor

@HaoZeke HaoZeke commented Oct 5, 2024

OpenBLAS is absolutely massive; but this seems to be a reasonable starting point for seeing wider use. We support a bunch of configurations seen here [1] (but only for dynamic builds, which should be the use case for subproject usage anyway).
The project will eventually be upstreamed if enough adoption takes place, and progress can be tracked on the forked repo from whence this is generated [2].
Tests pass on CI for Windows [3], Linux and MacOS [4]; for a reasonable set of baseline architectures.

This is a squashed single-commit contribution representing commits by me (@HaoZeke), @mtsokol and @rgommers.

Extraction from the branch is done via rsync, basically:

rsync -zarv --prune-empty-dirs --include="*/" --include="meson_options.txt" --exclude="*" $SDIR/ $DDIR
rsync -zarv --prune-empty-dirs --include="*/" --include="meson.build" --exclude="*" $SDIR/ $DDIR
# Remove benchmarks

Along with some manual copies of additional scripts and .c files only needed for the meson build.

As a complex project with a plethora of options, any suggestions are also welcome (maybe best expressed via issues on the fork [2]).

[1] https://github.com/HaoZeke/OpenBLAS/blob/mesonBasic/meson_options.txt
[2] https://github.com/HaoZeke/OpenBLAS/tree/mesonBasic
[3] https://github.com/HaoZeke/OpenBLAS/blob/mesonBasic/.github/workflows/meson.yml
[4] https://github.com/HaoZeke/OpenBLAS/blob/mesonBasic/.github/workflows/meson_linux_darwin.yml

@HaoZeke HaoZeke force-pushed the opblas branch 3 times, most recently from 90f3c4d to e7886a6 Compare October 5, 2024 22:07
@jpakkane
Copy link
Member

jpakkane commented Oct 5, 2024

WARNING: Variable 'openblas_dep' in the subproject 'subprojects/OpenBLAS-0.3.28' is not found

@HaoZeke HaoZeke force-pushed the opblas branch 2 times, most recently from 9846bde to acf7285 Compare October 5, 2024 22:47
@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 5, 2024

WARNING: Variable 'openblas_dep' in the subproject 'subprojects/OpenBLAS-0.3.28' is not found

Sorry, checked via tools/sanity_checks.pynow.

@jpakkane
Copy link
Member

ERROR: Unknown compiler(s): [['gfortran'], ['flang'], ['nvfortran'], ['pgfortran'], ['ifort'], ['ifx'], ['g95']]

You need to edit ci_config.json to add the Debian packages needed to build this package.

@HaoZeke HaoZeke force-pushed the opblas branch 2 times, most recently from 90f0b7d to caae084 Compare October 14, 2024 00:20
@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 21, 2024

Not sure what the deal is with the failures, such an early failures don't show up on my fork..
https://github.com/HaoZeke/wrapdb/actions/runs/11433496876

Will check back to see if it finishes.

@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 28, 2024

@jpakkane I think this should be in better shape now.

@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 28, 2024

Really sorry about that @jpakkane, got a clean bill of health locally now:

ok
test_releases_json (__main__.TestReleases) ... ok

----------------------------------------------------------------------
Ran 3 tests in 103.732s

OK (skipped=1)
(ops_env) 
wrapdb on  opblas [?] via 🅒 ops_env took 1m43s 
➜ ./tools/fake_tty.py ./tools/sanity_checks.py

@jpakkane
Copy link
Member

You probably have to rebase against current master to make the error go away.

@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 28, 2024 via email

@jpakkane
Copy link
Member

Not sure what the issue is with Windows, but the Mac failure should be fixable by adding a Fortran compiler to brew_packages in ci_config.json.

Co-authored-by: mtsokol <[email protected]>
Co-authored-by: rgommers <[email protected]>
@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 28, 2024

Not sure what the issue is with Windows, but the Mac failure should be fixable by adding a Fortran compiler to brew_packages in ci_config.json.

Updated the brew part, I thought that this would skip windows (the visualstudio ones):

  "openblas": {
    "_comment": "Doesn't work with windows native compilers",
    "build_on": {
      "windows": false
    },
    "alpine_packages": [
      "gfortran"
    ],
    "brew_packages": [
      "coreutils",
      "gfortran"
    ],
    "debian_packages": [
      "libtinfo5",
      "gfortran"
    ],
    "msys_packages": [      
      "base-devel",
      "mingw-w64-ucrt-x86_64-toolchain",
      "mingw-w64-ucrt-x86_64-fc"
    ]
  },

Also preemptively added to msys_packages; is there a way to separately pass in packages for clang64? Also is it OK if I add you as a co-author on this contribution?

@eli-schwartz
Copy link
Member

Updated the brew part, I thought that this would skip windows (the visualstudio ones):

Does it not work because the the source code can't support MSVC? What's the issue?

@HaoZeke
Copy link
Contributor Author

HaoZeke commented Oct 28, 2024

Updated the brew part, I thought that this would skip windows (the visualstudio ones):

Does it not work because the the source code can't support MSVC? What's the issue?

We've only tested the meson.build on UCRT64 so far https://github.com/HaoZeke/OpenBLAS/blob/bad74d752bad0f05cca6717e3670d75f101fb57e/.github/workflows/meson.yml#L78-L94

Upstream does also build with clang https://github.com/HaoZeke/OpenBLAS/blob/bad74d752bad0f05cca6717e3670d75f101fb57e/.github/workflows/dynamic_arch.yml#L154-L212 and also cl on Azure https://github.com/HaoZeke/OpenBLAS/blob/bad74d752bad0f05cca6717e3670d75f101fb57e/azure-pipelines.yml#L61-L139 ..

Eventually the meson.build will also work for the same set, but as of this submission it is basically just Linux / Darwin / UCRT64 windows.

@eli-schwartz
Copy link
Member

Ok so in theory it could work for cl but just isn't wired up.

IMO it's trivial to just not care that wrapdb CI will fail on cl / Visual Studio for now. The "build_on" settings are intended more for cases such as Linux "liburing" which binds to a kernel API and can't logically run on Windows.

A failed CI job won't have any effect on future wraps -- if we know exactly why the CI fails for the openblas + VS combination then we can simply decide the current state of the PR is "an improvement on what we had before" and merge it.

@HaoZeke
Copy link
Contributor Author

HaoZeke commented Nov 2, 2024

@eli-schwartz and @jpakkane given the clarifications, would it make sense to merge this as is for now and work on the microsoft builds later?

@jpakkane jpakkane merged commit bdb5dbe into mesonbuild:master Nov 3, 2024
4 of 9 checks passed
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.

3 participants