Bundle SLEEF and submodules using meson wrap [sdist compatible] #143
Conversation
Switching to wrap from submodule systems
|
My one wild guess is that sleef available on conds-forge is causing some issues with our tests so vendoring ours within sdist and building everything from there maybe solves them free |
|
Thank you for trying this! If someone else wants to have a look at the failure of one test using sleef from conda forge, please see conda-forge/numpy_quaddtype-feedstock#4 |
If conda-forge is fine with this and doesn't want to unbundle SLEEF then I'm happy with this too. It's annoying to have to download and build SLEEF separately. @SwayamInSync I'll try to review this soon. |
SwayamInSync
changed the title
|
Packaged the sleef as well, it simplified the user-side workflow by a lot, just build times got increased. We can use ccache for that but I think we are good with the current setup |
|
To include LISCENSE I just copied it to the quaddtype folder (I hope it's okay) I tried pointing to the outside's LISCENSE but that didn't work |
|
The README needs updates too, right? It would help me review if you update that too. Moving a copy of the license file into this directory is fine. |
Sure, doing it. |
quaddtype/sdist_test.sh
Outdated
|
|
||
| python -m pip uninstall -y numpy_quaddtype | ||
| python -m build --sdist --outdir dist/ | ||
| python -m pip install dist/numpy_quaddtype-0.1.0.tar.gz -v |
There was a problem hiding this comment.
Could we avoid hard coding the version into the path?
There was a problem hiding this comment.
Done (although this is just a utility, @ngoldbaum let me know if you want to keep it)
There was a problem hiding this comment.
Reason behind that, you can't test both local build and sdist together as for changes to be visible in sdist we need to commit those changes first only then they get reflected and ready to test inside sdist. So I thought to do it separately.
|
These new commits just add an additional testing on top of the built sdist, to install and run pytest |
|
@ngoldbaum can you take a look at this too please |
|
On it - just starting my day here! |
There was a problem hiding this comment.
Awesome! This is a huge usability improvement. I edited the PR description to include the string "closes #107" which will auto-close that issue when this PR is merged.
I have some suggestions for the readme, take 'em or leave 'em.
Co-authored-by: Nathan Goldbaum <[email protected]>
|
@ngoldbaum after test passes, you can merge this and we can do another release so that me and @JaRoSchm can use these upstream changes to test on conda-forge |
|
It looks like this makes CI a lot slower, probably because SLEEF and QBLAS are getting rebuilt repeatedly. There's probably a way to add caching to avoid that. |
Maybe just need to turn on ccache caching? https://github.com/hendrikmuhs/ccache-action |
I tried to use ccache here on linux SwayamInSync#11 didn't help much (or maybe I am doing it wrong) ~20 mins on linux (without ccache it was 16mins 😄 ) will surely look into it. |
|
I'm OK with temporarily making CI slow and merging this now but let's try to fix that before anyone else comes along and wants to experiment with a new dtype... |
Yup, will play with the action you mentioned, it seem simpler than what I was doing |
Another release sounds good to me, perhaps we can wait until #145 is merged? |
|
Yes @juntyr I am waiting for that to merge and only then make a release |
Closes #107
This PR bundles the submodule QBLAS and dependency SLEEF within the package using meson wrap
This also simplifies the GitHub workflows @ngoldbaum please take a look, if setup looks good I'll try to do same with sleef