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

Update9.1.0 #128

Merged
merged 46 commits into from
Oct 14, 2024
Merged

Update9.1.0 #128

merged 46 commits into from
Oct 14, 2024

Conversation

alugowski
Copy link
Collaborator

Update to #127

@alugowski
Copy link
Collaborator Author

@eriknw Here's the options for the JIT that I can think of.

The JIT needs to be configured with a compiler, which is done at GraphBLAS build time by GraphBLAS_JIT_configure.cmake. Of course that's a non-starter for Python because the user's machine is different than the build machine, but AFAIK it's possible to set a new config at runtime.

One option is to pip install cmake and run that configure script at import time (or an explicit enable_jit()), then set that config. As long as there's safe a fallback, explicit documentation, and a guided experience through possible failures then this can work. Shoaib Kamil's SEJITS did this a decade ago and we used it to compile user-defined semirings.

Another is to tie in to the Python compiler packages like numba (ships LLVM and tackles the versioning and compatibility nighmare), or pythran or even Cython. This would be a nicer user experience, but I haven't dug too much into how much of GraphBLAS the JIT compiles. This may work if it's a user callback or even a kernel, but likely not if it's a large chunk.

Another is to only support the JIT on conda. I believe they have some sort of compiler guarantees in their environments, so maybe then you can assume the build-time compiler is available at runtime. May require some magic to handle different absolute paths to conda environments, but I'm sure they have some best practices for this.

Apart from perhaps conda, JIT support isn't going to be a simple flip-a-flag affair. Either way, I recommend handling it separately, I opened #129 to track it.

I'll let you decide if what we have here is enough or if more work needs to be done.

@DrTimothyAldenDavis
Copy link
Member

Yes, there is a way to set the compiler used in the JIT at run time, with GrB_set. I have many options for controlling the JIT with GrB_set / GrB_get:

    GxB_JIT_C_COMPILER_NAME = 7024,  // CPU JIT C compiler name
    GxB_JIT_C_COMPILER_FLAGS = 7025, // CPU JIT C compiler flags
    GxB_JIT_C_LINKER_FLAGS = 7026,   // CPU JIT C linker flags
    GxB_JIT_C_LIBRARIES = 7027,      // CPU JIT C libraries
    GxB_JIT_C_PREFACE = 7028,        // CPU JIT C preface
    GxB_JIT_C_CONTROL = 7029,        // CPU JIT C control
    GxB_JIT_CACHE_PATH = 7030,       // CPU/CUDA JIT path for compiled kernels
    GxB_JIT_C_CMAKE_LIBS = 7031,     // CPU JIT C libraries when using cmake
    GxB_JIT_USE_CMAKE = 7032,        // CPU JIT: use cmake or direct compile
    GxB_JIT_ERROR_LOG = 7033,        // CPU JIT: error log file

The defaults for all of these are found by cmake when GraphBLAS itself is compiled and so by default I use the compiler, flags, and so on, that were used to build GraphBLAS (seems like a reasonable default, but of course it is different for the python-graphblas wrapper). I configure the GraphBLAS/Config/GB_config.h.in file to create GraphBLAS/Config/GB_config.h. When GrB_init starts, it sets all those strings to the defaults in GB_config.h, and if desired they can be changed later at any time, in the end user application. I only use the definitions in GB_config.h at GrB_init time. So none of those strings in GB_config.h are baked in stone; all of them can be changed later at run time.

So that means at runtime, the python wrapper can make any decisions it likes. It can try to set up its own defaults when python starts. It could also accept some option from the python user, like gb.compiler('/usr/bin/mynewcompiler') or something, as you like.

@DrTimothyAldenDavis
Copy link
Member

It's not a Python error. It's a SIGABORT on a call into GraphBLAS:

python-suitesparse-graphblas/suitesparse_graphblas/tests/test_io.py

Line 104 in 045d489

lib.GrB_Matrix_eWiseAdd_BinaryOp(C[0], NULL, NULL, _eq_ops[T], A[0], B[0], NULL),
Somebody will have to turn on logging, or replicate it locally.

The burble can help. I could also try to replicate it on my system, if I had some help with getting things installed and compiled (I rarely use python so I'd need some help).

@DrTimothyAldenDavis
Copy link
Member

I guess my last two comments should be added to #129. I'll copy them there.

@DrTimothyAldenDavis
Copy link
Member

Regarding the complex type in GraphBLAS.h:

Ah yes, I remember writing the hacks to make MSVC complex types work for the Windows wheels :) I believe there are some deeper guarantees between compatibility of complex types cross-language and likely cross-compiler (like it's guaranteed safe to typecast C and C++ complex types because they're explicitly speced to have the same memory layout), but I suppose the fragility will stay. I appreciate the cmake approach, but beware that it appears to break sometimes.

I can add a flag that allows the end user application (in this case the python interface to GraphBLAS) to take control from cmake and bypass the #cmakedefine's in GraphBLAS/Config/GraphBLAS.h.in. Something like this:

DrTimothyAldenDavis/SuiteSparse@e81e6c1

If that works (I'm still testing it), I can add it to GraphBLAS 9.3.0.beta2. Would that work for python-graphblas?

For the JIT, you'd need to add something after GrB_init, which would use GrB_get to get the current C flags, then GrB_set to append -DGXB_CMAKE_BYPASS -DGXB_HAVE_COMPLEX_C99 (for example). A bit tedious of course but the end python user wouldn't see it.

@alugowski
Copy link
Collaborator Author

If that works (I'm still testing it), I can add it to GraphBLAS 9.3.0.beta2. Would that work for python-graphblas?

My comment about complex values is for other GraphBLAS users, not us. We have the luxury of known build environments and already have a place where I have to do a bunch of patching for MSVC complex values. The workaround for us is already in this PR and works on 9.1:

extra_compile_args = list(extra_compile_args) + ["-DGxB_HAVE_COMPLEX_MSVC"]

I pointed it out because GraphBLAS is already a high-friction library and that small problem is just another thing a new user has to solve. It seems like something that can be solved automatically with no intervention needed at all. Sounds like I may have missed some interaction between GNU and MS tools that you found, and in that case I defer to what you found to be necessary. However as you can see the upstream method didn't work for us and it required manual intervention.

@DrTimothyAldenDavis
Copy link
Member

I pointed it out because GraphBLAS is already a high-friction library and that small problem is just another thing a new user has to solve. It seems like something that can be solved automatically with no intervention needed at all. Sounds like I may have missed some interaction between GNU and MS tools that you found, and in that case I defer to what you found to be necessary. However as you can see the upstream method didn't work for us and it required manual intervention.

It can get complicated, with all the system variations we see in SuiteSparse; the github CI there tries to account for them and test them. I realize that the complex variable type thing can be a painful friction.

What I have done so far is to make sure your workaround will always work. If an end user package is compiled with GraphBLAS, and they want to insist to see a particular complex type on their side, they can do either

#define GxB_HAVE_COMPLEX_C99
#include "GraphBLAS.h"

or

#define GxB_HAVE_COMPLEX_MSVC
#include "GraphBLAS.h"

or, to add the corresponding -D(whatever) to the compile command. This is only needed if the configuration of GraphBLAS by cmake differs from what the user wants in their application.

So I've added this to GraphBLAS.h:

https://github.com/DrTimothyAldenDavis/GraphBLAS/blob/c634199269d46bd63dba9583b5692850e77c5170/Config/GraphBLAS.h.in#L166-L191

Copy link
Member

@eriknw eriknw left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much @alugowski! I made a few tweaks:

  • Build with numpy 2
  • Add Python 3.13
  • Drop Python 3.8
  • Update cibuildwheel b/c yum install stopped working due to centos mirrors being EOLed

Care to take a quick look before we merge? Also, should we jump straight to SuiteSparse:GraphBLAS 9.3.1 (the latest)?

@DrTimothyAldenDavis
Copy link
Member

GraphBLAS 9.3.1 has 3 bug fixes for bugs in 9.1.0. So going to 9.3.1 would be a good idea.

@DrTimothyAldenDavis
Copy link
Member

See these in the GraphBLAS/Doc/ChangeLog:

9.3.1:

  • (56) bug fix: wrong type for fgetc return value in JITpackage; leads to infinite loop on some systems when building GraphBLAS.

Aug 2, 2024: version 9.3.0

* (55) bug fix: GrB_apply with user-defined index-unary op would fail if A was iso and the JIT was disabled (failure in generic case). Caught by Christina Koutsou.
* (54) bug fix: reducing a huge iso full matrix to a scalar resulted in integer overflow if nrows*ncols was larger than about 2^60.

@alugowski
Copy link
Collaborator Author

LGTM, thanks so much @alugowski! I made a few tweaks:

* Build with numpy 2

* Add Python 3.13

* Drop Python 3.8

* Update `cibuildwheel` b/c `yum install` stopped working due to centos mirrors being EOLed

Care to take a quick look before we merge? Also, should we jump straight to SuiteSparse:GraphBLAS 9.3.1 (the latest)?

Looks great!

We should jump to latest before releasing, but in a separate PR.

@eriknw
Copy link
Member

eriknw commented Oct 13, 2024

Thanks for the quick responses! The requested updates have been made. Let's merge after CI passes, then update to latest SuiteSparse:GraphBLAS in a new PR.

@eriknw eriknw merged commit 4bf5c72 into main Oct 14, 2024
25 checks passed
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