`create_kernel` API Update
To reduce the kwargs
hell, we introduced dataclasses, which handle the settings of create_kernel
and similar functions.
In addition we introduced type-hints for the API functions to increase usability and simplify development.
Merge request reports
Activity
mentioned in issue #21 (closed)
added 9 commits
Toggle commit listI have experimented a little with the first API changes. See kernelcreation.py. I really like that the setting is now at one place. (Notice that the old way of using
create_kernel
is still possible. The only drawback is that the docstring is now somewhere else.)I also started to introduce Enums. I started with the most obvious one:
target
. I like that the Enum clearly shows, what is not implemented for certain functions (some support only'cpu'
or'gpu'
. However I have mixed feelings about touching so many lines of code, just to introduce the Enum. The "strings" version would still work. The user ofcreate_kernel
would get a deprication warning.The current WIP is the idea to merge the 3 different general
create_kernel
functions (create_kernel
,create_indexed_kernel
,create_staggered_kernel
) as they overlap a lot in functionality. I would use the same dataclass for the settings. This would add 3-4 member variables to the dataclass, which I think is still feasible.Do you think Enums and
create_kernel
merge is a good idea? @holzer @kuronThe merging of
create_kernelS
seems to be a good idea. I run into very few issues.I have continued to investigate the
target
issue. It is even more complicated then I thought. The main problem is, that it is misinterpreted by different developers (including myself). We have two different parameters.target
andbackend
.target
seems to be originally intended ascpu
orgpu
, whereasbackend
seems to bec
,llvm
orcuda
. We then introducedopencl
which is currently bothtarget
andbackend
. However it clearly should be onlybackend
.If you (@holzer @kuron) would take a peak around
pystencils
, you should be horrified, by the missuses of both terms. I am asking again: Do you think it is worth my time, or should we proceed with strings as enums?merging of
create_kernel
sSure, why not. I would leave
create_staggered_kernel
as it is though because all it does is generate the staggering-specific stuff and then hands off to the generalcreate_kernel
.I have mixed feelings about touching so many lines of code, just to introduce the Enum.
I think it's worth it. Using strings for something enumerable is generally a bad practice and string comparisons like
if target == 'cpu': [...]; else [...]
are obviously dangerous because you might miss cases when new enum values get added.target
andbackend
Ugh. Can you change
opencl
to only be a backend while you're at it? It's clearly wrong and confusing and will haunt us if someone ever decides to add another target (FPGA?) or backend (Vulkan? Fortran? Lisp?). By the way, is your NEC AURORA work going to be a separate target or are you pretending it is a GPU? I think the reason why @seitz madeopencl
a separate target is because it uses pyopencl whilegpu
makes heavy use ofpycuda
. Perhaps we should renamegpu
tooffload
?create_staggered_kernel
Yes, that was my thinking as well, since it seems to be very specific and not easy to introduce to the generic
create_kernel
.target
andbackend
Alright, so I will continue with my effort. A student is implementing some FPGA capabilities, although I don't know, whether it will be ever merged to master. That would be a new
target
AND a newbackend
. NEC Aurora should be a differenttarget
withc
asbackend
.Regarding
gpu
andopencl
/cuda
.gpu
ist thetarget
.cuda
oropencl
thebackend
. I would implement it with legacy code in mind. Meaning if user specifies onlytarget
, the backend will be introduced automatically (cpu
->c
,gpu
->cuda
).I will hack on and report back, what other
targets
/backends
I have found.Edited by Michael KuronAlright, I think a rework of the top level API is very much needed for both pystencils and lbmpy. Introducing Enums will be much work, but I highly think that it is worth the effort. When I am back from my vacation I will also do the same in lbmpy (e.g. method should be handled as Enum etc.)
Regarding the backend/target problematic, I also agree. This got messy at some point. I think quite a problem here is also, that we always have one backend which is used by most of the people (c and cuda) and thus maintained while the other (llvm and opencl) is not used so much. Thus, some hacks were introduced to satisfy the pipelines but the usable was not on focus anymore.
A clear separation here would also help very much.
Regarding the fusion of the create functions, I think it might be dangerous. I fear that some parameter options will be problematic or meaningless. I need to look at this a bit closer, but it would be really important that it is intuitive, that the function can create these two versions of kernels.
added 2 commits