Allows the OpenCL queue to run events in Async.

Summary
This PR allows the OpenCL queue to execute in a asynchronous manner. The OpenCL device can do three things at the same time

Read in data from host
Compute data
Send data to host

With the standard queue we can only do one of the 3 at once. So
A = B + C;
D = F + G;

Will first compute and pass back all of B + C into A and then compute and pass back F + G to D. With async we can now pass over the data for B + C, then as B + C is computing we can pass in the data for F + G. Similarly F + G can be computed while the result A is being transferred back to the host.
So pretty nice! I'm going to run some tests tonight to see what the speedup is. Heuristically staring at nvtop while executing tests and it seems like the throughput is much higher!
This PR does a few things

matrix_cl's now have a mutable vector of cl::Events for both read_write and write. When a matrix_cl is to be written to, the kernel's event is added to the write stack while the reads are added to the read_write stack. I chose having a write and read_write stack over just read and write stacks as anytime we want to wait for a read we will also want to wait for the writes to the matrix_cl as well.

The underlying calls to the kernels now directly take in matrix_cl objects. Where before we had

opencl_kernels::add(cl::NDRange(A.rows(), A.cols()), C.buffer(),
 A.buffer(), B.buffer(), A.rows(), A.cols());

We now do
opencl_kernels::add(cl::NDRange(A.rows(), A.cols()), C, A, B, A.rows(),
                        A.cols());

Two new structs write_buffer and read_buffer are used in the struct templates for the kernels to specify whether a matrices buffer is going to be read from or written to.

The event dependencies for each matrix_cl sent to the kernel is defined at initial construction. The kernel struct template utilize new types write_buffer and read_buffer to tell the kernel struct whether the event from the kernel should go on the write or read_write event stack ala

// In the above example, C would have the add kernel's event added
// to it's `write_event_stack` while A and B would have the event attached
// to their `read_write_event_stack`
const global_range_kernel<write_buffer, read_buffer, read_buffer, int, int> add(
    "add", add_kernel_code);

To make the stuff work for (2:4), we have functions select_event, assign_event, and get_kernel_arg which are used in the kernel struct to select / assign events to the matrix_cls and pull out buffers from the matrix_cl's, respectively.

To help clean up a bit of the stuff for (2:4), to_matrix and to_buffer are used in the kernel struct template initialization to give us the correct signatures for the kernels (to_matrix(write_buffer) -> matrix_cl) as well as pass down the correct types for the templates of the kernel functor (to_buffer(write_buffer) -> cl::buffer)

In the device queue set the execution mode to CL_QUEUE_OUT_OF_ORDER_EXEC_MODE_ENABLE if async is available on the device.

There are two instances where the OpenCL program has to halt.
a. Passing data back to an Eigen matrix.
b. Performing a check to verify the integrity of the data.

Because the kernels now use matrix_cl types we have to forward declare the functions that operate on the matrix_cl inplace (aka zeros, triangular_transpose, sub_block) then use them later.

Tests
The tests in async_test tries to thrash the queue a bit and check that we get the same results as a normal stan model. It also checks that read and write dependencies are respected for assignment in cases like
self = self*self;
// or
self = A * B;
A = B * 2;

these can be run with
./runTests.py -j32 ./test/unit/math/opencl/async_test.cpp

Side Effects
The whole thing!
Checklist

[x] Math issue #1163

[x] Copyright holder: Steve Bronder

[ ] the basic tests are passing (in progress)

unit tests pass (to run, use: ./runTests.py test/unit)
header checks pass, (make test-headers)
docs build, (make doxygen)
code passes the built in C++ standards checks (make cpplint)

[x] the code is written in idiomatic C++ and changes are documented in the doxygen

[x] the new changes are tested

The Weirds
There are some weird things going on within the () operator of the global_range_kernel so I thought I'd comment that heavily here. I can add this to the code as well
  auto operator()(
      cl::NDRange global_thread_size,
     // This is going to take the args write_buffer, etc. and turn them
     //  into `matrix_cl` types
      const typename internal::to_matrix<Args>::type&... args) const {
    auto f = make_functor();
    // Gets the correct event dependency stack depending on whether
    //  the buffer is read or written to
    std::vector<cl::Event> kernel_events
        = vec_concat(select_events<Args>(args)...);
    cl::EnqueueArgs eargs(opencl_context.queue(), kernel_events,
                          global_thread_size);
   // get_kernel arg returns either the matrices cl::buffer or what it was passed
    cl::Event kern_event = f(eargs, get_kernel_arg(args)...);
    // This is just a dummy to set off the variadic function
    int dummy[] = {0, (assign_event<Args>(args, kern_event), 0)...};
    return kern_event;
  }

We actually do cholesky_decompose inplace, so I had to give it's signature cholesky_decompose(matrix_cl A). I think it would be a good idea to forward declare cholesky and then call it via A.cholesky_decompose()

Oh, forgot the weirdest weird.
When I try to place the stuff in event_concat_cl.hpp into the internal namespace, everything goofs? I'm not sure if this is some weird namespace issue or if I'm doing something fundamentally wrong.

^EDIT: above comment is false

I might take a crack at some of this so we have a better chance of getting it in for the release tomorrow...

Ay! Just woke up, going to get some breakfast then work on this.
Taking a crack at it would be v appreciated! Then I'll go do the cmdstan stuff after bfast

Cool ! I'm going to look at getting rid of Margs first and then focus on some other TMP stuff to see what we can do

I got it working, but I realized that if we want these kernels to work directly with Eigen types, then we might want to keep the Margs so we can use the same global_range_kernel objects with both matrix_cl and matrix types. Is that something we want to do, though? Or do we pretty much always want to use these kernels through the wrappers one level higher?

This is basically what it looks like:

+namespace internal {
+template <typename T>
+struct to_matrix {
+  typedef T type;
+};
+
+template <>
+struct to_matrix <cl::Buffer> {
+  typedef matrix_cl type;
+};
+}
+
 /**
  * Compile an OpenCL kernel.
  *
@@ -128,8 +140,8 @@ struct global_range_kernel {
    * @param args The arguments to pass to the kernel.
    * @tparam Args Parameter pack of all kernel argument types.
    */
-  template <typename... Margs>
-  auto operator()(cl::NDRange global_thread_size, const Margs&... args) const {
+  auto operator()(cl::NDRange global_thread_size,
+                  const typename internal::to_matrix<Args>::type&... args) const {

Now I'm going through and fixing the kernels to always have the destination first, so we can add the events to it. does that make sense?

I think it does yeah. I fix the unpack kernel in the other PR.

Now I'm going through and fixing the kernels to always have the destination first, so we can add the events to it. does that make sense?

My only concern is that we end up writing code in the future and we forget that's a thing. Or if we write code in the future that has two output matrices

My only concern is that we end up writing code in the future and we forget that's a thing.

this is a good concern. to help us here we also have const - only the first argument is non-const, so reviewing for const-correctness in the calling code should help us out. We can also name the argument "dest" or "destination" or something like that. It seems like an almost equivalent worry that someone calls the kernel and doesn't use the cl::Event that's returned, maybe?

Or if we write code in the future that has two output matrices

can you think of any examples of this?

Actually our diag_inv code does this, where we manipulate both A and tmp_inv. Gonna double check we are catching those.
But yeah, async is weird. We got rid of a lot of boilerplate! But I think some boilerplate may be good here. I think about it like informed consent, "I understand this can go off in a non-sequential order and all subsequent calls to these matrices need to wait for X and Y"

Actually our diag_inv code does this, where we manipulate both A and tmp_inv. Gonna double check we are catching those.

It doesn't sound like tmp_inv is meant to be read from afterwards, no?

, "I understand this can go off in a non-sequential order and all subsequent calls to these matrices need to wait for X and Y"

Which level of the API needs that exposed is the question, right? We're hiding it for anything outside of the opencl namespace right now.
I think we would both prefer a mechanism that ensured you couldn't forget to wait for the event, no?

Yep, tmp_inv is just a temporary storage becase shared memory is too small/ limits you with the block size too much.

Which level of the API needs that exposed is the question, right? We're hiding it for anything outside of the opencl namespace right now.
I think we would both prefer a mechanism that ensured you couldn't forget to wait for the event, no?

Yes we are def on the same page. Let me think about this for 5 minutes. At the moment I like this approach
opencl_kernels::add<First, Second>(cl::NDRange(A.rows(), A.cols()), ...);

It can have a default of First, hides the implementation behind the call, and can attach the event to multiple buffers.
How difficult would the above be? If it would be more complex than 'to do today' then let's shelve it for another PR, verify that non of our current kernels need to attach an event to two matrices, and implement yours.

What does First and Second mean here?

Oh apologies, so the proposal here is to declare in a template what matrices need to have the event from the kernel attached to them. We could use an enum to make that look nice like
enum class event_cl { First = 1, Second = 2, Third = 3, ...}

but we could also just pass 1, 2, and 3 etc. into the template.
I def agree this would be better to have at the kernel definition. Ty didn't think of that. Right now the kernel definition for add is
const global_range_kernel<cl::Buffer, cl::Buffer, cl::Buffer, int, int> add(
    "add", add_kernel_code);

So to say, "This needs to attach the event to the first buffer since it's the output buffer" would we make it
const global_range_kernel<cl::Buffer, cl::Buffer, cl::Buffer, int, int>
add<event_cl::First>( "add", add_kernel_code);

?

Separately, I'm worried we aren't tracking write-after-read hazards at all here... is that taken care of for us somehow? For example, let's say we do
C = A + B
A = B * 2

We need to delay assignment to A until C is done, right?

We need to delay assignment to A until C is done, right?

Yes so in the above we would need to attach the event returned from the kernel for C = A + B to both C and A. That's not something we will know when we define the kernel. So then my original method of explicitly putting the events onto each matrix after the kernels could be used to catch the above. Let me check if that's going on anywhere

Maybe a decent way to deal with WAR hazards here would be to have two event vectors, one for reads and one for writes? and then we somehow tell the kernel builder which ones are read and which are write...

I can't get the current stuff to compile can you? It looks like to_matrix doesn't accept when it's a pointer to a matrix_cl type (which we use for this in calling kernels within the class)

Maybe a decent way to deal with WAR hazards here would be to have two event vectors, one for reads and one for writes? and then we somehow tell the kernel builder which ones are read and which are write...

If you want to try that out can you make another branch off this one so I can take a look? It's not very clear to me how that would look and work though

Oh, I see what's going on

Sean I know we normally wait but if you are around this is ready to be looked at. There's one unresolved convo around the name for get_kernel_arg but I think that's pretty much all that's unresolved at this point
Code looks so much cleaner :pray: :pray: :pray:

Odd, Jenkins has a red X for the GPU tests but it looks like it didn't do anything?

A deep deep sadness flows through my body
https://stackoverflow.com/questions/28100332/opencl-command-queue-creation-on-osx

Like, whatever. Mac is very lame. Out of order mode will still work on windows (LoL) and linux

haaaaaaaaa. that is so so so lame. do we have to special case mac out?

Yep: https://github.com/stan-dev/math/pull/1162/commits/4dfa7208295e775e7c01167a52595ab9d7702b60

Sean can this still fit in 2.19?

I'm waiting for a docker container to build so wanted to write out a spec for an issue this pr has rn.
One reason to slow this PR down is for this race condition
// assume all matrix_cl objects
C = A + B; // (1)
A = B * 2; // (2)

How the above works is wrong (I think) in our current implementation. C would use A and B, but then A is assigned to be equal to B * 2. We need (2) to know that A is being used rn in (1).
In our code, matrix_cl::operator= just moves the buffer from the RHS to LHS matrix_cl and if C has not gone off yet then we end up in a race condition. I'm pretty sure this is correct. I looked into how OpenCL buffers handle this and tracing through their code it looks like (Buffer::operator= -> Memory::operator= -> Wrapper::operator=` it's just doing a move. So idt the above code is safe
The fix here is straightfoward. We are not catching the events that need read blocks so we need to break out the event stack into read and write event stacks. We can make an enum type that can go into the template for defining kernels. We already have templates for kernel definitions so I think we should have that function return a struct that we then call with something like
enum class cl_event = {first = 1, second = 2, third = 3, ...}
...
const global_range_kernel<cl::Buffer, cl::Buffer, cl::Buffer, int, int> add_kern(
    "add", add_kernel_code);
const event_kernel add = add_kern<cl_event::first>() // define which buffer is the output

(above is toy example)
Or, idk something like that. The above tells the kernel that the first buffer for add is written to and the other buffers are read from. So when we actually call this kernel, the first input matrix_cl has the kernel event written to their write_event stack while the second and third matrix_cl has the kernel's event added to it's read_stack.
So with the above I think some stuff starts falling out nicely that we will be able to handle pretty automajically for users / developers

matrix_cl::operator= has to wait for both the read and write stack to finish
matrix_cl -> EigenXd has to wait for all writes
EigenXd -> matrix_cl attaches an event to the write_stack
matrix_cl's that are being used for reading in a kernel only have to pass the write_stack to the kernel and can ignore the read stack
matrix_cl's that are outputs have to have both the read and write events added to the kernels event stack.

This mostly let's us do async in sequences where a matrix is only read from. Which seems good to me

Thanks for writing this up, it all looks good to me. I might summarize it as saying that any time you need to read to a kernel, you need to check its write stack, and before you write to a kernel you need to wait for its read stack. (on write you add to write stack, on read you add to read stack). I think that works out? Taking this to the extreme we get something like a dependency graph to topologically sort but I hope we never have to mess with that. :P Let me know if I can help!

If you can run things async in general...have you considered to calculate the gradients in async? So you could run the cholesky primitive version, then return immediately a adj_jac_vari (or whatever this is called), but calculate the cholesky gradient asynchronously. The adj_jac_vari would essentially be a returned future which locks the main process only whenever the adj of that vari is called. This pattern can be explored in general once we go crazy on threading which we should. Is my idea understandable?

Are you suggesting this because you think we might not end up needing some
computation and the laziness will be more efficient? or because you think
this will allow parallelism?
On Thu, Mar 21, 2019 at 2:55 PM wds15<EMAIL_ADDRESS>wrote:

If you can run things async in general...have you considered to calculate
the gradients in async? So you could run the cholesky primitive version,
then return immediately a adj_jac_vari (or whatever this is called), but
calculate the cholesky gradient asynchronously. The adj_jac_vari would
essentially be a returned future which locks the main process only whenever
the adj of that vari is called. This pattern can be explored in general
once we go crazy on threading which we should. Is my idea understandable?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/1162#issuecomment-475263116, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAxJ7L7i-1kzgV0wonMEfJcC_fFgpeZdks5vY517gaJpZM4b4LL8
.

--
📲

I was thinking about parallelism here. So we would calculate the gradients while the main program continues already.
I will even consider this for ode s as the prim version is very cheap to get while the gradients are much harder to get...this allows to split of independent tasks from the main process without any user intervention! However some work is doubled in such an approach so it’s not applicable everywhere.

Thanks for writing this up, it all looks good to me. I might summarize it as saying that any time you need to read to a kernel, you need to check its write stack, and before you write to a kernel you need to wait for its read stack.

lol yes that's a much easier way to say it

If you can run things async in general...have you considered to calculate the gradients in async?

So the big catch here is that once the queue is told to go back to the CPU that becomes a stopping point. All previous events need to finish so we can go gpu -> cpu.
Or maybe I'm misunderstanding?

I think he's talking about a much grander framework for async in the Math library as a whole. I think it might actually make sense, since we can feed the async gradient computation with any shared data from the current function evaluation at the current inputs. But doesn't each function's gradient rely on the gradients of its inputs? My gut here is that this would be a huge, fairly complicated change in exchange for a theoretical maximum of 2x parallelism (if I'm thinking about this right, you could now just calculate a gradient at the same time as a function evaluation, but not like all gradients in parallel with each other or anything, because they must also be serialized).

Ahh icic. Yeah Sebastian the main 'win' with this Async imo is handling the async you get across threads. Cuz then multiple threads can be going off, hitting the same job queue, and executing as soon as they are able

@seantalts : I have simpler things in mind. The partial derivatives of a function wrt to the operands can be calculated while the main program proceeds in doing the forward sweep of reverse mode (which is to build the tree). Only once the reverse mode adjoint propagation is performed the results of the partials need to be ready. So if you are able to quickly return the function value, then you can asynchronously calculate the partials.
@SteveBronder Ok, I see. Now we do cpu -> gpu (now in async) -> cpu ... but then the cpu is waiting a ton of time for the gpu to finish, right? I am wondering how we can get the cpu busy in the meantime; but this beyond this PR.

Yep, the CPU is currently unoccupied when the GPU does its job. Once async is finished, occupying the CPU becomes simple.
For instance [here]https://github.com/bstatcomp/math/blob/develop/stan/math/rev/mat/fun/multiply.hpp#L189). The CPU would send adjAB and A or B to the GPU (whichever is bigger), calculates the other product and copies back the result. So in pseudocode something like:
asyncSendToGPU(adjAB)
asyncSendToGPU(A)
adjB = asyncMultiply(adjAB,A)
asyncReadFromGPU(adjB)
adjA = adjAB*B
waitForAsyncReadOf(ajdB)

If we are lucky, the wait finds that adjB is already copied back and just moves on. The current plan is not to have the read backs be blocking, but we could also change that. Although that seems a bit dangerous if a developer doesnt add a wait after an async read.
The trick is guessing whether you will be faster that way.  But this kind of optimization could be done during warmup as we have already discussed. These kind of self-adapting strategies to heterogeneous architectures is also a very hot topic in the GPU community and would be a worthy contribution to that community. Although that is not the goal here which is speeding up Stan :)

@wds15 one longer term thing I'm thinking about related to what your saying (idk how far off but could b a big V far off). With OpenCL 2.0 we can pass over more than just primitives. So at that point we can send over whole pieces of the expression graph to be evaluated on the OpenCL side and with the async setup right that would just happen in async with no effort
But I think there's a lot to do before we start thinking about that

Sean how about this?
const global_range_kernel<read_write_buffer, read_buffer, read_buffer, int, int> add("add", add_kernel_code);

I think this is possible? We would just need some clever templating and a struct of typedefs?

That seems like a pretty good API to me!

Alright so I put up what I have so far on the branch below. The main issue is that since we are modifying the matrix_cl event stacks we can't have const matrix_cl&'s in the signature. And only references leads to errors like

./stan/math/opencl/cholesky_decompose.hpp:86:51:
error: cannot bind non-const lvalue reference of type ‘stan::math::matrix_cl&’ to an rvalue of type ‘const stan::math::matrix_cl’
           A_21, transpose(lower_triangular_inverse(L_11)));

But the add and copy tests pass so far. Gonna read into the best/easiest way to move those around
https://github.com/bstatcomp/math/tree/opencl-api-async
Sean do you have any ideas?

**if I remove all the references in signature then it's fine, though I feel like there has to be an easier way to let things be a reference sometimes and not others. something something forwarding

Okay actually this was pretty simple and I just had to make the add_events() const and the event stacks mutable and then everything can be const refs again. Except for cholesky? Going to figure that one out tmrw.
Also I'm guessing the next jenkins run will fail because, while the API is all setup idt I sat down and looked at it hard enough

Alright so I got this working on my laptop and desktop, need to write one more test then a sanity check and then it should be good for review!
I updated the PR at the top to give a better overview of what's changed. While it looks like a lot has happened it's mostly aesthetic changes to the kernel calls. Though copying has changed significantly

Awesome! feel free to tag me when it's ready, and thank you!

@seantalts think this is ready to rock!

Ben is correct, but he says "except for data and transformed data," which
are the ones I'm worried about here. Unless you are saying there's some
reason those types of matrices wouldn't get events?
On Mon, Apr 8, 2019 at 00:41 Steve Bronder<EMAIL_ADDRESS>wrote:

@SteveBronder commented on this pull request.
In stan/math/opencl/matrix_cl.hpp
https://github.com/stan-dev/math/pull/1162#discussion_r272883791:

cl::Event assign_event;

cl::CommandQueue queue = opencl_context.queue();
if (event_type == event_cl::read) {

 return write_events_;

} else if (event_type == event_cl::write) {

 return read_write_events_;

}
}

/**

Get the events from the event stacks.

@tparam event_type Specifies which event stack to add to.

@param new_event The event to be pushed on the event stack.

*/
template <event_cl event_type = event_cl::write>
inline void add_event(cl::Event new_event) const {

From Ben's comment here I thought the container holding these will be
destroyed between iterations? Or is the stuff here preserved across
iterations? If so then yeah I'll try to walk through the code and see where
the best place is
https://discourse.mc-stan.org/t/efficient-static-matrix-type/2136/6?u=stevo15025
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/1162#discussion_r272883791, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAxJ7OJVpAG2_HYfBfPWRbxLJgMVzM5jks5veshggaJpZM4b4LL8
.

Ben is correct, but he says "except for data and transformed data," which
are the ones I'm worried about here. Unless you are saying there's some
reason those types of matrices wouldn't get events?

I think it matters how the RAII works here. Since the matrix_cl objects are constructed outside of the data and transformed data blocks. I can slam this into cmdstan with a printf when I get home to see whether these get destroyed between iterations

But that means we have this time bomb where like, if we want the Stan compiler to start emitting code using matrix_cls explicitly, we have to remember to fix this. In general I'd rather keep things tidy as we go along; much less to keep in my head.

Yeah that makes sense, wrt to that where would be the cheapest place to put the function that cleans up the event stack? The safest while still being cheap-ish is by specializing vec concat. Where we can allocate the size of the new vector ahead of constructing it, then in the recursive step pop off any completed events.
Though if we are talking about having the compiler generate code with matrix_cls explicitly, at the beginning of the iterations couldn't we just have a A.clear_events() function that the compiler also generates? idk how the compiler will do this stuff so I'm totally guessing in whether or not that's possible or reasonable. Personally I think we are still v far away from doing that. We should write the rev versions of + - * etc. and use the gpu stuff a lot more before we start talking about that

Like I would imagine the compiler could do something like
matrix_cl A_cl(A); // Passing Eigen data matrix over to OpenCL
...
for (sample_iters) {
A_cl.clear_events(); // Iterates over the event stack and clears out completed events
...
}

ftr I've never looked at the algorithm or generated code so I have no idea what's feasible here

Yeah it could do something like that. I still feel kind of uncomfortable because this (and/or the caches) is the first time we're adding state to the Math library, and I'd really like it to continue behaving as if it was a stateless library. That means that hidden side effects can't become visible ideally no matter how you use it.

So thinking about this a little harder, it seems like:

we can meaningfully build up multiple read and write events during normal matrix operations that we need to keep track of
all matrix_cl GPU I/O happens through copy
Right now, copying from GPU waits for all the write events, which makes sense. At that point we can clear the write queue.
Right now, copying TO GPU doesn't wait for anything, but intelligently adds a write event and enqueues the copy to happen after that. I think without much performance loss for normal code, we could instead have copy-to-GPU wait for all read events and clear the read queue.

What do you think of that idea? More comments to come.

Your concern is real and justified, been taking my time on this PR to make sure we are comfortable and clear on how we want this to look since the gains are cool and good but there's def risk in not doing this nicely.
I like your idea, I'll take a stab at hashing that out this weekend to see how it looks and feels. idt the CPU->GPU wait will be that bad. For (4) If we didn't want to wait, we could do a getInfo on each event in the read stack and pop out all the completed events

Though my preference is to keep it simple and wait there to clearing out the whole read stack. Gonna try both and if I can make the non-wait look nice we can talk about that

Ah I think actually the copy-to-gpu should have the enqueue command wait on all the read events for the buffer, right? And the copy-from-gpu should have the enqueue command wait on all of the write events to the buffer? And then copy-from-gpu calls copy_event.wait() and we can clear the write queue. and copy-to-gpu we could either just copy_event.wait() or add use https://www.khronos.org/registry/OpenCL/sdk/1.1/docs/man/xhtml/clSetEventCallback.html to have the copy_event clear the read event queue on that matrix_cl, but need to double check std::vector is threadsafe for that pattern.

Ah I think actually the copy-to-gpu should have the enqueue command wait on all the read events for the buffer, right?

Yes, we normally don't reuse the matrix_cls like that but yeah we def should

And the copy-from-gpu should have the enqueue command wait on all of the write events to the buffer? And then copy-from-gpu calls copy_event.wait() and we can clear the write queue. and copy-to-gpu we could either just copy_event.wait() or add use https://www.khronos.org/registry/OpenCL/sdk/1.1/docs/man/xhtml/clSetEventCallback.html to have the copy_event clear the read event queue on that matrix_cl, but need to double check std::vector is threadsafe for that pattern.

Yeah this is true, I'll check if that pattern works for us

Actually, could we also use callbacks on the check functions? From below, it looks like we could have it throw like it does now but not have to wait for the read back to the cpu
https://stackoverflow.com/questions/55492683/opencl-c-bindings-how-to-implement-a-callback-for-enqueuewritebuffer-competit

Hmm, I'm gonna try that later today, think could just throw a lambda into the call argument

The set callback stuff was confusing to me so I set this up so that

host -> device copies wait for the matrix_cl's read and writes, then clears all of them before the write to device
device -> host copies wait for the matrix_cl's writes, then clears the write stack before the read from device to host

Once the tests pass this is good for review

Do you have a sense for which files were just moved around and which ones have new changes? If you could write that down or mark them that would help me a lot in reviewing so I can skip those :D

Yes I'm out rn but I'll write it down tmrw and ping you

Do you have a sense for which files were just moved around and which ones have new changes?

The files below have meaningful changes, the rest are just conforming to the new kernel_cl structure
stan/math/opencl/assign_events.hpp
stan/math/opencl/buffer_types.hpp
stan/math/opencl/cholesky_decompose.hpp
stan/math/opencl/copy.hpp
stan/math/opencl/get_kernel_arg.hpp
stan/math/opencl/kernel_cl.hpp
stan/math/opencl/matrix_cl.hpp
stan/math/opencl/select_events.hpp
stan/math/prim/arr/fun/vec_concat.hpp
test/unit/math/opencl/async_test.cpp

Apologies this PR got a bit big, ended up being a pretty decent rewrite to put the async stuff in here nicely

This looks awesome, I really like the API you came up with here with the read and write buffer type aliases. A++++

Ty!!

I think we have a few implementation details to work out still with handling the event vectors but I think it's pretty close.

It's close but yeah could def use your fine eye here. The only thing I'm still contentious about here is whether we are future proofing too much by clearing out the read/write stacks. Also going to make a note at where I tried to do some 'perfect forwarding'. I feel like the perfect in perfect  forwarding is you needing to do the forwarding perfect and not the C++ does the forwarding perfectly for you.

I assumed when reading this that sub_block, zeros, and triangular_transpose were just moved to a new location, right?

Yep!

Something weird is throwing on the MPI tests?
g++: internal compiler error: Killed (program cc1plus)
...
<builtin>: recipe for target 'test/unit/math/rev/mat/fun/matrix_exp_multiply_test.o' failed
make: *** [test/unit/math/rev/mat/fun/matrix_exp_multiply_test.o] Error 4
make: *** Waiting for unfinished jobs....
g++: internal compiler error: Killed (program cc1plus)

tagging in @serban-nicusor-toptal - this ran on one of the new spotfleet nodes with -j400. did it die just because it ran out of RAM? I'll kick of a re-run now just in case.

@seantalts Moved the fleet to the updated Template. Should end successfully now.

Gotta update to the latest, but also seeing
make -jnull test-headers

from the header test. Going to run this again

I think that's another fleet template issue, right @serban-nicusor-toptal ?

Working now, must have been something goofy

Looking at it now, tbh I don't know why it's set to null ...
sudo echo "export CXX=clang++-6.0" >> /etc/profile
sudo echo "export GCC=g++" >> /etc/profile
sudo echo "export MPICXX=mpicxx.openmpi" >> /etc/profile
sudo echo "export N_TESTS=300" >> /etc/profile
sudo echo "export PARALLEL=40" >> /etc/profile

This is in the latest version of the template. ( Currently used in the EC2 instances )

Ty! All looks good now, like was just a goof

@seantalts PTAL

It's close but yeah could def use your fine eye here. The only thing I'm still contentious about here is whether we are future proofing too much by clearing out the read/write stacks.

Would it help to have a call about this or something? To me it feels like when you're building a library like this, you should avoid purposefully leaving in memory leaks and generally make sure the calls that are available are safe and do what they say they will do.

Also going to make a note at where I tried to do some 'perfect forwarding'. I feel like the perfect in perfect forwarding is you needing to do the forwarding perfect and not the C++ does the forwarding perfectly for you.

Where do you think we need perfect forwarding? Herb talks a bit about that in the talk.

I think those are probably fine. Anything that doesn't require //NOLINT at every call site

Lol fair, let's do this in a separate PR since this one has already gotten pretty big

Would it help to have a call about this or something?

Yes let's def chat tmrw, going to try to write my part of the paper today.

I think we need to verify whether Eigen types qualify as "Cheap to move" (from the chart) or not and then do what the chart says, basically. I am actually not sure which one they'd fit in to - I would lean towards thinking they are probably cheap to move because Eigen devs are smart but who knows, they might not be using much C++11 stuff.

Yeah I agree, tbh I'm fine with setting it to pass by value for now and optimizing this later

Would it help to have a call about this or something? To me it feels like when you're building a library like this, you should avoid purposefully leaving in memory leaks and generally make sure the calls that are available are safe and do what they say they will do.

Yeah let's chat on the phone tmrw. Is cleaning those event stacks a memory leak issue? I assumed RAII would handle cleaning these up when the matrix_cl goes out of scope?

@serban-nicusor-toptal could you take a look at the Jenkins windows build to see if something is up? It was hanging last night on the windows tests, reset it this morning but still hanging there

Sean I put the matrix_cl argument to the cholesky as a ref and added the clears for the read stack in the unpacking copy function

some weird notes:

There are conflicts, so I'm surprised the tests are running at all...?
there is a backlog of tests requiring Windows which you can see on the Jenkins home page
that backlog seems to be caused by @wds15 TBB PR which is taking a really long time to build on all platforms...?

Yeah let's chat on the phone tmrw. Is cleaning those event stacks a memory leak issue? I assumed RAII would handle cleaning these up when the matrix_cl goes out of scope?

Do you still wanna chat? Missed that message, sorry. Text me & we can find a time. the vectors will be cleaned appropriately when a matrix_cl goes out of scope. But having unbounded memory growth through side effects during normal use is nice to avoid if possible, and it seems easy enough.

There are conflicts, so I'm surprised the tests are running at all...?

Oh sorry totally missed those, I'll look at those when I get home

We'll wait until Jenkins is hunky dory before doing anything here, but to update there is a discussion going on here where Rok and I check and verify the the open source pocl and intel OpenCL IDCs handle async events s.t. we can store and pass them around at our level in an std::vec and feel safe about when they are destroyed.
So far I think it looks like they are good? If so then I think the only remaining thing here is one last spot check to make sure I didn't flub up any async call dependencies and I think this is good to go. @seantalts do you have any check items that still need to be addressed besides the above?

We'll wait until Jenkins is hunky dory before doing anything here,

This might not be until Monday or later. The old iMac running Jenkins Master may have finally bit the bullet.

This might not be until Monday or later. The old iMac running Jenkins Master may have finally bit the bullet.

Yikes! That's fine, gives me this weekend to give it another spot check

Odd, it wont let me comment on some of your review comments

Yeah, and even having read about it a few times I'm still sort of hoping it will line up the Args template parameters with the args matrices appropriately. What do you think about just writing the variadic assign_event?

Yeah I think that's reasonable

Okay, pretty sure I've hit up all the review comments that don't have Qs associated with them. This is a pretty big slab of code so feel free to do the review in bits and pieces

Sean it's still kind of unclear to me why we want to clear out the event stacks when going from CPU->OpenCL and vice versa
To my understanding, the intention of clearning the event stack is that when we move to the stan3 compiler and can generate code that directly uses matrix_cls then we don't want the event stacks to grow off to infinity. But what we are doing here doesn't really satisfy that since we only clear the stack when going from CPU->OpenCL and OpenCL->CPU. In my mind we would wait and clear these event stacks between each iteration of the sampler at a higher level.
eod, anything related to future proofing for stan3 just feels very vague to me atm and I'd rather worry about it when we get there. Or does clearing the event stacks the way we do now satisfy the above intent?
I know previously you had a concern about how these are cleaned up, but we looked over the implementation and figured out the RAII on cleaning these up works fine.
Is there another reason?

@rok-cesnovar Could we move the
check_nan("cholesky_decompose (OpenCL)", "Matrix m", A);
check_diagonal_zeros("cholesky_decompose (OpenCL)", "Matrix m", A);

in the recursive cholesky_decompose here to go off in the prim code after the recursive code is run?
With the Async stuff, the checks here create a blocking event (we need to wait to throw an error here) so the Async stuff has to stop and lineup here.

Yeah, we can move those checks. I can do that in the morning.

^I got it!

Thanks!

We want to keep the math library looking reasonably stateless, and clearing events and not growing them to infinity is part of that contract.

Sundials seems to be goofed on windows?
C:\Rtools\mingw_64\bin\ar.exe: unable to rename 'lib/sundials_4.1.0/lib/libsundials_idas.a'; reason: Permission denied

Logged in to the machine to clear the workspace and restarted the stage
On Thu, May 2, 2019 at 11:54 Steve Bronder<EMAIL_ADDRESS>wrote:

Sundials seems to be goofed on windows?
C:\Rtools\mingw_64\bin\ar.exe: unable to rename 'lib/sundials_4.1.0/lib/libsundials_idas.a'; reason: Permission denied
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stan-dev/math/pull/1162#issuecomment-488728752, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGET3GZE32FMSX64TUKCXLPTMFDBANCNFSM4G7AWL6A
.

Do you mind if I just go through and try to do some edits myself? I want to suggest some particularly annoying renames (like the read/write/readwrite buffers should probably use the terminology from C++: in, out, and in-out) and I feel bad going back and forth so many times on this PR (though it is huge and has multiple great refactorings within it)

Do you mind if I just go through and try to do some edits myself? I want to suggest some particularly annoying renames (like the read/write/readwrite buffers should probably use the terminology from C++: in, out, and in-out) and I feel bad going back and forth so many times on this PR (though it is huge and has multiple great refactorings within it)

Yeah go for it!

 I took a pass over copies based on our conversation with the following rules:

Don't wait for copies TO opencl devices in general
Always wait for write events before writing or reading
Always wait for read events before writing

If this looks good to you I think this is finally good to go :) Great work everyone!

Nice! Those rules are correct. I'll rubber duck your commits and the overall PR
Glanced over it and all looked good. Have to get used to the in/out_buffer names but that's fine

Alrighty looked it over and I agree with the changes! Once you approve and tests pass this should be good to go!

TIL about https://en.cppreference.com/w/cpp/language/function-try-block !
Lots of good stuff in there, thank you for catching all of that.
I think my last question is: were there other kernels that should be switched to in_out (read/write) buffer args besides cholesky_decompose? like the PR mentions that zeros, triangular_transpose, sub_block are in-place - should we mark them as such as well?

I went through all the kernels and did the following changes:

diag_inv(in_out, in_out) - was out, out
A is the matrix we are inverting, tmp_inv is the batch of identities that is also modified in the kernel, but only used as temp data, not used afterwards

neg_rect_lower_tri_multiply(in_out, in) - was out, in
A is again the matrix we are inverting, temp is only used as input

scalar_mul_diagonal(in_out) - was out

triangular_transpose(in_out) - was out

both are in-place
Zeros and sub_block just set the values, they dont read them so I think leaving them as out is fine.

Soooo... you guys are the best!
Primitive Cholesky on develop:
Benchmark                          Time           CPU Iterations
BM_cholesky_cl/chol_500     11901092 ns   11807470 ns         52
BM_cholesky_cl/chol_750     19108036 ns   19092002 ns         37
BM_cholesky_cl/chol_1000    28274247 ns   28235917 ns         24
BM_cholesky_cl/chol_1500    49715671 ns   49664310 ns         14
BM_cholesky_cl/chol_2000    76176244 ns   76114813 ns          9
BM_cholesky_cl/chol_3000   161095747 ns  161030213 ns          4
BM_cholesky_cl/chol_4000   280346213 ns  279051062 ns          2
BM_cholesky_cl/chol_5000   489755846 ns  482237291 ns          2
BM_cholesky_cl/chol_6000   659892390 ns  655835495 ns          1
Primitive Cholesky on opencl-async:
Benchmark                          Time           CPU Iterations
BM_cholesky_cl/chol_500      8680560 ns    8536700 ns         72
BM_cholesky_cl/chol_750     13683231 ns   13657210 ns         51
BM_cholesky_cl/chol_1000    20426311 ns   20405342 ns         34
BM_cholesky_cl/chol_1500    38293310 ns   38221407 ns         18
BM_cholesky_cl/chol_2000    59945334 ns   59814637 ns         11
BM_cholesky_cl/chol_3000   134024679 ns  131870909 ns          5
BM_cholesky_cl/chol_4000   255157210 ns  224711788 ns          3
BM_cholesky_cl/chol_5000   412889673 ns  348433566 ns          2
BM_cholesky_cl/chol_6000   618730698 ns  511490830 ns          2
A minimum ob 15% reduction in time, best cases even 30+%. Thank you!

Mamma Mia!!

Sean I went ahead and changed instances of read with in and write with out. For a minute here we had in/out_buffer and read/write_events. idk why we would have two names for the same things that happen at the same level so I changed then all to in/out

Thanks for bringing that up! I think the key insight for me here was realizing that we have two different interfaces - one that is for developers writing kernels, and another is for the folks dealing with the underlying async events. At the kernel-writing level, we're just annotating the arguments to the kernel with their directionality essentially in C++ terms. At the level below that, we take those annotations and do whatever underlying OpenCL actions that need to happen based on those argument directionalities.  We happen to use those indications to keep track of read and write events for matrices. That’s why it’s okay that in/out does the same thing currently as out - we’re asking kernel writers to label kernel arguments according to the user-visible semantics of the kernel, which is an orthogonal concern to the underlying opencl event stacks and other async management we might do.
There is a separate question for whether we prefer to think of the read and write events as "in" and "out" events, in which case the names would be unified. I have a slight preference towards read/write events because it just sounds better to me, but curious what @rok-cesnovar and others think?

I think the key insight for me here was realizing that we have two different interfaces - one that is for developers writing kernels, and another is for the folks dealing with the underlying async events

But those people are often one and the same :)
So that means the single developer has to remember one more piece of circuit logic and im not a fan of that
If you look in the code theres also times where, at a higher level like in copy, we have to use the add_read_event. So its unclear to me where the distinction between the two groups sits. sub_block is also a good example of this
Idc if we have in/out or read/write. I originally did read/write because that's what OpenCL uses for in/out. But in/out is also fine and has precedence in c++. So either is fine, being consistent is more important

I am more accustomed to read/write coming from the GPU/embedded background but actually prefer in/out here:

I think the way we have built the OpenCL core in Stan Math, anyone that would want to add a new kernel doesn't really have to go through the whole OpenCL experience one would have to go through to call a kernel in a standalone C++ app. So there is no real need to follow OpenCL uses of read/write. It might even be confusing. I think its more like adding something to the C++, so using the C++ style is better.
we use [in], [in,out] in doxygen comments and I think its easier to double check to double check if we have [in] -> in_buffer, [out] -> out_buffer, etc.

@rok-cesnovar do you think that the event queues should also be named after the parameter directionality? Or should they follow the OpenCL conventions since they are at that level of abstraction? To be clear, I think we should keep in/out/inout for the argument declarations and use read/write for the event queues since the former are at the C++ interface level and the latter are at the OpenCL level.

But those people are often one and the same :)
So that means the single developer has to remember one more piece of circuit logic and im not a fan of that

You could use this argument against any form of abstraction, and yet we all still enjoy using programming languages without calling all variables "registers" or something. The fact is that in/out arguments refers to a different thing than read/write event queues, so giving them the same name is more confusing as it implies equivalency. The developer crossing that boundary actually needs to remember that those concepts are different. A key motivating example in the current code is that an out parameter requires waiting on both read and write event queues, illustrating that the mapping is not 1-to-1.

But those people are often one and the same :)
So that means the single developer has to remember one more piece of circuit logic and im not a fan of that

You could use this argument against any form of abstraction, and yet we all still enjoy using programming languages without calling all variables "registers" or something.

You can and it's a good argument! Line between the right level of abstraction is not always clear!

The fact is that in/out arguments refers to a different thing than read/write event queues, so giving them the same name is more confusing as it implies equivalency. The developer crossing that boundary actually needs to remember that those concepts are different. A key motivating example in the current code is that an out parameter requires waiting on both read and write event queues, illustrating that the mapping is not 1-to-1.

idt they really do tho? Like I get what your saying here and don't disagree. But I think we would explain this as

in_buffers depend on the out_events while out_buffers depend on the results of in_events and out_events

vs

in_buffers depend on the write_events while out_buffers depend on the results of read_events and write_events

Personally I like the first one better, but ftr, if you really really really really think this abstraction is good and necessary then I'm fine with reverting the last two commits by the powers of the lead of the technical working group.
And again we also have accessible functions for getting directly to the in_events (read_events) and out_events (write_events). Like in sub_block we both call OpenCL functions directly but also call a kernel. The OpenCL call route then has to call this->add_out_event(copy_event); because this is an output. So breaking the read/write and in/out into two different layers doesn't really feel great because we have to cross that abstraction layer. If both of these layers were independent then I'd get having two names, but they are not really.

I think for what you want we would have to write an abstraction layer over all the OpenCL methods we currently call so everything gets taken care in a seperate layer of abstraction. e.g. this
std::vector<cl::Event> kernel_events
  = vec_concat(A.out_events(), this->in_out_events());
cl::Event copy_event;
queue.enqueueCopyBufferRect(A.buffer(), this->buffer(), src_offset,
 dst_offset, size, A.rows() * sizeof(double),
 A.rows() * A.cols() * sizeof(double),
 sizeof(double) * this->rows(),
 this->rows() * this->cols() * sizeof(double),
 &kernel_events, &copy_event);
A.add_in_event(copy_event);

becomes
copy_rect(A, *this, src_offset, dst_offset, size, ...);

which then alls write/read_events() etc.
which I'm for, but that's probably something for a separate PR

Yeah, that sounds like a separate PR. But I think choosing good concepts is important as early as you can. I'd be happy to let Rok be the tiebreaker here on whether the event queues should have the same names - I have a lot of SWE experience but you're both working on this code the most and this decision is minor enough that it's more important to just choose one.

I would go with read/write queues and in/out buffers. I think Sean's point that its more related to the OpenCL abstraction layer and is more likely to be maintained by people that have a stronger background in OpenCL is on point.
However, Steve is also right that we need to separate the layers in order for this not be confusing. I think we have successfully masked almost everything OpenCL related to the point where a kernel can be viewed as a C-like function that runs in parallel (if you ignore STRINGIFY, __kernel and the thread id functions). Apart from a few keywords and thread ID functions the devs dont really deal with OpenCL. So if possible, it would be great if we could make these two layers separate.
TLDR:

go back to in/out event queues
make a separate issue & PR to make the layers as separate as possible (if it can be done)

slightly offtopic:
The feedback from other devs that have joined the project in Ljubljana with fresh eyes is that this OpenCL backend (adding new kernels and using them) is great and the learning curve is not steep. I have tried contributing the Cholesky kernels back then to the ViennaCL project and you had to have an understanding of almost their entire backend to add a kernel. in CLBlast on the other hand you can easily add your own kernel, but the API to use them then is basically barebone OpenCL.

just a short question: As I understand you plan to have queues for different stuff to do. Would this work still if we are going to sample in a single process multiple chains? httpstan will be doing that and my own threading thoughts go into this direction. This doesn't have to be resolved now - I was just wondering if this has been considered.

Yeah, Steve want to make an issue for the refactor you pointed out? I think it looked like a good idea from the example you gave.
Sebastian, there will be just 1 command queue for the device, but the GPU itself has a couple of components that can be pipelined and this async refactor allows it to do so. The read/write event queues just keep track of the dependencies between these commands in software.
I think multiple chains might function but not efficiently. The math library is designed to not know about or optimize any computation at the sampler level - same reason it's difficult for us to change just the math library to save initial values for iterative algorithms across leapfrog steps, or cache matrices on the GPU, etc. And that's fine - in our architecture, we need to deal with sampler-level optimizations at the compiler level.

Word. Aight I have to do work stuff rn can one of yinz revert this back to b013410? Otherwise I can when I get home

just a short question: As I understand you plan to have queues for different stuff to do.

Nope just one queue! This PR makes that queue much more efficient by giving the GPU better throughput. In the future we'll have one queue per device and a load balancer to figure out which device to send things.

Would this work still if we are going to sample in a single process multiple chains?

Probably? I originally made this PR so that the OpenCL stuff would play nicely with threading. The idea there is that each thread can be sending jobs to the queue and the GPU can interweave the threads jobs together efficiently.
We will have to test it!

Just did the revert back to Rok's last commit that has the read/write for the matrix_cl so one checks pass this should be good to go!


#mongo-optimization-task. Alexander Zenchyk

https://travis-ci.org/github/zeeenjaaa/db-assignments/builds/735073806

Closing the PR as accepted! Thanks!


Next.js does not bring over prebuilds folder into .next/server: "No native build was found for ..."

When using a dependency that uses node-gyp-build prebuilt binaries, next does not seem to pick up those binaries.
The directory it searches for binaries is ./.next/server instead of ./node_modules/zeromq in my case. If I change this and hardcode it to /absolute/path/to/node_modules/zeromq it works because it sees that prebuilds.
https://github.com/prebuild/node-gyp-build/blob/0078d7884dfeb0e708477b3e20bfc4f520c1e2f8/index.js#L26
As you might have noticed I am using a fork that was made on 4.5.0, because that's what was done for https://github.com/zeromq/zeromq.js

[!NOTE]
Reproduce by cloning https://github.com/eulersson/zeromq.js-next.js-errors, npm install, npm run dev. Or by running the Docker container in there.

are you passing the __dirname to the exported func? sounds like you are passing the wrong dir

are you passing the __dirname to the exported func? sounds like you are passing the wrong dir

Many thanks for your response.
Can I set up the __dirname to be different just for one dependency? I guess the __dirname is fine to be at .next/server? I guess the the desired outcome would be to have the prebuilds folder in the .next/server somehow.
Do you know any other project that uses node-gyp-built deps with prebuilds in the context of a Next.js? Maybe it would help to see how people resolve this issue...

You can just “hoist” all the prebuilds from node_modules into that folder if the deps are on latest node-gyp-build. Ie mb move node_modules/sodium-native/prebuilds into .next/server/prebuilds
in a later version all the prebuilds are named so you can have them all in the same folder

If I move them manually and run npm run dev it cleans the entire .next/server and repopulates it missing the files I just copied. It does not seem to persist. I guess somewhere Next.js needs to be informed about those. I don't know what building infrastructure Next.js uses, maybe there's webpack involved underneath but I am not sure.

The problem has been identified, it's with vercel's file tracing tool "nft". I will provide a PR tomorrow. If someone is having this problem go to the issue on vercel's side https://github.com/vercel/nft/issues/391, I will therefore close his one. Thanks @mafintosh for the response :)

Actually I want to say I will fix it not in the elegant way of making the tool be able to dynamically resolve the path but to add a special case. In the future more users using node-gyp-build might run into similar problems but for now I will do it like @styfle suggested in this comment https://github.com/vercel/nft/issues/391#issuecomment-1938785569

Will be fixed if all goes will with this issue resolution https://github.com/vercel/nft/issues/391 with PR https://github.com/vercel/nft/pull/392.

@vercel/nft version 0.26.4 fixes it.


Add MSAA support

Commit 9f437a6a3f210220aad2790fd4465491c8d76612 adds support for SSAA.  I am leaving this issue open until MSAA is implemented.

Commit 0c61605d77ccddd73faea71e063bf13b53275117 implements MSAA for OpenGL, and includes a number of architectural improvements towards supporting MSAA on Vulkan.  MSAA on Vulkan visually appears to work, but there are remaining validation layer errors that need to be addressed that are indicative of remaining architectural deficiencies, so support is currently disabled.
I am leaving this issue open until the remaining Vulkan deficiencies can be addressed.

Commit e5dcb1833bc5099bccc4052a167ec93067dcc8c4 completes the Vulkan MSAA implementation, and passes cleanly against validation layers from <IP_ADDRESS> and <IP_ADDRESS>.


Transaction Sandbox

Create a website where you can write out transactions and run them on a database directly in the browser. Useful way to learn web development stuff, and a great tool for people to test out code. In practice, this will work just like the Caustic syntax unit tests.

Client edits transaction using ace.js and sends to server for execution. Each client is assigned a database on which it executes the transaction. Server replies to client with the result of execution and the current state of its database (all key-value pairs).

Maybe instead of a sandbox, this could be a REPL. Might be slightly more challenging to implement, but it could be significantly more useful.

import caustic.runtime.service._
import scala.concurrent.{Future, blocking}

// Bootstrap an in-memory server.
val server = Future(blocking(Server(memory.LocalDatabase.empty).serve(9090)))

// Construct a client.
val client = Client("localhost", 9090)

// Execute transactions on the client.
client.execute(cons(write("x", 3), read("x"))


memory usage(working set) growing way too much

...and often way too fast. this is really visible when downloading bigger torrents(5-10GB+) and at relatively higher download speed(100 Mbits or so)
on an ancient 4GB machine I use to test stuff PT's working set grows to roughly about 3GB literally in minutes, lets say ~5 at which point the machine becomes totally unresponsible often for quite some time, 5 minutes or more, maybe up to 20. I think, depending on the torrent's size
picotorrent v0.18 amd64
windows 10 v1909
sadly I'm not sure if I had the issue pre 0.18 or it was introduced with it. I think it started with 0.17 or 0.18
offtopic, thanx for making a clean, lightweight torrent client
cheers

Same as #734, I can confirm that the issue is introduced in 0.17. I have checked the older versions and they don't have the same issue.

@armpogart thanks for the additional data point!


Browserify don’t know about process.env.NODE_ENV

React and other libraries need process.env.NODE_ENV for proper minification. Great also for Redux "real world example" workflow.
There are 2 possibilities:

use babel - http://babeljs.io/docs/plugins/transform-inline-environment-variables/
use envify - using transformation in browserify - https://www.npmjs.com/package/envify

I’m not sure which choose. Babel is maybe more simple to implement?

On Yachtacharter I just used envify and it saves 55kB - 6% of JS file size.

Are you sure it doesn't work in the latest version? I've implemented and tested that some time ago.
See https://github.com/actum/gulp-dev-stack/blob/master/gulp/tasks/js.js
There is envify + uglifyify for proper browserify env handling and minification as well as uglify for minification of the whole bundle…

Yep, it works fine. Related commit: https://github.com/actum/gulp-dev-stack/commit/e46282c2a04e308a12c56e16f9e4021cab1a588c

Nice, sorry for that :-)
I did not update our old version on YCH.


[YouTube] Unable to log in: bad username or password

Make sure you are using the latest version: run youtube-dl --version and ensure your version is 2016.08.19. If it's not read this FAQ entry and update. Issues with outdated version will be rejected.

[x] I've verified and I assure that I'm running youtube-dl 2016.08.19

Before submitting an issue make sure you have:

[x] At least skimmed through README and most notably FAQ and BUGS sections
[x] Searched the bugtracker for similar issues including closed ones

What is the purpose of your issue?

[x] Bug report (encountered problems with youtube-dl)

Failed to login and download my Watch Later list. Using netrc, which I haven't edited, worked fine  couple of days ago, but never once since. Works fine without --netrc.
[download https://www.youtube.com/playlist?list=WL]
[debug] System config: []
[debug] User config: []
[debug] Command-line args: [u'--no-mtime', u'--ignore-errors', u'--rate-limit=1000K', u'--netrc', u'--mark-watched', u'--verbose', u'https://www.youtube.com/playlist?list=WL']
[debug] Encodings: locale UTF-8, fs UTF-8, out None, pref UTF-8
[debug] youtube-dl version 2016.08.19
[debug] Python version 2.7.12 - Linux-4.4.0-34-generic-x86_64-with-Ubuntu-16.04-xenial
[debug] exe versions: ffmpeg 2.8.6-1ubuntu2, ffprobe 2.8.6-1ubuntu2, rtmpdump 2.4
[debug] Proxy map: {}
[youtube:watchlater] Downloading login page
[youtube:watchlater] Logging in
WARNING: unable to log in: bad username or password
[youtube:watchlater] WL: Downloading webpage
ERROR: Unable to extract title; please report this issue on https://yt-dl.org/bug . Make sure you are using the latest version; type  youtube-dl -U  to update. Be sure to call youtube-dl with the --verbose flag and include its complete output.
Traceback (most recent call last):
  File "/usr/local/bin/youtube-dl/youtube_dl/YoutubeDL.py", line 691, in extract_info
    ie_result = ie.extract(url)
  File "/usr/local/bin/youtube-dl/youtube_dl/extractor/common.py", line 347, in extract
    return self._real_extract(url)
  File "/usr/local/bin/youtube-dl/youtube_dl/extractor/youtube.py", line 2333, in _real_extract
    return self._extract_playlist('WL')
  File "/usr/local/bin/youtube-dl/youtube_dl/extractor/youtube.py", line 1922, in _extract_playlist
    page, 'title')
  File "/usr/local/bin/youtube-dl/youtube_dl/extractor/common.py", line 659, in _html_search_regex
    res = self._search_regex(pattern, string, name, default, fatal, flags, group)
  File "/usr/local/bin/youtube-dl/youtube_dl/extractor/common.py", line 650, in _search_regex
    raise RegexNotFoundError('Unable to extract %s' % _name)
RegexNotFoundError: Unable to extract title; please report this issue on https://yt-dl.org/bug . Make sure you are using the latest version; type  youtube-dl -U  to update. Be sure to call youtube-dl with the --verbose flag and include its complete output.

same problem here
android@android:~$ youtube-dl -f 43 -u<EMAIL_ADDRESS>-p thisisfake https://www.youtube.com/watch?v=c6oWQ8p0FuI -v
[debug] System config: []
[debug] User config: []
[debug] Command-line args: [u'-f', u'43', u'-u', u'PRIVATE', u'-p', u'PRIVATE', u'https://www.youtube.com/watch?v=c6oWQ8p0FuI', u'-v']
[debug] Encodings: locale UTF-8, fs UTF-8, out UTF-8, pref UTF-8
[debug] youtube-dl version 2016.08.17
[debug] Python version 2.7.12 - Linux-4.4.0-34-generic-x86_64-with-Ubuntu-16.04-xenial
[debug] exe versions: ffmpeg 2.8.6-1ubuntu2, ffprobe 2.8.6-1ubuntu2, rtmpdump 2.4
[debug] Proxy map: {}
[youtube] Downloading login page
[youtube] Logging in
WARNING: unable to log in: bad username or password
[youtube] c6oWQ8p0FuI: Downloading webpage
[youtube] c6oWQ8p0FuI: Downloading embed webpage
[youtube] c6oWQ8p0FuI: Refetching age-gated info webpage
ERROR: c6oWQ8p0FuI: YouTube said: Please sign in to view this video.
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/youtube_dl/YoutubeDL.py", line 691, in extract_info
    ie_result = ie.extract(url)
  File "/usr/lib/python2.7/dist-packages/youtube_dl/extractor/common.py", line 347, in extract
    return self._real_extract(url)
  File "/usr/lib/python2.7/dist-packages/youtube_dl/extractor/youtube.py", line 1321, in _real_extract
    expected=True, video_id=video_id)
ExtractorError: c6oWQ8p0FuI: YouTube said: Please sign in to view this video.

even after update
youtube-dl -f 43 -u<EMAIL_ADDRESS>-p thisisanotherfake https://www.youtube.com/watch?v=c6oWQ8p0FuI -v
[debug] System config: []
[debug] User config: []
[debug] Command-line args: [u'-f', u'43', u'-u', u'PRIVATE', u'-p', u'PRIVATE', u'https://www.youtube.com/watch?v=c6oWQ8p0FuI', u'-v']
[debug] Encodings: locale UTF-8, fs UTF-8, out UTF-8, pref UTF-8
[debug] youtube-dl version 2016.08.19
[debug] Python version 2.7.12 - Linux-4.4.0-34-generic-x86_64-with-Ubuntu-16.04-xenial
[debug] exe versions: ffmpeg 2.8.6-1ubuntu2, ffprobe 2.8.6-1ubuntu2, rtmpdump 2.4
[debug] Proxy map: {}
[youtube] Downloading login page
[youtube] Logging in
WARNING: unable to log in: bad username or password
[youtube] c6oWQ8p0FuI: Downloading webpage
[youtube] c6oWQ8p0FuI: Downloading embed webpage
[youtube] c6oWQ8p0FuI: Refetching age-gated info webpage
ERROR: c6oWQ8p0FuI: YouTube said: Please sign in to view this video.
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/youtube_dl/YoutubeDL.py", line 691, in extract_info
    ie_result = ie.extract(url)
  File "/usr/lib/python2.7/dist-packages/youtube_dl/extractor/common.py", line 347, in extract
    return self._real_extract(url)
  File "/usr/lib/python2.7/dist-packages/youtube_dl/extractor/youtube.py", line 1321, in _real_extract
    expected=True, video_id=video_id)
ExtractorError: c6oWQ8p0FuI: YouTube said: Please sign in to view this video.

Post the output with --dump-pages.

sudo youtube-dl -U && youtube-dl -u<EMAIL_ADDRESS>-p fakepass -v --dump-pages https://www.youtube.com/playlist?list=WL &> ~/yt.log
Output was too big so I attached it.
yt.txt

Same issue here, verified that username and password are correct in .netrc and two factor authentication is off.
log.txt

Same here since yesterday evening GMT 23:00, before this time everything worked fine. Upgraded to latest version (22-08-2016), to no avail. :-(
Perhaps YT's login page changed?
youtube-dl -u %username% -p %password% -v --dump-pages :ytsubs > YTlog.txt
[debug] System config: []
[debug] User config: []
[debug] Command-line args: ['-u', 'PRIVATE', '-p', 'PRIVATE', '-v', '--dump-pages', ':ytsubs']
[debug] Encodings: locale cp1252, fs mbcs, out cp1252, pref cp1252
[debug] youtube-dl version 2016.08.22
[debug] Python version 3.4.4 - Windows-7-6.1.7601-SP1
[debug] exe versions: ffmpeg N-67388-g37425fc, ffprobe N-67388-g37425fc, rtmpdump 2.4-47-gc26c541
[debug] Proxy map: {}
WARNING: unable to log in: bad username or password
YTlog.txt

I will also chime in with "same issue here' type of message. :)
[debug] System config: []
[debug] User config: []
[debug] Command-line args: ['-vvv', '--no-check-certificate', '--playlist-end', '299', '-u', 'PRIVATE', '-p', 'PRIVATE', '-o', '%(upload_date)s#%(playlist_index)s#%(title)s#%(uploader)s.%(ext)s', '--no-mtime', '--restrict-filenames', '-f', '137+bestaudio/298+bestaudio/22/136+bestaudio/135+bestaudio/134+bestaudio/133+bestaudio', '--write-sub', '--sub-lang', 'pl,en', '--merge-output-format', 'mkv', '--dateafter', '20160822', '--datebefore', '20160822', '--playlist-reverse', '-i', ':ytsubs']
[debug] Encodings: locale cp1250, fs mbcs, out cp852, pref cp1250
[debug] youtube-dl version 2016.08.22
[debug] Python version 3.4.4 - Windows-10-10.0.10586
[debug] exe versions: ffmpeg N-79260-g0c94906, rtmpdump 2.4
[debug] Proxy map: {}
[youtube:subscriptions] Downloading login page
[youtube:subscriptions] Logging in
WARNING: unable to log in: bad username or password
[youtube:subscriptions] Youtube Subscriptions: Downloading webpage
[download] Downloading playlist: Youtube Subscriptions
[youtube:subscriptions] playlist Youtube Subscriptions: Collected 0 video ids (downloading 0 of them)
[download] Finished downloading playlist: Youtube Subscriptions

dump.txt


Recursively add groups with LDAP authentication

Say a user is a member of LDAP group Operators, and Operators is a member of Developers.
Both the Operators and Developers groups are added as teams in Portainer.
Currently the user is only added to the Operators team, but not the Developers group.
This is helpful when you want a group to have access to everything another group has access to. As it is right now you have to add each user to both groups.

Hi! I am also interested in this feature. Are there any intentions by the developer team to make it possible for indirect members of an LDAP team to be counted as team members in Portainer? 😅

I'm facing the same issue - it block us to implement a proper RBAC access to Portainer


Wrong inlay hint when passing *arg or **kwarg

When passing arguments to a function as *ars or **kwargs, an inlay hint with the name of just the first parameter is shown. I think, it either must be no hint or all the parameter names that actually match, if it's possible.
Environment data

Language Server version: 2023.10.10

Code Snippet
def f(a, b):
    return a + b

args = [1, 2]
f(*args)

kwargs = {"a": 1, "b": 2}
f(**kwargs)

I believe @bschnurr you fixed this to pass no hint? (Validating release now)

This issue has been fixed in prerelease version 2023.10.51, which we've just released. You can find the changelog here: CHANGELOG.md


Update validation-basics.md

Remove apostrophes from possessive pronouns.

thanks @jbeacham
@EisenbergEffect CLA check

@jbeacham Thank you for contributing this pull request! We're truly excited and honored to have you as a member of our community. We look forward to working together with you to further build the Aurelia platform.
We noticed that you haven't yet signed our CLA. We would appreciate it if you would consider doing that for us. Before signing, please take a few minutes to read our contributing guidelines. These guidelines will help get you up to speed on how our community likes to operate and how the pull request process works. Once you have read that, assuming you are in agreement, simply sign our online CLA.
After you've signed, please ping this issue to let us know and we'll get your improvements merged in.
Thank you again!
The Aurelia Team

@jbeacham bump

Sorry, I signed the online CLA earlier today. Forgot to ping.
Oh, and thanks for the awesome documentation and examples! They are very helpful and are often times the tipping point when converting co-workers and friends over to Aurelia.

thanks!


Checking for the existence of a record causes an infinite loop when read permissions rely on oldData

The files are here: issue.zip
Using the following permissions:
record:
    "*":
        read: "oldData.xyz === 3"
        create: true
        write: true
        delete: true
        listen: true

event:
    "*":
        publish: true
        subscribe: true
        listen: true

rpc:
    "*":
        provide: true
        request: true

presence:
    "*":
        allow: true

Now if you look at the client.js file (line 13), the callback passed to record.has is never invoked. If you remove, in the permissions file read: "oldData.xyz === 3" with read: true, it'll work as expected.
const deepstream = require('deepstream.io-client-js');
const client = deepstream('localhost:6020');

client.on('error', (error) => {
  console.log('--- Deepstream error: ', error);
});

client.login({
  username: "userA",
  password: "password",
}, () => {
  console.log('before record.has');
  client.record.has('someRecordThatDoesNotExists', (error, exists) => {
    if (error) console.log(error);
    console.log('after record.has, returned value: ', exists);
  });
});

After making some researches in the source code, in permission/rule-applications.js, _getArguments() will always trigger a recordRequest. This causes _onLoadComplete to set this._runScheduled = true, thus making _runto loop indefinitely. I don't know if this may help or not.

I don't know if that matters, I'm using the latest 2.3.6 MacOS build.

Thank you for raising this! We will aim to get it into a release soon


Cookie extractor from Set-Cookie response.

I needed this for working with an API that uses Django auth/sessions to work with CSRF cookie and X-CSRFToken header.

Can one of the admins verify this patch?

@danielatdattrixdotcom Thank you, this is a feature that I know is in fairly high demand.  I've added a couple comments (small changes) before this is ready to be merged.  It also needs some sort of small unit test in test_validators.py.
This weekend I'm going to play with it a bit more and look more closely at the format of output (it might make sense to add a few options).
You get kudos by the way for looking closely at how the other extractors are implemented and following that (and adding documentation as well)!

@svanoort Let me know on the args/options to extractors in the YAML/JSON files and your thoughts on that and I'll make the updates and tests.

Oh no, I had write the same feature code, but @danielatdattrixdotcom 's documents is nice.
😂

@danielatdattrixdotcom Coming back to this now that the crunch period at work is settling down (spoiler: Jenkins, and specifically work on Pipeline Stage View plugin):  I think it's not worth adding options to the extractor right now - I had a notion of eventually allowing use of regexes or other matchers in extraction, but it's not useful yet.  We need support for templating with collections (or transformations on collections), and/or some embedded transformation language, if we're going to go down that route.  It's planned in the future, but too far off to worry about now.
Added one comment about a Python 3 compatibility challenge (just an import change).
So, from my point of view, to get to merge, what we need are automated tests, covering a couple key edge cases: cookie doesn't exist, cookie exists with multiple values, normal cookie case.  This means a unit test in test_validators.py, and a functional test in functionaltest.py -- the unit test just covers the extractor parsing and behavior (you can use some cookie info taken from a browser session).
The functional test needs to work with real HTTP requests; I'd prefer to add cookie-setting to the test Django-tastypie app (probably easiest done with the sessions middleware option), but if that proves hard we can just invoke a public API or site, as long as the request is trivial.  I've been using GitHub for this in the past.


go/types, types2: Identical does not fully match pairwise-identical signature type parameters

In the type checker, our notion of type identity for signatures considers type parameters in the signature declaration identical if they have identical constraints. In other words, it ignores the type parameter names:
Identical(func[P ~int](){}, func[Q ~int](){}) == true

But we don't extend this type parameter identity when comparing parameters or results:
Identical(func[P ~int](P){}, func[Q ~int](Q){}) == false

This is most likely an oversight since it doesn't matter in type-checked code, but we should be consistent.
CC @griesemer

Another incorrect case: we don't substitute correctly.
Identical(func[P1 any, P2 ~*P1](){}; func[Q1 any, Q2 ~*Q1]() {}) == false


Meta: v0.1.0

Goal
Tracking the steps to an initial release. The goal is a minimum installable package. Useful but non-required functionality is pushed out to v0.2.0.
Features

[x] #5
[x] #6
[x] #7
[ ] #8
[ ] #9
[ ] Posterior draws: cases
[ ] Posterior draws: r
[ ] Posterior draws: Rt
[ ] Predict method
[ ] Plot method
[ ] #12
[ ] Readme:v0.1.0 release updates

This looks good. Obviously point that you could use individual issues and a project workflow but this will work.

I'm tempted to say once currents PRs are in that should be 0.1.0 release and then move all the tooling around that here to 0.2.0 (and move that to 0.3.0) etc

Would it be possible to include a "getting_started" vignette or readme with 0.1.0? Other than that I'm supportive!

I want to hold the getting started vignette until we have some simulated data to fit to (#9). The readme should definitely make it though.
Thoughts on including the data + vignette or bumping?

I don't have strong opinions -- I think this depends on whether the goal of the release is to have something for you to pin to, or for others to use/follow

Yeah exactly. I would vote go as it's nice to have something to pin to and point to say this is the initial idea but if the plan is to send it to users we should wait.