-
-
Notifications
You must be signed in to change notification settings - Fork 785
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
SciPy v1.13.0 #4719
base: main
Are you sure you want to change the base?
SciPy v1.13.0 #4719
Conversation
I see that SciPy is being reverted from v1.13.0 to v1.12.0 in #4712 – I'm not sure what that means, so I'll be happy to wait until further progress or discussion on that PR (my understanding is that whichever packages still pass the tests with newer versions can stay and will not be reverted?) |
We do one big PR for all the easy packages and then have to handle the ones that cause trouble one at a time as we have time. Please go ahead with working on this PR. If there is a conflict with #4712 for some reason I can help resolve it. |
@agriyakhetarpal just curious, what is the status of this? Since you mentioned "as as I look to fix some of the older patches locally" I am guessing that you are still working on updating the patches? For some weird reasons, it looks like the full CI was not run (random guess maybe because this is a Draft PR, although seems unlikely), can you try pushing an empty commit to trigger the CI again? I was confused and thought this PR was green untill I unfolded the "All checks" and found out that there are only readthedocs and pre-commit ... |
Hi, @lesteve, thank you for the ping. I have just returned to this PR late last week, and I was going to post an update today about how this is going, so, yes, doing this and updating the patches here is well on my radar for this week. I'll be sure to request help wherever I don't understand something fully!
Ah, I guess the full CI was not run because I added the |
Ah makes sense I missed the |
It's an unfortunate thing about Skip Ci. Maybe we should add a "ci ran" check. Of course we can merge changes that skip CI, but the Ci is flaky enough that we merge failing CI a lot. |
[skip ci]
Co-authored-by: Gyeongjae Choi <def6488@gmail.com>
The patch is updated to account for the change from wrap_dummy_g77_abi.f to wrap_dummy_g77_abi.c, in which case return values of methods are changed in order to return integer values rather than void based on Emscripten regulations. Changes to fblas_l1.pyf.src have been dropped, putting into consideration the fact that they have made it upstream to v1.13.0 already and are most likely fixed. Co-Authored-By: Joe Marshall <joe.marshall@nottingham.ac.uk>
eda368c
to
725cc1e
Compare
I rebased on top of the latest changes from the |
I assume that the failures under |
Oops, it looks like Edit: never mind, I saw that some new builds popped up on CircleCI. |
Note to self: most of the errors look to be coming from Fortran code because the f2c sub-command failed; I have to look at syntax errors in a few files, remove warnings about a few compiler flags (probably replace -Wno-maybe-unintialized with -Wno-unintialized and look at how Emscripten sets runtime search paths because -rpath was reported to be unknown, etc.), and correctly identify the file leading to the cause of the error (I am currently browsing through the logs on a mobile device and that does not help) – it's most likely inside FITPACK. |
Btw, @hoodmane, if you could elaborate on this slightly, I would be happy to add this "ci ran" check in some shape or form (if the change would not be non-trivial, that is, of course). |
The errors were in if(t(i).le.t(i-1))then; ier=30; go to 80; endif
...
if(x(1).lt.t(k1) .or. x(m).gt.t(nk2))then; ier=40; go to 80;
endif
...
if(i.ge.m)then; ier=50; go to 80; endif and so on. I checked the file versus SciPy v1.12 and scipy/scipy@ and now SciPy v1.13.0 builds successfully (🎉): workflow logs. I'll wait for the build to finish, following which I can add a CHANGELOG entry when this will be merged (please let me know if I should skip CI for that one) and complete the rest of the checklists. |
Never mind, there are 4 failures out of 5 tests on SciPy: Edit: and similarly for |
I don't entirely know how to debug this and I am afraid that my attempts to look further did not lead to much insight. As far as I understand, not being experienced with Fortran codes, there could be a missing Happy to receive suggestions on how I can debug this further and work towards a solution, @ryanking13 and @hoodmane – your comments will be highly appreciated! |
I'll take a look when I have a minute. At pycon right now. |
If it helps, scipy/scipy#20232 should be the relevant SciPy PR I believe. It replaced some of the problematic Fortran wrappers for ABI differences with C wrappers. |
Thanks for working on this! I will take a look after the PyCon (probably after May 23) |
@rgommers, I had a question that is extrinsic to these changes but probably still relevant – is there a policy on upstreaming particular non-intrusive patches to SciPy? I haven't followed the discussion in scipy/scipy#15290 till the end but based on a quick look, there is a history of some having been upstreamed – in this case, patch 9/9 is a very small one and I would like to propose fixing it upstream after this PR here is complete/merged; it would (should) not break any existing changes and the syntax is probably more compatible across these different variants for Fortran compilers. |
That all depends on whether the changes improve the SciPy code, or whether it's a workaround that's very specific to Pyodide. In this case, the original Fortran code seems correct and easier to read than the patched version - and this looks like an |
The problem seems to be that
which is frustratingly only the first of potentially many problems and also pretty darn vague. Checking the type of the symbol that it is getting (which presumably hasn't changed): pyodide._module.resolveGlobalSymbol("cbbcsd_").sym.type() we see that it takes 29 i32 params and returns one i32 result.
shows that
so the problem is the void return type. Of course. |
Looks like |
Okay progress, now problem is
Specifically: line = line.replace("ret = chla_transtype(", "call chla_transtype(ret, 1,") |
Where did all the build artifacts go? Meson seems to hide them better than the old build system did. |
I am not sure what happens under cross-compilation, but the build artifacts typically show up inside a |
Hmm don't see any |
By default they go into a temporary directory if you use a plain |
We should just add |
@hoodmane, where would you recommend I add this? I tried to follow along similar recipes but couldn't find any, and then I looked at how this is done for the Pyodide CLI in |
Add it to |
7ba7a6b
to
a9008b8
Compare
Okay, it failed to build SciPy but this is fixable, based on https://mesonbuild.com/meson-python/how-to-guides/config-settings.html#how-to-guides-config-settings. Let me push a change to fix this and analyse which other modules fail to import after the changes to the |
I think I have tried all combinations in
and none of them seems to work with |
The correct invocation should be: backend-flags: build-dir=build Searching other build recipes in this repo for examples is usually helpful. The numpy package has a usage of |
Ah, there we go, thanks, @rgommers! I did do a search for this throughout the recipes before, and the reason I did not find anything was because I searched for I feel this could be documented somewhere for use by others. There is a mention in the CHANGELOG, but it's not highlighted for package authors and maintainers on how to use |
1f528c6
to
5930926
Compare
Documentation PRs are always very welcome. Our docs are very incomplete, and it would be nice for them to be less incomplete. |
Description
This PR bumps the in-tree SciPy version in the Pyodide distribution to v1.13.0. This is an in-progress change, as I look to fix some of the older patches locally, and therefore this PR is being opened for purposes of visibility only at this time.
cc: @rgommers, and the package maintainers @lesteve, @steppi as listed in the recipe.
To be done
scipy/_build_utils/src/wrap_g77_abi.c
– changed fromscipy/_build_utils/src/wrap_g77_abi.f
in SciPy v1.12.0imports:
Checklists