- Notifications
You must be signed in to change notification settings - Fork15
Update to SuiteSparse:GraphBLAS 8.0.0#456
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
coveralls commentedMay 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
eriknw commentedMay 21, 2023
Alright, I think we should consider how we want contexts to behave and not rely on default behavior. For example, I think it would be nice to be able to layer contexts: withContext(nthreads=4):# nthreads is 4withContext(chunk=0.5):# nthreads is still 4 (new behavior!) ...# nthreads is still 4 (new behavior!) |
eriknw commentedMay 22, 2023
I updated contexts to chain and stack by default as illustrated in my previous comment. Descriptor-like context values (such as I/we still need to document and test descriptors, and work on the JIT. If anybody wants to continue to work or play with this PR, ping me on discord or slack. |
eriknw commentedMay 22, 2023
In response to this comment,DrTimothyAldenDavis/GraphBLAS#218 (comment), I wonder whether it would be better for us to have a single context per thread that we update (it sounds like engaging a context in a thread may be required for kernel fusion in the future, and engagingmany contexts in a thread may wreak havoc on this). This would mean that |
eriknw commentedJun 29, 2023
JIT tests are now passing in CI for Linux and Windows! 🎉 |
| jit_ffi=FFI() | ||
| defregister_new(name,jit_c_definition,*,np_type=None): |
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.
Should we remove thename argument here? It's required to be the same name as the typedef, and I bet we could determine the name fromjit_c_definition.
| __call__=TypedUserUnaryOp.__call__ | ||
| defregister_new(name,jit_c_definition,input_type,ret_type): |
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.
Similarly, should we get the name, input type, and return type from the C definition here?
Alternatively, shouldname not be required to match the name used in the C definition?
Should we call this function something else, such asgb.unary.ss.register_jit?
Also, should we consider addingregister_anonymous or allowing it to be anonymous?
eriknw commentedJul 5, 2023
This is going in! We still need to add some tests and documentaion, so SuiteSparse 8 is not yet "officially" supported. The metadata only allows SuiteSparse 7. This also gives us more time to review and refine the JIT. Feel free to add review comments to this PR even after it's merged. |
eriknw commentedJul 5, 2023
btw, the JIT is working for CI for all OSes with both |
Uh oh!
There was an error while loading.Please reload this page.
This begins updating to SuiteSparse:GraphBLAS 8.0.0, which added the JIT and contexts. Currently, this PR adds support for context and updates the configurations. We still need to support the JIT.
See release log here:https://github.com/DrTimothyAldenDavis/GraphBLAS/releases/tag/v8.0.0
Contexts
There are two types of contexts: global, and thread-local.
gb.global_context.gb.ss.Context().with gb.ss.Context(nthreads=4):.c.engage()andc.disengage()methods that are used by__enter__and__exit__.Local contexts do not stack. The inner context replaces the outer context here:
I don't know if we want to try to keep track of the stack ourselves. Perhaps if/when somebody needs such a workflow.
The other tricky thing with contexts is that
nthreadsandchunkswere removed as descriptor options. This is awkward for us, since I like the syntax ofexpr.new(nthreads=8)andC(nthreads=8) << exprand would like to keep it (and not add incompatible changes). So, I kept it. If any context option is used in**opts(which is typically for the descriptor), then we either update the current context that the user explicitly engaged, or we create a new context whose lifetime matches the descriptor lifetime. This ought to work well enough in practice, but I'm not fond of relying on__del__to disengage the context. I would prefer if e.g.nthreadswas still part of the descriptor.Configs
Two changes happened with configuration in SuiteSparse:GraphBLAS:
nthreadsandchunkswere removed (they're now handled by contextWe have some choices for how we can handle these.
First, the global config can have all configurations and global contexts:
Second, we can have a new
global_contextobject:Third, we can have a new
jitorjitconfigobject:So, if we want
global_contextand/orjit/jitconfigobjects, we could remove one/both of them from the global config, which simplifies the global config considerably. For example, here's the global config without the JIT options:If wealso removed the global context items from the global config, we get:
@jim22k@SultanOrazbayev how would you like to organize the global configuration and global context? It's helpful to expose the global context so users can do
global_contect.engage()to reset the context. I also like keepingnthreadsin the global config (which gets and sets via the global context). So, my main question I'm struggling with is whether to split out the jit config.Finally, we don't yet support getting or setting functions such as malloc, free, print, and flush (the third change in 8.0.0).
JIT
Coming soon!