-
Notifications
You must be signed in to change notification settings - Fork 54
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
[BLOG] Added blog post for enhancing developer experience at SciPy - Intel oneAPI, MSVC and spin support #893
base: main
Are you sure you want to change the base?
Conversation
…Intel oneAPI, MSVC and spin support
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I ran the website locally with this PR. The changes looked good. I think Please let me know if changes are in needed in the commit message, contents of the blog post or anything else. |
The blog preview is https://labs-ksnzv1tzf-quansight.vercel.app/blog/intel-oneapi-msvc-spin-scipy. All the content is as it is ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I have used SciPy logo as featured and hero images. I hope that should work because its 100% SciPy work.
I have shortened the links. However at some places (very few) I had to use PR titles so I put the title inside "`" and linked the issue [[pr_number]](pr_link). That way the link is short. Please let me know if any other changes are needed. |
I checked the preview - https://labs-pu7mgao24-quansight.vercel.app - everything loads correctly ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @czgdp1807. A set of inline comments. Can you please line-break the content at 80 chars or so? That's way easier to review line by line.
|
||
<script src="https://asciinema.org/a/aZstdaf6B6nlkW9JFccT7z7Dr.js" id="asciicast-aZstdaf6B6nlkW9JFccT7z7Dr" async="true"></script> | ||
|
||
The above video showcases how `spin` works fine with Homebrew installed Python 3.10. The build completes. However, `dev.py` fails because it is unable to resolve paths correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add acknowledgements - this was done with NASA funding. Can you please copy the phrasing at the end of Albert's recent blog post for this?
|
||
#### Intel oneAPI Support in SciPy | ||
|
||
Intel offers oneAPI - a complete toolkit for HPC use cases. It has ICX (for C), ICPX (for C++), IFX (for Fortran) and MKL (an OpenBLAS alternative). We targeted two operating systems, Windows and Linux. `CI: adding a Windows CI job with MSVC + MKL + Intel Fortran (ifx)` [[gh-20878](https://github.com/scipy/scipy/issues/20878)] was the starting point. Solving this issue meant, avoiding regressions with MSVC and Intel oneAPI in SciPy. This issue also links [gh-20728](https://github.com/scipy/scipy/issues/20728). The author of this issue is building SciPy with `icx`, `icpx`, `ifx` and using MKL for BLAS and LAPACK. So a complete usage of Intel oneAPI. In addition, failure to build scipy with msvc [[gh-20860]](https://github.com/scipy/scipy/issues/20860) is related to build failure of SciPy with MSVC. We started off by fixing these two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "It has ICX ..." is repeated from a few paragraphs up. I actually suggest moving that content here.
1. Intel oneAPI and MSVC Support in SciPy | ||
2. Moving away from `dev.py` and migrating to `spin`. | ||
|
||
First, let me share the motivation behind these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better I think to share the general motivation in fewer words. In-depth paragraphs about both topics before repeating part of the in-depth content in the separate sections on OneAPI and spin
feels a little off.
Sorry, only half a review, hit submit too early - more comments coming. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be useful to spell check the content - there are a few too many mistakes.
The asciinema videos are still not rendering, I don't understand what is happening in the previews. The videos also need alt text (see checklist in PR description).
|
||
First, let me share the motivation behind these. | ||
|
||
Intel offers a complete toolkit for high performance computing (HPC), numerical computing use cases. It goes by the name of [oneAPI](https://www.intel.com/content/www/us/en/developer/tools/oneapi/overview.html). It provides, ICX (for C), ICPX (for C++), IFX (for Fortran) and Math Kernal Library (MKL for optimized math routines like BLAS, LAPACK, fast FFT, etc.) [[1]](https://en.wikipedia.org/wiki/Math_Kernel_Library). Since, SciPy caters to HPC and numerical computing use cases, so supporting building SciPy with Intel oneAPI is a natural requirement. In fact, users reported segmentation fault in `arpack` with `ifx` [[gh-20728]](https://github.com/scipy/scipy/issues/20728) and `scipy.linalg.solve` crash when used with Intel oneAPI on Windows 11 [[gh-21997]](https://github.com/scipy/scipy/issues/21997) are two such examples. Therefore we decided to work on this to enhance the experience of SciPy users who use Intel oneAPI as their toolkit. Regarding MSVC, since it is the de-facto compiler for C/C++ on Windows. So adding support for it is again a natural requirement and also a must. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'd use "MKL (Math Kernel Library, ...)" rather than the other way around, since you're also using the abbreviations for ICX & co
- MSVC: we didn't actually add support for it though, so the rationale here is a little off. Try to rephrase to what we actually aimed to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think MSVC wasn't tested on CI before our work, right? On Windows we used clang for CI testing. So may be a better line can be, "Since MSVC is the de-facto compiler for C/C++ on Windows, we added CI jobs for testing SciPy compilation with MSVC"
|
||
Intel offers a complete toolkit for high performance computing (HPC), numerical computing use cases. It goes by the name of [oneAPI](https://www.intel.com/content/www/us/en/developer/tools/oneapi/overview.html). It provides, ICX (for C), ICPX (for C++), IFX (for Fortran) and Math Kernal Library (MKL for optimized math routines like BLAS, LAPACK, fast FFT, etc.) [[1]](https://en.wikipedia.org/wiki/Math_Kernel_Library). Since, SciPy caters to HPC and numerical computing use cases, so supporting building SciPy with Intel oneAPI is a natural requirement. In fact, users reported segmentation fault in `arpack` with `ifx` [[gh-20728]](https://github.com/scipy/scipy/issues/20728) and `scipy.linalg.solve` crash when used with Intel oneAPI on Windows 11 [[gh-21997]](https://github.com/scipy/scipy/issues/21997) are two such examples. Therefore we decided to work on this to enhance the experience of SciPy users who use Intel oneAPI as their toolkit. Regarding MSVC, since it is the de-facto compiler for C/C++ on Windows. So adding support for it is again a natural requirement and also a must. | ||
|
||
Now let's see why we decided to spin from `dev.py` to `spin` (pun inteded). `dev.py` (or `do.py` [[2]](https://labs.quansight.org/blog/the-evolution-of-the-scipy-developer-cli)) was added to improve the experience for SciPy developers. Basically, it offers better documentation for the various commands needed (like, `build`, `test`, `shell`) while working on SciPy codebase. It has better looking CLI outputs. However, with time several issues with `dev.py` were reported. For example, in some cases, doit based dev interface interferes with pdb command history [[gh-16452]](https://github.com/scipy/scipy/issues/16452), `dev.py` giving problems in a Windows CI environment because of emojis usage in dev.py [[gh-18046]](https://github.com/scipy/scipy/issues/18046), and dev.py unable to resolve homebrew installed Python correctly [[gh-18998]](https://github.com/scipy/scipy/issues/18998). `spin` easily fixes this issues. [[3]](https://github.com/scipy/scipy/pull/21674), especially those issues arising due to `doit`. `dev.py` depends on `doit` but `spin` doesn't. Also, since `spin` is used by many other projects, so all the improvements in `spin` due to the issues reported by their maintainers will directly benefit SciPy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say the long-term maintenance load sharing and the familiarity for contributors coming from numpy
or other project are the main motivations. The paper cuts & bugs are very minor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it'd be good to say somehting like "dev.py
was a major improvement in development workflow, and came before spin
. spin
took its inspiration in large part from dev.py
, and generalized its interface to work with more projects without customization as heavy as dev.py
has for SciPy".
|
||
One major issue with MSVC is that its support for handling arrays on stack (with runtime sizes) is limited. Expressions like, `std::complex<double> cwrk[n];` fail to compile with MSVC unless `n` is a constant. So we did runtime allocation on heap (using `new`) and performed `delete` at the end to free memory. This is a manual approach of handling memory, however this is the only solution which works with MSVC. It was the only option that we went ahead with. On a side note, Clang due to LLVM backend doesn't have this limitation. After fixing this issue we added a CI job with MSVC + `ifx` + OpenBLAS combination. This helped in avoiding future regression with MSVC build of SciPy. It was also a first step towards supporting Intel oneAPI toolkit entirely. | ||
|
||
Now coming on to the `arpack` issue. Fixing this issue required supporting building SciPy with Intel oneAPI. On Linux the `arpack` issue was already resolved. However, there were some other issues with `ifx`. For example, `test_equal_bounds` test in `scipy/optimize/tests/test_optimize.py` failed due to floating point issues with `ifx`. Also tolerances for other tests had to be increased (i.e., slight lesser accuracy/precision in order to make things with `ifx`). All of this was done in `BUG/CI: Compile and run tests with `ifx`+`MKL` on Linux` [[gh-21173]](https://github.com/scipy/scipy/pull/21173). I also added a CI job in this PR with `gcc` + `g++` +`ifx` + `MKL` combination. A second steps towards supporting Intel oneAPI with SciPy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to link to https://github.com/opencollab/arpack-ng, most readers won't know what ARPACK is.
|
||
Now coming on to the `arpack` issue. Fixing this issue required supporting building SciPy with Intel oneAPI. On Linux the `arpack` issue was already resolved. However, there were some other issues with `ifx`. For example, `test_equal_bounds` test in `scipy/optimize/tests/test_optimize.py` failed due to floating point issues with `ifx`. Also tolerances for other tests had to be increased (i.e., slight lesser accuracy/precision in order to make things with `ifx`). All of this was done in `BUG/CI: Compile and run tests with `ifx`+`MKL` on Linux` [[gh-21173]](https://github.com/scipy/scipy/pull/21173). I also added a CI job in this PR with `gcc` + `g++` +`ifx` + `MKL` combination. A second steps towards supporting Intel oneAPI with SciPy. | ||
|
||
The final checkpoint for Intel oneAPI was `CI: Test icx + icpx + ifx + MKL build of SciPy` [[gh-21254]](https://github.com/scipy/scipy/pull/21254). Here we replaced `gcc` with `icx` and `g++` with `icpx` and TADA, it worked on Linux. Regarding Windows, we are still at `MSVC` + `ifx` + `OpenBLAS` combination because of the `arpack` import error. We tried several ways to fix it but all resulted in a dead end. We tried to build NumPy with `icx`, `icpx` but `icx` is unable to compile NumPy code as is. Some workarounds had to be made. However, still it didn't work for us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last two sentences are not a very good ending. Try to say why we stopped (e.g., "the effort wasn't justified by ..."), otherwise it leaves the reader wondering.
|
||
Now it would be easy to understand our work below. | ||
|
||
We decided to shift to `spin` because of the several issues reported in `dev.py`. For example, [doit interfering with command history of pdb] [[gh-16452]](https://github.com/scipy/scipy/issues/16452), usage of emojis giving issues in a Windows CI environment [[gh-18046]](https://github.com/scipy/scipy/issues/18046), and dev.py unable to resolve homebrew installed Python [[gh-18998]](https://github.com/scipy/scipy/issues/18998). A bunch of those are due to `doit` being a dependency of `dev.py`. One nice feature of `spin` is it has lesser dependencies. This means, lesser bugs because more dependencies means more points of failure. The tradeoff here is to compromise looks and feel of the output a little bit to avoid problems while developing SciPy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeating grammar issue, so commenting: it's "fewer bugs" rather than "lesser bugs" (check for "lesser" in the whole post please)
@rgommers Taking into account your reviews, I have done the following,
Please go through it now and let me know your experience. Drop in any reviews where you think changes are needed. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czgdp1807 Thanks for authoring this post, and for your patience with the review. It's a good read, and I learnt some new things! I've shared a few narrative suggestions.
The videos not rendering in the preview is a blocker. I'm not familiar with asciinema, and need to look into it.
Once we addressed this issue, we added a CI job to test SciPy's compilation with MSVC + `ifx` + OpenBLAS, which helped prevent future regressions in the MSVC build. | ||
This also marked an early step toward supporting Intel oneAPI within SciPy. | ||
|
||
The next major hurdle involved the [`arpack`](https://github.com/opencollab/arpack-ng) issue, which required full Intel oneAPI support to resolve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand what the "arpack issue" is? Could we link to any issue on the scipy tracker that has more context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am referring to scipy/scipy#20728. You are right, just linking it here as well should make it clear. I have referred to this issue in lines 42-44.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Let me know if it looks good to you.
We’re waiting for the PR [scientific-python/spin#257](https://github.com/scientific-python/spin/pull/257) to be merged, after which we’ll be ready to fully integrate `spin` into SciPy. | ||
Our work has been fully tested in the SciPy CI system, ensuring its stability. | ||
|
||
Here’s a quick comparison of the SciPy build experience with `dev.py` versus `spin`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The videos don't seem to rendering in the preview: https://labs-git-fork-czgdp1807-feature-intel-oneapi-spin-quansight.vercel.app/blog/intel-oneapi-msvc-spin-scipy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I can try https://docs.asciinema.org/manual/server/embedding/#preview-image-link for sharing the videos. asciinema
will provide an image. If someone clicks it then the videos plays on asciinema website. That should solve the problems showing up due to embedding the player using script
tag. Let me know if it would work.
@@ -0,0 +1,174 @@ | |||
--- | |||
title: 'Enhancing Developer Experience at SciPy - Intel oneAPI/MSVC Support and Migrating to spin' | |||
published: February 21, 2025 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note to ensure the date is updated before merging this PR.
Co-authored-by: Pavithra Eswaramoorthy <[email protected]>
Text styling
Non-text contents