Skip to content
Snippets Groups Projects

`create_kernel` API Update

Merged Jan Hönig requested to merge hoenig/pystencils:create_kernel_api into master

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.

Edited by Jan Hönig

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Jan Hönig mentioned in issue #21 (closed)

    mentioned in issue #21 (closed)

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    • 328cd5fb - WIP moving back to 3.8 part 2

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 9 commits

    added 9 commits

    Compare with previous version

    Toggle commit list
  • Author Owner

    I 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 of create_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 @kuron

  • Jan Hönig added 1 commit

    added 1 commit

    • 50ce2bb1 - Merging top level create_kernels

    Compare with previous version

  • Author Owner

    The 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 and backend. target seems to be originally intended as cpu or gpu, whereas backend seems to be c, llvm or cuda. We then introduced opencl which is currently both target and backend. However it clearly should be only backend.

    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_kernels

    Sure, 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 general create_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 and backend

    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 made opencl a separate target is because it uses pyopencl while gpu makes heavy use of pycuda. Perhaps we should rename gpu to offload?

  • Author Owner

    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 and backend

    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 new backend. NEC Aurora should be a different target with c as backend.

    Regarding gpu and opencl/cuda. gpu ist the target. cuda or opencl the backend. I would implement it with legacy code in mind. Meaning if user specifies only target, 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 Kuron
  • Alright, 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.

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 1 commit

    added 1 commit

    Compare with previous version

  • Jan Hönig added 2 commits

    added 2 commits

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply