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 support for MKL pardiso in meson build. #84

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Apr 29, 2024

No description provided.

@@ -89,11 +58,13 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ ubuntu-latest, macos-latest, windows-latest ]
os: [ ubuntu-latest, macos-13, windows-latest ]
Copy link
Owner

Choose a reason for hiding this comment

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

I presume this is 13 here rather than latest because 14+ is only on M1? If so it would be good to add a comment here explaining that.

@bodono
Copy link
Owner

bodono commented May 2, 2024

Thanks for looking into this @isuruf !

This LGTM though I don't know much about meson. Pinging @enzbus who knows a lot more to see if he has any feedback.

@enzbus
Copy link
Contributor

enzbus commented May 2, 2024

This looks great on the surface, but @bodono you are too kind I really just helped putting together the installation script and am far from an expert in this. I guess if it passes CI tests it's a go for me? Your call...

@enzbus
Copy link
Contributor

enzbus commented May 2, 2024

@isuruf I see the removed anaconda hack in the meson.build and I approve, it was very tricky to get that to work, though

@bodono
Copy link
Owner

bodono commented May 2, 2024

Ok LGTM, I think @isuruf you can merge yourself now that I have approved? If not let me know and I can merge

@isuruf
Copy link
Contributor Author

isuruf commented May 2, 2024

Thanks for the review. I don't have permissions to merge even if you have approved. You'll have to do it. Thanks.

@bodono bodono merged commit ed94b23 into bodono:master May 2, 2024
55 checks passed
@enzbus
Copy link
Contributor

enzbus commented May 2, 2024

@bodono I have a hunch this patch broke windows wheels, I'm inspecting the test run logs. the "hack" was needed after all. I suggest you revert the merge

@enzbus
Copy link
Contributor

enzbus commented May 2, 2024

(blas/lapack is not being linked)

@isuruf
Copy link
Contributor Author

isuruf commented May 2, 2024

Hmm, the run at https://github.com/bodono/scs-python/actions/runs/8926143160/job/24516515638 shows that it is being linked to openblas.

The Meson build system
Version: 1.4.0
Source dir: C:\Users\runneradmin\AppData\Local\Temp\build-via-sdist-cale5a5x\scs-3.2.4
Build dir: C:\Users\runneradmin\AppData\Local\Temp\build-via-sdist-cale5a5x\scs-3.2.4.mesonpy-j5y40ebb
Build type: native build
Project name: scs
Project version: undefined
C compiler for the host machine: gcc (gcc 12.2.0 "gcc (x86_64-posix-seh-rev2, Built by MinGW-W64 project) 12.2.0")
C linker for the host machine: gcc ld.bfd 2.39
Host machine cpu family: x86_64
Host machine cpu: x86_64
Program python found: YES (C:\Users\runneradmin\AppData\Local\Temp\build-env-gjpcqlz_\Scripts\python.exe)
Library openblas found: NO
Found pkg-config: YES (C:\Miniconda3\envs\test\Library\bin\pkg-config.EXE) 0.29.2
Run-time dependency openblas found: YES 0.3.27
Compiler for C supports arguments -Wno-unused-result: YES
Run-time dependency python found: YES 3.12
Build targets in project: 2

@enzbus
Copy link
Contributor

enzbus commented May 2, 2024

Sorry @isuruf , I may be wrong. I'm re-reading the GH yaml file and it should work, there's the explicit removal of conda openblas to make sure everything is linked statically, and there is a SDP problem in the testfiles. I'm concerned about this line (build 3.12, windows-latest, false)

 Created wheel for scs: filename=scs-3.2.4-cp312-cp312-win_amd64.whl size=139833  ...

it should be around 10MB, but maybe it's before the delvewheel calll; GH removed logs from earlier runs so I can't compare...

@enzbus
Copy link
Contributor

enzbus commented May 2, 2024

Ok, now I get it; you are installing pkg-config from conda and meson then is able to see openblas, makes sense. Thanks, @bodono I revert my reservations.

@bodono
Copy link
Owner

bodono commented May 3, 2024

Thanks for looking into it @enzbus , I will leave as is then.

@enzbus
Copy link
Contributor

enzbus commented May 4, 2024

Yes, it's a good MR @bodono , @isuruf improved the build a lot especially on Windows. I came across this project recently https://cibuildwheel.pypa.io , you probably already know it, I guess it would be a nice addition (not sure how difficult it is to use, we still need to customize the Windows build, install conda, ...)

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