Update version and switch to v1 recipe #4
Conversation
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
|
@SwayamInSync The current problem under Linux seems to be due to QBLAS not being present. I tried to disable it for now, but in meson.build it is always looked for the directory, which is however not present in the sdist from PyPI. I think the best solution would be to also publish QBLAS on conda-forge such that we can depend on it here. Otherwise we have to clone the QBLAS git repo during build time here. What would you suggest? |
|
@JaRoSchm thanks a lot for your effort and catching the issue. I'll also check and try to update the sdist over PyPI to contain the submodule |
This comment was marked as outdated.
This comment was marked as outdated.
|
Looks like the linux build works now! I noticed that the license file is missing in the sdist. This is probably important to fix. There are, however, many test failures right now. Can you spot what is going wrong there? |
|
I checked and the repo didn't include the One more thing I noticed that recipe is fetching sleef from conda-forge (3.9) we clone the 3.8 from source and I just checked that it is missing too in sdist I think this might be too much manual tweaking, I will try to update the sdist to contain everything and that might can ease the workflow here |
|
or we might try pulling the 3.8 sleef from conda-forge |
|
Yeah, I tried pinning sleef to 3.8 locally already, but this did not help (I pushed it a moment ago). I think in principle using sleef from conda-forge should be the preferred option here. And some tests are passing, so it does not seem to go completely wrong. |
|
Actually, it seems to be that |
|
Yes, that's why I thought maybe in 3.9 something is breaking there. Since on our CI building from 3.8 source works on all platforms. |
|
Another interesting observation is that the number of failed |
|
This is very weird, I am not exactly sure the root cause. other than LISCENCE and QBLAS, it seem to have the correct updated code files. I am not very well known about this but can you please take a look at the mentioned file and see if there is any issue that causing this conflict? |
|
I manually tried building sdist then installing and testing on top it. Everything seems to work As my wild guess, can we try building sleef from source just as a test? since logically those failed tests rely on Sleef APIs maybe this can give us better insight. I'm so sorry for this hassle. |
|
I compared the files from the Concerning building sleef from source here: This sounds like a good attempt but I didn't have enough time to get this working locally. Here is what is used by conda-forge to build sleef (see https://github.com/conda-forge/sleef-feedstock/blob/main/recipe/build.sh): mkdir build
cd build
if [[ "$target_platform" == linux-* ]]; then
LDFLAGS="-lrt ${LDFLAGS}"
fi
cmake ${CMAKE_ARGS} \
-GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DSLEEF_BUILD_TESTS=OFF \
-DSLEEF_BUILD_SHARED_LIBS=ON \
-DSLEEF_BUILD_SCALAR_LIB=ON \
-DSLEEF_BUILD_QUAD=ON \
-DSLEEF_BUILD_GNUABI_LIBS=ON \
-DSLEEF_ENABLE_TLFLOAT=OFF \
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DCMAKE_PREFIX_PATH=$PREFIX \
-DCMAKE_INSTALL_LIBDIR=lib \
..
ninja -j${CPU_COUNT} installDoes something look to you like it could cause problems? |
|
And please don't worry, I am really thankful for your work on quaddtype and look forward to start using it! |
|
Thanks @JaRoSchm I'm right putting everything to sdist (including sleef) testing it over our repo and as all goes well. |
|
Hey @JaRoSchm we are ready to do another release, and I was just thinking that will it be possible for you to test on sdist from our repo to confirm things look good (lisence, readme, sleef and other requirements) and once confirm we can push the release to PyPI and we can synchronize it here? |
|
I will have a look. |
|
@JaRoSchm I think we can unpin sleef from conda-forge and maybe we can also skip the meson build command, it should be as simple as just doing |
|
rattler-build is not able to pull the sdist directly from the CI of numpy-user-dtypes directly, therefore I temporarily added the content of the latest sdist from the main branch here for testing. Locally, this builds fine (with tests still failing). However, here in the conda-forge CI already the build is failing. Maybe the conda-forge environment flags like CMAKE_ARGS aren't correctly propagated through meson? |
Are these the same tests? |
|
Yes and additionally |
I see so ninja and cmake is found but our meson is configured for "unix make" which is not available on conda-forge. I can change the script to use ninja if detected |
You sure, this is not getting linked to some locally installed sleef in your system? Okay I am updating the meson and till then see if you can just clone our repo and do the following in a completely new temporary environment Test failing does not make sense as on CI same tests are passing for 3 different OS, 7 different python versions incl. free-threading |
|
After adding |
|
Sure, first you need to have pixi installed: https://pixi.sh/latest/installation/ I the build fails, you will get some output on the console which allows you to go to the work directory and run the build step-by-step manually for debugging. |
|
Oh man, found the found the culprit, there is a data corruption happening which somehow didn't got triggered in github CI. |
|
That's great!
Honestly, I have no idea. I will try to think about it more after seeing the actual fix. So in principle this means, that be could use the sleef already contained in conda-forge like in the original proposal, right? This way sleef wouldn't have to be built multiple times in conda-forge CI. What would be preferable from your point of view? I think the approach we are currently using here is similar to what scipy does. So both approaches are fine with me. |
|
so I was mistakenly casting the byte memory (char *) to entire union, expecting it'll be filling the right candidate of that union's elements violating the strict aliasing rule here is the PR numpy/numpy-user-dtypes#148 |
Yes, it should now also worked with conda-packaged sleef. For users installing from soruce code, SLEEF is a big bottleneck to handle its installation and then managing to provide correct shared library path. build:
number: 0
script:
- ${{ PYTHON }} -m pip install . -v
skip:
- win
- osxI will be checking later to utilize the ccache for improving build times |
|
@JaRoSchm the SDIST got generated on that PR, can you fetch that and test it one more time here |
|
I was getting the same error when building locally inside the docker. Does it require anything from package side? |
|
The problem is that I used numpy-quaddtype while you used numpy_quaddtype as a package name before. We have two options here:
Anyways, I think you can move forward with the release, the build looks fine! |
|
I agree, let's keep the name with sync with PyPI. |
|
After reading https://github.com/conda-forge/admin-requests?tab=readme-ov-file#add-a-package-output-to-a-feedstock more carefully, I think renaming isn‘t an option, just adding an output. So I think we will have to stay with what we have. |
I see, and is it possible to completely upload the new and future sdist under the |
recipe/recipe.yaml
Outdated
| # - ${{ PYTHON }} -m pip install --find-links dist numpy-quaddtype | ||
| - ${{ PYTHON }} -m pip install . -v | ||
| skip: | ||
| - win |
There was a problem hiding this comment.
And can we also test on these?
There was a problem hiding this comment.
I will just try to add it. If it doesn’t work, I will try to fix it tomorrow (it’s getting late here). ;)
Everything seems to be working now. I think it's ok for the package on pypi to use an underscore. Let's not change it. I'm also not sure if pypi's package name similarity detection would trigger and prevent an upload with the new name. I also personally don't like it when packages have dashes in the name. It means the name of the package doesn't match the name of the "main" module exported by the package, which is a bad user experience IMO. |
Yes, this shouldn’t be a problem, we just can’t use the name variable in the pypi url in recipe.yaml. I looked through the pull requests in https://github.com/conda-forge/admin-requests and the only option for renaming seems to be to archive this feedstock and create a new one. In my opinion this doesn’t sound like it’s worth it. |
|
Sure, so Anyways within the python interface it'll be use like @ngoldbaum pypi has the dash basically 🥲 |
|
Sure, @JaRoSchm let me know when we can move to the release after testing windows and mac, you can plug the path back to PyPI source to fetch sdist from. |
Ah, well, too bad. We're not the only ones. Looking at you scikit-learn... |
|
@JaRoSchm from the errors it seems windows is compilng with QBLAS (which we keep disable because of incompatibility with MSVC, I can do it in the meson but for now this has to be explictly passed as flag) as follows (ONLY for Windows build): export CFLAGS="/DDISABLE_QUADBLAS $CFLAGS"
export CXXFLAGS="/DDISABLE_QUADBLAS $CXXFLAGS" |
|
Cool, everything seems to work. Is it ready to merge @JaRoSchm ? |
|
No, this still contains the source code. As 0.1.0 was not able to be built on conda-forge, I will update this PR to 0.2.0 after you made the release and then you can go ahead and merge. After that I will restart the migration in #3. |
JaRoSchm
force-pushed
the
update_version
branch
from
996f241 to
0035e3b
Compare
|
@conda-forge-admin, please rerender |
|
@SwayamInSync If CI runs through, this is finally ready to be merged! |
|
@JaRoSchm thanks for all your help and advice over here! |
|
Awesome, merging it Thanks a lot @JaRoSchm for helping us out |
|
It was a pleasure! |
4 participants
closes #2
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)Hi @SwayamInSync, sadly I don't have enough time today and probably this week to try to finish this. This is the best I could come up with right now. Locally, this was only tested on Linux (even there it doesn't work yet), but will definitely not work on other platforms. I am more than happy if someone else is able to help here as I have no experience with meson.
Additionally to the updated version, I took the freedom to bump this to a v1 recipe using
rattler-buildandpixias build tools. For working locally on this, the commandspixi run lint,pixi run rerenderandpixi run buildare very helpful.