search

vincefn / pyvkfft

Python interface to VkFFT
MIT License
51 stars 6 forks source link

Allow separate queue for fft and ifft #17

Closed isuruf closed 1 year ago

vincefn commented 1 year ago

Hi @isuruf , @inducer - sorry for the long delay, as you can see I'm still undecided regarding which queue policy to follow, but it would be good to finalise this soon.

Using the queue supplied as a parameter to fft or ifft as the first choice is very clear.

However when no queue is passed to fft/ifft, I don't like the idea of having another two (or three) extra choices (the app, source or destiantion queues) - that will be confusing.

So in the end the two possibilities:

  1. use the FFT app queue and ignore the array queues. This has the advantage of using the same approach as the cuda API (the arrays don't have a stream attribute AFAIK), and avoids issues with mismatched source and destination arrays queue, and has no backward-compatibility issues
  2. use the source and destination arrays (hopefully matching) and deprecate the app's queue (as in the current PR). This allows to use the embedded queues and rely in with_queue but I don't use this personally

So I prefer 1) which seems clearer but if there is a widespread reliance on the embedded arrays queues then maybe 2) would be better. Let me know you thoughts.

inducer commented 1 year ago

I think the key point is that a single app object could validly be used, concurrently, by work going on in multiple queues, and in that case, the app's queue is an unhappy default. The queue that comes with the array is much more likely to be right.

As for input and output having separate queues, pyopencl only ever considers queues on inputs, at least for now. https://github.com/inducer/pyopencl/issues/668. But if the information is used, I think it's wise to check that things match, at least in __debug__ mode.

vincefn commented 1 year ago

I think the key point is that a single app object could validly be used, concurrently, by work going on in multiple queues, and in that case, the app's queue is an unhappy default. The queue that comes with the array is much more likely to be right.

Ok, you probably have a broader view of usage of pyopencl queues, so let's do it your way (https://github.com/vincefn/pyvkfft/pull/17#discussion_r894039098). I'll try to add warning if queues don't match the app's - with a way to disable it.

I need to finish handling strides first...

vincefn commented 1 year ago

Note that I am now editing this in https://github.com/vincefn/pyvkfft/tree/queue