net/http/httptrace: add ServerTrace hooks

Problem
A very common pattern in Go HTTP servers is to implement an http.ResponseWriter that wraps another http.ResponseWriter and captures the status code.  This is often used for logging and metrics collection.
  You can find examples in nearly every Go web framework out there.  One example is https://github.com/urfave/negroni/blob/master/response_writer.go#L13-L26.
There are some issues with this approach.  For instance, my statusCaptureWriter doesn't implement other interfaces like http.Flusher, http.CloseNotifier or http.Pusher.  I can't determine at compile time whether the underlying http.ResponseWriter implementation implements any of these interfaces, so if I choose to implement them I might lie to callers at higher levels of the stack and inadvertently break things.  (This particularly a problem with CloseNotifier.)
Proposal
I'd like to propose an additional interface, http.Statuser (better name welcome) that exposes the status code within a http.ResponseWriter implementation.  The internal http.(*response) implementation already tracks the status code written, so this can just be exposed and it will automatically implement this interface.
Software could avoid wrapping the http.ResponseWriter by instead type asserting it to http.Statuser and getting the status as needed there.  (And it could optionally continue to wrap the ResponseWriter as needed until this is widely deployed.)
type loggedHandler struct {
    handler http.Handler
    logger SomeLogger
}

func (h *loggedHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    // Could check for http.Statuser implementation here and wrap http.ResponseWriter
    // if necessary, but omitting for brevity

    h.handler.ServeHTTP(w, r)

    status := 0
    if s, ok := w.(http.Statuser); ok {
        status = s.Status()
    }

    h.logger.Logf("status=%d ...", status)
}

More examples:
https://github.com/gobuffalo/buffalo/blob/5b5e01469cfd064cad0a56b1eee4728a41d2ec46/response.go
https://github.com/rikonor/go-status-code-tracker
https://github.com/gin-gonic/gin/blob/2dae550eb5392006a4582ce9c90016a9b5a74e8b/response_writer.go#L19-L48
https://github.com/goji/glogrus2/blob/master/writer_proxy.go

These exist apparently, but why?
Code should know what it did previously.

@bradfitz The example I posted is a simplified instance: you create a wrapper that handles logging of all requests and then the individual handlers don't need to do or understand logging themselves.  They are free to just call w.WriteHeader(http.StatusOK) and things get logged at a higher level.  This plays nicely with middleware, too.
Other things that might get logged there are the URL/path, the duration of the call, etc.  It would be a pain to have to do that at essentially every exit site of every middleware or handler.  I think it's also a valid separation of concerns.

Another example beyond logging is collection of metrics.  You need to know when you serve 5xx errors, for instance.  Again, you don't necessarily want to be doing this everywhere it happens (and in the case of panics, I'm not sure offhand it's possible.)

Rather than a dozen optional methods, then, I'd prefer to see something like a State() method that returned a concrete struct type to which we could add fields over time.

Given that the primary target of this feature is logging, I suggest that the length of the response body would be a good addition to the output of State()

What if we just provide one canonical ResponseWriter wrapper instead?
And if this is about logging, we should (re-)consider the HTTP logging issues/proposals.

@bradfitz the thing that worries me about providing a wrapper type is also wrapping the other interfaces that may optionally be supported by the underlying type.  For instance, http.Pusher is something that's only supported by the http2 ResponseWriters.  This wrapper type either has to eschew support for Pusher or it has to "lie" about supporting it.

I've written a wrapper type like this as well, except instead of just logging the response code, my wrapper type also logged the number of response body bytes written, an md5sum of the response body, the time between the first and last Write call, and a few other miscellaneous things. Skimming through the examples in this comment, I see that I'm not alone -- many of the wrappers provide more than just the response code. I worry that adding a types like http.Statuser is an endless game of whack-a-mole. Even with Brad's State() idea, I wouldn't expect the http library to compute an md5sum of the response body.
Have you considered designing a server-side version of httptrace? We considered this at one time, but never worked through the details. Also see #18095 and #3344.

These exist apparently, but why?
Code should know what it did previously.

It is convenient to put all logging code in one place. One way to do this is by wrapping the http.ResponseWriter. This approach becomes more compelling when the logger tracks a bunch of things such as timing info or a hash of the response body, rather than just the response code (which is easy to remember by itself).

@tombergan i hadn't considered an httptrace-equivalent, but perhaps that is a better way to go in order to support different use cases like hashing response bodies.

I just uploaded a CL that has the very start of this by introducing an httputil.ServerTrace struct.  At the moment it only implements a WroteHeader hook.  It'd be nice if people could take a look and give an opinion on the general approach.
I expect to add StartHandler and EndHandler hooks as well.  Since it's implemented in the server, it could contain connection-level hooks as well.
One things that's a little strange about it is that the data isn't accessible from the handlers, which I imagine could be a problem if logging code wanted to include additional request-specific data.  One possibility might be to pass the http.Request into the hooks, have handlers add values to the req.Context and have an EndHandler hook extract and log there?

I think you're right that the server has both connection-level events and request-level events, and those may need to be handled separately. We could start by supporting request-level events only, since that seems to be what is wanted the most.
A nice thing about ClientTrace is that it's easy to create a new ClientTrace struct for each request. For example, here's how I use ClientTrace:
info := &reqInfo{}
req = req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{
  GotConn: info.GotConn,
  ...
}))
transport.RoundTrip(req)
...
log(info)

This lets me accumulate all info about a single request into a single struct (reqInfo). It would be ideal if I could a similar pattern for server traces. I think we could do this be changing your proposal slightly:
packge http
type Server struct {
  ...
  NewTrace func() *httptrace.ServerTrace   // optional callback to create a trace for a new request
}

One things that's a little strange about it is that the data isn't accessible from the handlers, which I imagine could be a problem if logging code wanted to include additional request-specific data. One possibility might be to pass the http.Request into the hooks, have handlers add values to the req.Context and have an EndHandler hook extract and log there?

That would work. Another idea is to add an opaquely-typed field to ServerTrace that can be set by NewTrace. Perhaps UserData interface{}. Then, the request handler could extract the trace info using httptrace.ContextServerTrace(req.Context()).

@joeshaw It is theoretically possible to dynamically decide, whether to wrap pusher or not:
func wrap(wrap, with http.ResponseWriter) http.ResponseWriter {
    if p, ok := wrap.(http.Pusher); ok {
        return struct {
            http.ResponseWriter
            http.Pusher
        }{ with, p }
    }
    return with
}

The trouble is a) the combinatoric explosion. You have to provide a branch for every combination of interface that could exist. And b) that the wrapping code must know about all possible optional interfaces. You can get around these by providing generic wrapping-code, but that also only solves the issue for all the optional interfaces in net/http itself, not potential third-party packages using the same trick.
This is why I'm so troubled by "optional interfaces" and the fact that they are used in net/http so pervasively. It's a very leaky abstraction and sets a bad example.

@Merovius wow! that is a very interesting hack, and one i hadn't ever considered.  I agree that it's not scalable, but thanks for pointing that out.

@tombergan,

Then, the request handler could extract the trace info using httptrace.ContextServerTrace(req.Context())

Putting things on the context might be interesting. I don't know whether that crosses the line of acceptable uses of context, though. I'm fine with it, but I think others might balk. I think it's better than more optional interfaces at least.

Context makes some sense for client tracing, in that the client causes it to trace all requests being made, but that's arguably OK. On the server, though, you want to scope it to one request, not all the requests being handled by the server. It's unclear that context can be made to work here.
It sounds like there's not a concrete proposal here at this point and that someone needs to write a design doc that incorporates the feedback and feature discussion above.
Or we can move this issue out of the proposal process and leave it open as a regular issue until someone does want to write a proposal.

I think it makes sense to move it out of the proposal process for now, since my original proposal wasn't well-liked.
Using httptrace sounds like a better way to go, but I think more prototyping needs to be done before it's ready to be a formal proposal.

On the client, we can trace a single request like this:
req = req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{...}))
transport.RoundTrip(req)

On the server, we already create a context for every request. We could add a server hook into that context creation point so that callers can inject a trace object. Something like:
packge http
type Server struct {
  ...
  // A better name is needed.
  UpdateRequestContext func(ctx context.Context) context.Context
}

// server.go:972
ctx, cancelCtx := context.WithCancel(ctx)
if c.server.UpdateRequestContext != nil {
  ctx = c.server.UpdateRequestContext(ctx)
}

package main
...
&http.Server{
  UpdateRequestContext: func(ctx context.Context) context.Context {
    return httptrace.WithServerTrace(req.Context(), &httptrace.ServerTrace{...}))
  },
}

That feels like a direct mirror of how httptrace is used on the client. In practice we may need to move the context creation point into readRequest so that httptrace can capture errors reading the request, but that's an implementation detail.
As a side-effect, this change would allow callers to arbitrarily modify the ctx used for each request. I haven't thought deeply about whether or not this is a bad idea. In practice, I don't imagine it would be useful beyond httptrace, although I'm sure it could be abused.
I also haven't thought much about which methods we'd like on httptrace.ServerTrace -- that's a good question for @joeshaw and others who are hoping to use this feature.

As a side-effect, this change would allow callers to arbitrarily modify the ctx used for each request.

There was actually a very similar proposal to add such a set-the-initial-context hook earlier, but it didn't go in because we weren't quite sure what the use case would be. But this seems like a good use case.

Maybe we should limit the ability to modify the context by doing that behind the scenes in the server code, and have the hook on the http.Server simply return an httptrace.ServerTrace value.
type Server struct {
    // ...
    ServerTrace() *httptrace.ServerTrace
}

// server.go
ctx, cancelCtx := context.WithCancel(ctx)
if c.server.ServerTrace != nil {
    ctx = httptrace.WithServerTrace(ctx, c.server.ServerTrace()) // do we even need to export WithServerTrace?
}

And inside your handler you could still extract the trace from req.Context with a helper function in httptrace if you needed it.

I suggested that earlier, but forgot that http cannot import httptrace, so that doesn't work.

The canonical wrapper mentioned by @bradfitz in https://github.com/golang/go/issues/18997#issuecomment-279522713 is actually impossible to do reliably. Not only do you need to consider a host of different combinations, but you also need to check whether the ResponseWriter that you want to wrap already implements those methods. I've had to wrap ResponseWriter's five times so far and the logic can vary depending on the different cases, although there is certainly some overlap. See: 1, 2, 3, 4, and 5. @joeshaw if you restrict yourself to the standard library, the number of combinations isn't too bad; it does, somewhat, scale.
The idea of a httptrace.ServerTrace sounds like a really good solution to this. As for what fields it should capture, the only information I want, so far, is the status code and the response size.

How should we best track response size?  I don't think we want to expose every write to a ServerTrace call, so maybe it should be gathered up inside the net/http machinery instead.

I don't think we want to expose every write to a ServerTrace call

Why not? What if someone wants to scan the response body is some way other than computing the size, such as computing a hash of the response body or tee'ing the response body to a log? Hooking each Write call seems like the most straightforward way to achieve this although I'd be open to other ideas. There should be nearly zero overhead when a Write hook is not installed (just a branch) or very low overhead when a hook is installed (branch + indirect function call).
How about something like:
type ServerTrace struct {
  // Received a bad request (e.g., see errToLarge in net/http/server.go).
  // The ServeHTTP handler will not be called.
  // BadRequestInfo has the status code of the response (the current implementation
  // can return 431 or 400) and perhaps also the response body, which is an error string.
  // This addresses https://github.com/golang/go/issues/18095
  GotBadRequest(BadRequestInfo)

  // Called when receiving a request, just before calling the ServeHTTP handler.
  // RequestInfo would likely include the URL and Headers of the request (with caveats
  // about not mutating those values).
  // This would satisfy https://github.com/golang/go/issues/3344 -- see the linked camlistore code.
  GotRequest(RequestInfo)

  // Called when the handler calls WriteHeader.
  // WriteHeaderInfo includes the status and maybe also the headers (with caveats about
  // not mutating the headers). Or perhaps this is (status, headers) instead of WroteHeaderInfo.
  // This addresses the current bug.
  WroteHeader(WroteHeaderInfo)

  // Called each time the handler calls Write. This is the data fed to the ResponseWriter,
  // e.g., before any transfer encoding. Includes the return values of the Write call.
  // Caveats about mutating data.
  // This addresses the current bug.
  WroteBodyChunk(data []byte, n int, err error)

  // TODO: WroteTrailers?

  // Called when the ServeHTTP handler exits.
  HandlerDone(panicked bool /* maybe? or part of HandlerDoneInfo? */)
}

I think we're converging on an actual proposal. https://github.com/golang/go/issues/18997#issuecomment-292039169 proposes how to create a ServeTrace object for each request received by the server, and https://github.com/golang/go/issues/18997#issuecomment-296722404 proposes a set of hooks to resolve this bug and two others.
@bradfitz, do you want a more formal proposal or are the above two comments sufficient? I think @joeshaw volunteered to help with the implementation.

SGTM at a high level. I might tweak certain names/docs/signatures, but not much. We can discuss that during the code review too.

👍
https://github.com/felixge/httpsnoop/blob/master/wrap_generated_gteq_1.8.go
https://github.com/prometheus/client_golang/blob/master/prometheus/promhttp/delegator.go
This needs to stop.

@joeshaw Are you still interested in helping with the implementation? We could start reviewing the implementation at any time, although we shouldn't submit anything until the Go 1.10 window opens (in August I believe).

http cannot import httptrace, so that doesn't work.

Why not?  Other than example_test.go, nothing in httptrace imports http so a circular dependency isn't an issue.
I could see us wanting to make the http.Request (perhaps a copy of it?) available in the GotRequest trace hook, though.

net/http already imports httptrace.

Here's a sketch of how you might use it in practice:
type requestInfo struct {
    start time.Time
    path string
    statusCode int
    h hash.Hash
}

s := &http.Server{
    UpdateRequestContext: traceRequest,
}

func traceRequest(ctx context.Context) context.Context {
    info := requestInfo{
        h: sha1.New()
    }

    trace := &httptrace.ServerTrace{
        GotRequest: func(gri httptrace.GotRequestInfo) {
            info.start = time.Now()
            info.path = gri.URL.Path
        },
        WroteHeader: func(whi httptrace.WroteHeaderInfo) {
            info.statusCode = whi.StatusCode
        },
        WroteBodyChunk: func(data []byte, n int, err error) {
            if err != nil {
                // ???
            }

            info.h.Write(data)
        },
        HandlerDone: func(hdi HandlerDoneInfo) {
            log.Printf(
                "path=%s status=%d duration=%s hash=%x",
                info.path,
                info.status,
                time.Since(info.start),
                info.h.Sum(nil),
            )
        },
    }

    return httptrace.WithServerTrace(ctx, trace)
}

I'm still not 100% convinced that tying this in with an UpdateRequestContext function is the right way to go, especially if we can't come up with other circumstances to change the request context.  A ServerTrace-specific function on the Server seems cleaner to me.

For my specific use-case(s), the trace would be added by a middleware. Actually, multiple tracing hooks by different middlewares.
Unfortunately this will leave outside GotBadRequest hook proposed by @tombergan.

After discussing use cases last night, I've flip-flopped on using context for that.  And that's because you might want to inject things like request IDs in the function that sets up the server trace and use them in middleware/handlers.  Building on my example from earlier:
type requestInfo struct {
	start time.Time
	path string
	statusCode int
	h hash.Hash
	size int64
}

s := &http.Server{
	UpdateRequestContext: traceRequest,
}

func traceRequest(ctx context.Context) context.Context {
	reqID := generateRequestID()
	ctx = withRequestID(ctx, id)

	info := requestInfo{
		h: sha1.New()
	}

	trace := &httptrace.ServerTrace{
		GotRequest: func(gri httptrace.GotRequestInfo) {
			info.start = time.Now()
			info.path = gri.URL.Path
		},
		WroteHeader: func(whi httptrace.WroteHeaderInfo) {
			info.statusCode = whi.StatusCode
		},
		WroteBodyChunk: func(data []byte, n int, err error) {
			if err != nil {
				// ???
			}

			info.h.Write(data)
			info.size += n
		},
		HandlerDone: func(hdi httptrace.HandlerDoneInfo) {
			log.Printf(
				"req-id=%s path=%s status=%d duration=%s size=%d hash=%x",
				reqID,
				info.path,
				info.status,
				time.Since(info.start),
				info.size,
				info.h.Sum(nil),
			)
		},
	}

	return httptrace.WithServerTrace(ctx, trace)
}

This way the request ID is not only logged, but it's passed into the context so handlers can extract it and log it, pass it to other services, etc.

For my specific use-case(s), the trace would be added by a middleware. Actually, multiple tracing hooks by different middlewares.

Could you elaborate a little more on this?  Are you thinking that the trace would be added in the execution of middleware?  Or that setting up the middleware would also attach something to the server?
The reason why I ask is that this hook function is on the http.Server and called only once per request -- it won't be possible to attach to it in the middle of middleware execution.  For things that the middleware might want to attach, using the provided http.Request WithContext() method is a better approach.

http cannot import httptrace, so that doesn't work.
Why not? Other than example_test.go, nothing in httptrace imports http so a circular dependency isn't an issue.

You're right. I have no idea what I was thinking. Looking forward to the CL!

Now that we're looking at a partial implementation, I'm not sure that getting all three of data []byte, n int, err error is the right move for WroteBodyChunk. I think that getting both len(byte) and n is confusing in cases where they differ, and it makes it harder to correctly write the two use cases we're considering:

Compute a hash of the response body (would need to slice byte[:n]).
Calculate the response length (would be a mistake to look at byte).

So maybe what WroteBodyChunk should receive is the slice of bytes that was successfully written.
That made me wonder whether it also needs to get the write error, especially since that error is already being delivered to the application. I feel like it's really weird to get the same error twice. Countering that, @joeshaw suggests you might want to (as one example) do something different with the response hash if the connection was closed by the client.

That made me wonder whether it also needs to get the write error, especially since that error is already being delivered to the application. I feel like it's really weird to get the same error twice. Countering that, @joeshaw suggests you might want to (as one example) do something different with the response hash if the connection was closed by the client.

Just to clarify, I think checking for an error is helpful in that you could, for example, not log a confusing partial hash value.  But I think the most value in getting the write error is to give one the ability to log it in the context of the rest of the request info.

Panicked is also really appealing in HandlerDone but given that the handler recover() is scoped to the whole connection it doesn't really make sense unless we add a recover() to each ServeHTTP call.. And the application doesn't need any trace support to do that already.

So maybe what WroteBodyChunk should receive is the slice of bytes that was successfully written.

This SGTM.

given that the handler recover() is scoped to the whole connection

This is not entirely true. In HTTP/2, the recover() wraps ServeHTTP:
https://github.com/golang/net/blob/master/http2/server.go#L2037
In HTTP/1, recover() does wrap the connection, but only because of the nature of HTTP/1 -- it's impossible to reuse a connection in HTTP/1 unless the prior request/response was completed exactly, but after a panic, we don't know the state of the connection and must assume it cannot be reused.
https://github.com/golang/go/blob/master/src/net/http/server.go#L1690
That said, the godoc for Handler actually says that a panic will hang up the connection, which is not what's implemented in HTTP/2. Perhaps a godoc update is in order?
https://golang.org/pkg/net/http/#Handler
/cc @bradfitz

Sent you CL 48551 for doc update.

I have a counterproposal that might resolve some of the root issues.
It sounds like the necessary information can be obtained by wrapping the http.ResponseWriter .. the problem is that wrapping http.ResponseWriter implementations is impractical because new features are exposed by adding methods to the underlying value, so a particular http.ResponseWriter might additionally implement http.Flusher or http.Pusher. A wrapper would need to implement exactly the right set of methods at compile time, since the success or failure of the type assertion is often the only way to know if a particular feature (like flushing) is supported. Furthermore, these wrappers need to be kept up to date with changes to Go to support new features (like http/2 push).
My proposal is to add one last method, which if present will return a struct. The struct would have a field for each optional feature, which could be left nil or set to a function value.
package http // import "net/http"

type ResponseOptionser interface {
  ResponseOptions() *ResponseOptions
}

type ResponseOptions struct {
  CloseNotify func() <-chan bool
  Flush       func()
  Hijack      func() (net.Conn, *bufio.ReadWriter, error)
  Push        func(target string, opts *PushOptions) error
  WriterTo    io.WriterTo
}

This would not allow adding new behaviors outside of net/http, since each feature would need a field on this struct. Is it common to add behaviors in non-core packages?
An alternative that allows for that is to attach the new behaviors as Values on the http.Request's Context, in the same way that *http.Server is currently available:
func myHandler(w http.ResponseWriter, r *http.Request) {
  f, ok := r.Context().Value(http.FlusherContextKey).(http.Flusher)
  if ok {
    f.Flush()
  }
}

Experience report:
I maintain a closed-source package for distributed request tracing that works for Go http clients and servers. I've implemented it as a http.Handler wrapper that in turn wraps the http.ResponseWriter.
It reads some of the inbound http headers to get the request id and places that in the http.Request's Context. When calling the underlying http.Handler, it passes a copy of the *http.Request that has a Context including some additional Values. As soon as its ServeHTTP method is called, it checks the current time and emits an instrumentation event including the timestamp, request id, and portions of the url and headers. When an http.Handler panics, it prints the stack trace with the request id prefixed to each line. On a call to WriteHeader (or the first call to Write), it emits an instrumentation event including the status code. It also counts the number of bytes sent in the response (measured by the arguments to calls to Write).
It used to do a bunch more, before Go 1.7 included context and before Go 1.8 fixed the lifespan of the Context to match the request (vs matching the lifespan of the call to ServeHTTP).

@rhysh This is an interesting proposal.  There are a few things I would tweak about it, like returning interface values for the individual items in the ResponseOptions struct.  (ie, Flush func() would be Flusher http.Flusher, but in both cases the values would be nil if the interface weren't implemented.)
There are a few things I like about it:

It makes it explicit what interfaces the ResponseWriter implementation supports.
ResponseWriter wrappers need only to implement the ResponseWriter and ResponseOptionser interfaces.

The things I am not sure how I feel about:

It does not address the very widespread use of ResponseWriter wrappers for common problems like storing status codes, calculating response sizes, or handler durations.  (More on this below.)
It effectively deprecates type asserting the ResponseWriter to other interfaces, except ResponseOptionser.  In other words, in a handler it's no longer sufficient to do:

if p, ok := rw.(http.Pusher); ok {
    p.Push(...)
}

which is a very standard, idiomatic thing to do.  (It would still succeed much of the time, since the underlying private ResponseWriter implementation would support it, but wrappers would not.)
Instead, you have to do:
if ro, ok := rw.(http.ResponseOptionser); ok {
    if ro.Pusher != nil {
        ro.Pusher.Push(...)
    }
}

which is a little longer and less natural.
Back to the proliferation of ResponseWriter wrappers.  Is this a real problem?
It's likely impossible to eliminate the need for ResponseWriter wrappers entirely in all cases.  But the vast majority of the duplication out there are to track status codes and, to a lesser extent, things like timings and response body sizes.  Perhaps the standard library should provide these in a new State method and interface as @bradfitz first suggested?
This counterproposal doesn't address the proliferation of wrappers, whereas the ServerTrace one likely would.
Unless people are vehemently opposed to this, I may take a stab at it.

Back to the ServerTrace implementation, the separation of http and httptrace packages is really making the usability of ServerTrace ugly because of circular dependencies.  GotRequest would really benefit from having access to http.Request, but instead most of the fields on it have to duplicated.  Same for http.Header.  It makes me worried about the long-term viability of doing it this way.
To see @pteichman's and my progress on ServerTrace thus far: https://github.com/pteichman/go/tree/servertrace

An alternative would be, to enable reflect to dynamically create methods (via #15924 and #16522). That way, people who want to create wrappers with optional interfaces could do so, e.g. by
func wrap(res http.ResponseWriter, statusCallback func(int)) http.ResponseWriter {
    st := reflect.StructOf{
        reflect.StructField{
            Type: reflect.TypeOf(res),
            Anonymous: true,
        },
    }
    nt := reflect.NamedOf(st, "wrapped", []reflect.Method{
        Name: "WriteHeader",
        Type: reflect.TypeOf(func(interface{}, int) {}),
        Func: reflect.ValueOf(func(_ interface{}, code int) { statusCallback(code) }),
    }
    v := nt.New()
    v.Field(0).Set(reflect.ValueOf(st))
    return v.Interface().(http.ResponseWriter)
}

Not terribly ergonomic (and I papered over the detail of how NamedOf would work and what the argument of that function would be, given that the struct-type is dynamically created); but it would work.

Stepping back a bit, the reason we originally added httptrace.ClientTrace was to expose info that could not be tracked by simply wrapping a RoundTripper. If we focus solely on better ways to wrap a ResponseWriter in this issue, then we lose sight of exposing info that is not available to ResponseWriter. For example, see #18095, which was dup'd into this issue. That issue is about logging bad requests, which we cannot do with ResponseWriter wrappers because bad requests are never forwarded to request handlers in the first place. Further, we might imagine adding HTTP/2 specific hooks in the future, or hooks to get the exact headers written on the wire, which can change slightly compared to what the caller passes to WriteHeader.
There is also #16220, which was not very well motivated in isolation, but would be fixed as a side-effect of adding UpdateRequestContext.
There is also #20956, which is yet another example of people trying to get at the underlying net.Conn (especially see this comment).

It used to do a bunch more, before Go 1.7 included context and before Go 1.8 fixed the lifespan of the Context to match the request (vs matching the lifespan of the call to ServeHTTP).

@rhysh, can you say more about this? It sounds like you're currently unable to do something that you'd like to do as a result of the Context change.

But the vast majority of the duplication out there are to track status codes and, to a lesser extent, things like timings and response body sizes. Perhaps the standard library should provide these in a new State method and interface as @bradfitz first suggested?

I am strongly opposed to this. There is no way the standard library will cover all kinds of stats that every user wants to track. Rather than getting ourselves into the business of tracking stats, we should provide APIs that allow users to track whatever stats they need.

can you say more about this? It sounds like you're currently unable to do something that you'd like to do as a result of the Context change.

On the contrary: Many of the features that had to be implemented from scratch by that internal package are now done by the standard library. For instance it used to use http.CloseNotifier to learn that the client had disconnected prematurely; as of Go 1.8, the request-scoped Context now provides that signal.
As of Go 1.8, Context in net/http has been working well. The focus of the internal package has narrowed, thanks to improvements in the standard library.

httptrace provides a lot of information about the request lifecycle and very little of it overlaps with information available to http.RoundTripper implementations.
The partial overlap I've encountered is that my internal package would additionally wrap the request body to learn when the full request had been written. This forced chunked transfers, which unfortunately didn't work with HTTP/1.0 servers, but otherwise worked well enough. After the addition of httptrace, I was able to use the WroteRequest hook to get that signal.
Because there's so little overlap, what are the problem statements that this issue should address? Here are some possibilities:

It's not possible [for *http.Server / http.Handler code] to learn about invalid/malformed requests
It's not possible to measure how long it took to read the request headers, from first byte to full headers received
It's not possible to measure how long a client's connection was idle before it sent the first byte of headers, or to determine if the connection was reused from a previous (or concurrent http/2) request.
Measuring the status code returned by a handler is inconvenient
Measuring the wall clock time at which the status code was set is inconvenient
Writing code to measure the status code returned by a handler is likely to break features like http/2 push or sendfile (for http/1.1).

The first three seem like they'd be addressed well with a server-side equivalent to the current httptrace.ClientTrace hooks. The later three can be done by wrapping http.ResponseWriter values as middleware, but it's hard currently too hard to do well.

That is a nice summary. I think all six of your listed use cases could be handled by httptrace.ServerTrace. For 4 & 5, we'd pass through the status code (and possibly headers) from the ResponseWriter to the ServerTrace. 6 should "just work" since ServerTrace doesn't wrap the `ResponseWriter.

I think all six of your listed use cases could be handled by httptrace.ServerTrace
It could do any of them. Where should it focus? Where should the discussion focus?

httptrace.ClientTrace doesn't allow getting the status code; that's trivial to do in a http.RoundTripper. But it also doesn't allow getting the sizes of the request body or the response body—both possible, but inconvenient, to get from a http.RoundTripper wrapper.
ClientTrace focused on giving access to net/http internals that it alone could provide.
In the current implementation draft of ServerTrace, the GotBadRequest hook gives access to information that would not otherwise be available to http.Handler authors. This looks like a good way to give access to that information.
The WroteHeader and WroteBodyChunk callbacks give access to data that http.Handler authors already have. It's not very convenient access because of the pattern of adding secret bonus methods on http.ResponseWriters, but it's access.
Should ServerTrace have a callback for when Flush is called and for when http/2 push is used? If sendfile(2) is activated, should a hook be called at the beginning/middle/end of that transfer?
I think it would be helpful to split the discussion into 1) information present in the internals of net/http that is not exposed to users, and 2) information that users have, but need better access to.

Use of the http.Server.UpdateRequestContext field looks clunky. It's called before the http.Request is available, meaning that users would need to coordinate between the code that runs during that hook and a top-level http.Handler in order to match the ServerTrace hooks to an http.Request.
Since it's attached to the http.Server, it can't be used by middlewares. It's set once at the top level by the owner of the service, making it inaccessible to the maintainers of middleware packages. They're likely to continue wrapping http.ResponseWriters, since that would be the only way the code they control is able to access the data they need.
I have a proposal for an alternate design, if we split the functionality more or less into the two types of data I identified above:

A new field on http.Server points to a struct that contains the GotBadRequest hook (and maybe some others). Consider this logically as a structured extension of the http.Server.ErrorLog field.

A function in net/http that takes the Context value from a http.Request (or a *http.Request directly) and something like a httptrace.ServerTrace struct, registering it for callbacks as the request is served. Any layer of http.Handler middleware could call the function, just as any layer of http.RoundTripper middleware can with ClientTrace.

(The function wouldn't return a Context like the ClientTrace equivalent does; the http.Server implementation would use a private context key to store and access a function of type func(httptrace.ServerTrace). The net/http package's function would extract and call it.)

[UpdateRequestContext is] called before the http.Request is available, meaning that users would need to coordinate between the code that runs during that hook and a top-level http.Handler in order to match the ServerTrace hooks to an http.Request.

Not sure I followed? The hooks will be available via http.Request.Context(). A pattern like the following should work:
s := &http.Server{
  UpdateRequestContext: func(ctx context.Context) context.Context {
    sh := newServerHooks()  // some user-defined structs
    ctx = context.WithValue(ctx, key, sh)
    return httptrace.WithServerTrace(ctx, &httptrace.ServerTrace{
      GotRequest: sh.GotRequest,
      ...
    })
}

The handler can then use ctx.Value(key) to extract sh.

Since it's attached to the http.Server, it can't be used by middlewares. It's set once at the top level by the owner of the service, making it inaccessible to the maintainers of middleware packages.

The middleware needs some entry point in order to apply its http.ResponseWriter wrapper. Couldn't the middleware use a similar entry point to install its context wrapper? I am admittedly not familiar with many such libraries (aside from the few I've written myself for internal use), so I am very interested in hearing about libraries where UpdateRequestContext will be difficult to use.

A function in net/http that takes the Context value from a http.Request ...

Can you explain this with a pseudocode example? I did not follow.

A new field on http.Server points to a struct that contains the GotBadRequest hook ...
I think it would be helpful to split the discussion into 1) information present in the internals of net/http that is not exposed to users, and 2) information that users have, but need better access to.

That is a useful distinction for this discussion, however, my main concern is API clutter. I would like to minimize the API surface and avoid cluttering http.Server with callback fields, if possible. I fear we are heading towards API clutter unless the new functions/callbacks/fields we add are sufficiently general.
I am promoting two APIs: (1) Wrapping http.ResponseWriter. This will be the canonical way to affect how the response is produced. (2) UpdateRequestContext. This will be the canonical way to get a read-only view of each request for logging. There may need to be a long list of read-only hooks, but at least those hooks will be hidden in net/http/httptrace rather than cluttered in net/http.

Considering the case where a user wants to log the status code of all responses, here are three possible approaches:

No changes to net/http. Logging package authors wrap the http.ResponseWriter, providing a function with signature func(inner http.Handler) http.Handler. Application owners can call it any time they have a http.Handler, sometime before setting their http.Server.Handler field.

Add a field to http.Server, UpdateRequestContext. Logging package authors write two pieces of code: their usual middleware (to access the URI path from the http.Request), and a function to add to the UpdateRequestContext field (to access the status code). Application owners call one of the functions from their http.Handler, and the other where they set up the http.Server (which may be in a different file or package).

Add a function in http designed to be called from an http.Handler. Logging package authors convert their http.ResponseWriter wrappers to use the new function with no change in the API they provide to users. Applications owners can call it any time they have a http.Handler, sometime before setting their http.Server.Handler field.

The shortcoming of (1) is that it blocks access to additional methods on the http.ResponseWriter. The shortcoming of (2) is that it requires users to coordinate between two different parts of the net/http API. Users who only have access to an http.Handler cannot use it (and would be forced to use the existing option 1).
Code for each follows. Does example 2 match the API design you have in mind? Can you give a more concise demonstration of correct use of the new field?

No changes to net/http:

type statusRecorder struct {
	status int
	w      http.ResponseWriter
}

func (srw *statusRecorder) Header() Header {
	return srw.w.Header()
}

func (srw *statusRecorder) Write(p []byte) (int, error) {
	if srw.status == 0 {
		srw.status = http.StatusOK
	}
	return srw.w.Write(p)
}

func (srw *statusRecorder) WriteHeader(status int) {
	srw.status = status
	srw.w.WriteHeader(status)
}

func logStatusMiddleware_ResponseWriter(inner http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		srw := &statusRecorder{w: w}

		defer func() {
			log.Printf("path=%q status=%d", r.URL.Path, srw.status)
		}()

		inner.ServeHTTP(srw, r)
	})
}

Add a field to http.Server, UpdateRequestContext:

func logStatusMiddleware_AndServerUpdate(inner http.Handler, srv *http.Server) http.Handler {
	ctxKey := new(int)

	prevFn := srv.UpdateRequestContext
	newFn := func(ctx context.Context) context.Context {
		if prevFn != nil {
			ctx = prevFn(ctx)
		}
		var (
			mu   sync.Mutex
			info httptrace.WroteHeaderInfo
		)
		ctx = httptrace.WithServerTrace(ctx, &httptrace.ServerTrace{
			WroteHeader: func(i httptrace.WroteHeaderInfo) {
				mu.Lock()
				defer mu.Unlock()
				info = i
			},
		})
		ctx = context.WithValue(ctx, ctxKey, func() httptrace.WroteHeaderInfo {
			mu.Lock()
			defer mu.Unlock()
			return info
		})
	}
	srv.UpdateRequestContext = newFn

	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		defer func() {
			info := r.Context().Value(ctxKey).(func() httptrace.WroteHeaderInfo)()
			log.Printf("path=%q status=%d", r.URL.Path, info.StatusCode)
		}()

		inner.ServeHTTP(w, r)
	})
}

Add a function in http designed to be called from an http.Handler:

func logStatusMiddleware(inner http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		http.AddServerTrace(r.Context(), &httptrace.ServerTrace{
			WroteHeader: func(info httptrace.WroteHeaderInfo) {
				log.Printf("path=%q status=%d", r.URL.Path, info.StatusCode)
			},
		})

		inner.ServeHTTP(w, r)
	})
}

If I'm reading this correctly, ServerTrace proposal doesn't allow modification of the response body prior to writing. Therefore it will not allow things like compressing the response body prior to it being sent, for example.
Presumably, the ServerTrace struct will need to define another handler: WillWriteBody(body []byte, header http.Header) []byte
Such a handler would need to receive the body before it's being written, as well as the headers, if there's a need to modify them. And it should return the new body for actual writing. I assume that the request headers are already somehow available from within that handler already, since they also usually play a part here.

You make a good point @urandom , that packages like https://github.com/NYTimes/gziphandler are currently implemented by wrapping the ResponseWriter with special behavior for Write and Flush. If packages like that one continue to wrap the ResponseWriter, new features exposed as methods will continue to be shadowed.
That package also has special behavior for the Push method: it ensures that the synthetic request will have the "Accept-Encoding: gzip" header. And, the handler disables connection hijacking. It needs to intercept several methods in order to change their behavior.
Will the ServerTrace struct expand to allow modifying all additional ResponseWriter methods? When new behaviors are added to the ResponseWriter, will they be added to the ServerTrace struct as well? That seems like a lot of API duplication.

Looks like this didn't happen before the Go 1.10 freeze. Bumping to Go 1.11.

Another data point of a package which has to deal with the combinatorial explosion of optional interfaces:
https://github.com/prometheus/client_golang/blob/1cdba8fdde81f444626530cce66e138d2f68ae1c/prometheus/promhttp/delegator.go#L100


Advanced Physics & QoL

Accidentally created PR for fork original instead of own branch. Closed as includes too much changes compared to the original branch and excludes support for GDScript.


Limitations of the RSUSB backend

In the documentation on the RSUSB backend, it is mentioned that it has some limitations. For example, this page states:

As a general rule it is recommended to use the native kernel drivers, especially in production environment. The second method [RSUSB] augments the native installation and allows for a fully-functional SDK deployment at the expense of certain performance and functional limitations (e.g. multi-cam).

Similarly, the documentation on the old FORCE_LIBUVC flag here, which now activates the RSUSB backend, mentions that "it has well known limitations".
Despite supposedly being well known, unfortunately I could not find a list of these limitations. Maybe I am overlooking it? Or if there is no such list, would it be possible to set one up?
The reason I am asking is that I have a case where something works with the V4L2 backend, but not with the RSUSB backend, and I don't know whether that is a bug in the RSUSB backend or a known limitation. Specifically, with RSUSB I get an error "failed to set power state" when trying to change camera settings such as exposure time while the camera is currently streaming, while it works with V4L2. However, the V4L2 backend has the disadvantage that it sometimes drops frames, while the RSUSB backend seems to deliver them more reliably. Thus I would prefer using RSUSB.

Hi @puzzlepaint   For a comparison of RSUSB backend (previously also known as libuvc backend) with a patch-based librealsense build, please visit the link below and scroll down through the linked-to comment to the section headed What are the advantages and disadvantages of using libuvc vs patched kernel modules?
https://github.com/IntelRealSense/librealsense/issues/5212#issuecomment-552184604

Thank you for the quick and helpful reply! This resolves the question for me, but given that the information seems to be buried somewhere in a GitHub issue conversation, it seems to be hard to find in general. Maybe this could be described or linked for example in the list of config options or in the libuvc installation instructions? Apart from that, this issue can be closed, thanks again.

You are very welcome.  I have added a Documentation tag to this case so that your suggestion for official documentation on the subject of RSUSB vs patched kernels can be considered by the RealSense team at a future date.  This case should be kept open in the meantime, though you do not need to do anything further yourself.  Thanks!

I would just like to add that I found the solution to my specific issue I mentioned above:

Specifically, with RSUSB I get an error "failed to set power state" when trying to change camera settings such as exposure time while the camera is currently streaming, while it works with V4L2.

It was not a limitation of the RSUSB backend, but an issue with my code. The code queried the librealsense device, started streaming, and then continued to use the device object queried at the start, using code like this:
rs2::context ctx;

rs2::device_list devices = ctx.query_devices();
rs2::device device = devices.front();

std::unique_ptr<rs2::color_sensor> colorSensor;
colorSensor.reset(new rs2::color_sensor(device.first<rs2::color_sensor>()));

rs2::config cfg;
// Some code to set up cfg here ...

rs2::pipeline pipeline;
pipeline.start(cfg);

// At this point, trying to do something like:
colorSensor->set_option(RS2_OPTION_EXPOSURE, 10 * colorExposureTimeMS);
// ... works with the V4L2 backend, but causes "failed to set power state" with the RSUSB backend.

The issue with this is that apparently, starting the pipeline internally creates new uvc_sensor objects with the RSUSB backend, and trying to use the old uvc_sensor objects afterwards conflicts with the new ones. The correct approach is to re-query the objects from the rs2::pipeline_profile after starting streaming, by modifying the code to start the pipeline like this:
rs2::pipeline pipeline;
rs2::pipeline_profile pipeProfile = pipeline.start(cfg);

// Re-query the device and sensor objects after starting the pipeline
device = pipeProfile.get_device();
colorSensor.reset(new rs2::color_sensor(device.first<rs2::color_sensor>()));

With this, everything seems to work as expected.

Excellent news that you found a confirmed solution to your project's problem @puzzlepaint    :)

@puzzlepaint hello, having multiple instances to the same (shared) resource is the fundamental limitation with RSUSB backend. so indeed after opening the pipeline , the original handles are being invalidated.
Additionally, while the originally-posted code is working with V4L driver's it is still not correct to use it that way as it may introduce side-effect on GW-Host sync level, and thus the approach you've implement (to re-acquire the sensor's handle after opening pipe) is the proper one not only for RSUSB but also for native (V4L/MediaFoundation) backends.

Thanks for confirming that this is the right approach. It is however extremely easy for users of librealsense to do this incorrectly, since it appears to be working just fine with the V4L2 backend, and the need to re-acquire the handles is thus very easily overlooked.
From my point of view, it would thus be strongly advisable to:

Document this very clearly, with a clear warning, and/or
Print verbose, descriptive warnings at runtime if multiple handles to the same resource are attempted to be used (even with the V4L2 backend), and/or
Change the library design such that opening a pipeline does not invalidate the original handles.

@MartyG-RealSense Thanks for your explanation. My understanding is that RSUSB backend on Jetson devices does not provide extended frame metadata and hardware timestamp (all jetpack versions are based on kernel v4.9).
Could you elaborate a bit more on "performance"? As you said in #5212

The kernels are fast and (mostly) stable. Adding patched modules into kernel tree in controlled and pier-reviewed manner allows to get most of the benefits and also get the additional features.

and https://github.com/IntelRealSense/librealsense/blob/master/doc/installation_jetson.md#3-choose-librealsense-sdk-backend

... at the expense of certain performance and functional limitations

How is native backend faster than RSUSB backend (less memory copies / lower latency / less cpu usage)? How did you do the benchmark / test?
Because deploying patched L4T for jetson devices is painful and harder to maintain, I want to know if it worth the effort.

Hi @hg-zt  The advice about libuvc vs patched kernels in https://github.com/IntelRealSense/librealsense/issues/5212 was written by RealSense team member @ev-mp (who also provides advice in this case), who has specialist knowledge of that subject that I unfortunately do not have.
An RSUSB build of librealsense includes support for hardware metadata.  It is not dependent on Linux versions or kernel versions and does not require patching.


Helm 3.3.4 Issue

Note
Issues in this repo are for tracking bugs, feature requests and questions for the tasks in this repo
For a list:
https://github.com/Microsoft/azure-pipelines-tasks/tree/master/Tasks
If you have an issue or request for the Azure Pipelines service, use developer community instead:
https://developercommunity.visualstudio.com/spaces/21/index.html )
Required Information
Entering this information will route you directly to the right team and expedite traction.
Question, Bug, or Feature?
Type: Bug
Enter Task Name: HelmDeploy@0
list here (V# not needed):
https://github.com/Microsoft/azure-pipelines-tasks/tree/master/Tasks
Environment

Server - Azure Pipelines or TFS on-premises?

If using TFS on-premises, provide the version:

If using Azure Pipelines, provide the account name, team project name, build definition name/build number:

Agent - Hosted or Private:

If using Hosted agent, provide agent queue name:

If using private agent, provide the OS of the machine running the agent and the agent version:

Issue Description
[Include task name(s), screenshots and any other relevant details]
Helm 3.3.4 was released this morning and warnings are now being written to the error stream causing the task to fail. I logged an issue with helm here https://github.com/helm/helm/issues/8792, but they recommended that I log an issue with you.
bacongobbler commented 9 minutes ago •
edited
Please file an issue upstream with Azure DevOps. There's nothing we can do here. Helm is correctly reporting a security issue that the user should address. It is Azure DevOps that is handling the log output incorrectly.
An alternative option would be to fix the permissions of your kubeconfig file within the devops task. I assume this may also be set up by azure devops, which I would again urge you to file an issue there.
Task logs
2020-09-23T04:37:17.5225159Z ##[section]Starting: helm list
2020-09-23T04:37:17.5234692Z ==============================================================================
2020-09-23T04:37:17.5235057Z Task         : Package and deploy Helm charts
2020-09-23T04:37:17.5235503Z Description  : Deploy, configure, update a Kubernetes cluster in Azure Container Service by running helm commands
2020-09-23T04:37:17.5235832Z Version      : 0.175.4
2020-09-23T04:37:17.5236061Z Author       : Microsoft Corporation
2020-09-23T04:37:17.5236351Z Help         : https://aka.ms/azpipes-helm-tsg
2020-09-23T04:37:17.5236659Z ==============================================================================
2020-09-23T04:37:17.7716777Z [command]/workspace/_tool/helm/3.3.4/x64/linux-amd64/helm ls --namespace cib-k8-magic-dev --output json
2020-09-23T04:37:17.9788806Z WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /workspace/_temp/helmTask/1600835837733/config
2020-09-23T04:37:17.9789681Z WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /workspace/_temp/helmTask/1600835837733/config
2020-09-23T04:37:17.9792110Z [{"name":"dadjokes-gradle-sbt2-jre11-api","namespace":"cib-k8-magic-dev","revision":"1","updated":"2020-07-14 14:30:57.805873782 +0000 UTC","status":"failed","chart":"dadjokes-gradle-sbt2-jre11-api-0.0.2-63254193faa65349a32fb7e791f472107a5d19e0","app_version":"1.16.0"},{"name":"fluentd","namespace":"cib-k8-magic-dev","revision":"6","updated":"2020-09-21 08:54:04.668049177 +0000 UTC","status":"deployed","chart":"fluentd-1.9.1","app_version":"v2.4.0"},{"name":"hello-world-api","namespace":"cib-k8-magic-dev","revision":"419","updated":"2020-09-23 04:32:11.059193346 +0000 UTC","status":"deployed","chart":"hello-world-api-0.0.538","app_version":"1.16.0"}]
2020-09-23T04:37:17.9827315Z ##[error]WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /workspace/_temp/helmTask/1600835837733/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /workspace/_temp/helmTask/1600835837733/config
2020-09-23T04:37:17.9839718Z ##[section]Finishing: helm list
Troubleshooting
Checkout how to troubleshoot failures and collect debug logs: https://docs.microsoft.com/en-us/vsts/build-release/actions/troubleshooting
Error logs
2020-09-23T04:37:17.9827315Z ##[error]WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /workspace/_temp/helmTask/1600835837733/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /workspace/_temp/helmTask/1600835837733/config

@pieterv-icloud-com Thanks for reporting the issue. This security warning is being shown from Helm 3.3.2 onwards. I've already raised a PR to fix the permissions on Kubeconfig file. As a work around, you can continue to use the older helm versions while we work on deploying these changes.

While that solves the immediate problem, why not just set the default value of failOnStderr to false? There should not be a case where a pipeline should fail due to a warning or a diagnostic message displayed on standard error. If there's an error, Helm will return a non-zero exit code.
https://github.com/microsoft/azure-pipelines-tasks/blob/d2bc3b4b163dcf7826a8f72cf493f1b2968ca477/Tasks/HelmDeployV0/task.json#L433

Will definitely give it a try

On 23 Sep 2020, at 16:53, Matthew Fisher<EMAIL_ADDRESS>wrote:
While that solves the immediate problem, why not just set the default value of failOnStderr to false? There should not be a case where a pipeline should fail due to a warning or a diagnostic message displayed on standard error. If there's an error, Helm will return a non-zero exit code.
https://github.com/microsoft/azure-pipelines-tasks/blob/d2bc3b4b163dcf7826a8f72cf493f1b2968ca477/Tasks/HelmDeployV0/task.json#L433 https://github.com/microsoft/azure-pipelines-tasks/blob/d2bc3b4b163dcf7826a8f72cf493f1b2968ca477/Tasks/HelmDeployV0/task.json#L433
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/microsoft/azure-pipelines-tasks/issues/13594#issuecomment-697485926, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYPKPBSZFY76RXBQM4SEPLSHIDVVANCNFSM4RWTBXDA.

@bacongobbler The actual problem here was that the warning from Helm command was being written on stdout. This affected the output parsing logic for Helm version check in the task. Now that the warning is being written on stderr after this commit, and we are also updating the permissions on Kubeconfig file, this issue should be resolved. (Although for immediate fix, the users will have to set failOnStderr to false)
Coming to your suggestion that the default value of failOnStderr should be made false, it looks like a fair point. I'll raise this feedback to the PM's.

For anyone looking to follow what's a default and where, we think https://github.com/microsoft/azure-pipelines-tasks/blob/master/Tasks/HelmDeployV0/task.json#L433 being set to true is not a good default, and handling errors like the Bash job handles them (https://github.com/microsoft/azure-pipelines-tasks/blob/master/Tasks/BashV3/task.json#L91) would be a more reasonable default.

FYI: https://github.com/microsoft/azure-pipelines-tasks/pull/13638


roku-deploy does not respect bsconfig.json

My bsconfig.json looks like this
{
    "extends": "config/bsconfig.base.json",
   ...
}

But roku deploy does not seem to read from config/bsconfig.base.json
Making this fails (because it's looking in the wrong staging folder)
rokuDeploy.zipPackage({
    outDir: '.',
    outFile: zip_name
}).then(function(){
    console.log(`${zip_name} created`)
}, function(error) {
    console.error(error);
});

Currently roku-deploy doesn't support the extends field in bsconfig.json.
https://github.com/rokucommunity/roku-deploy/blob/0858e5bdb7032399ee0c9f31ede70867cb32ce83/src/RokuDeploy.ts#L811
The best way to fix this would be to centralize the bsconfig.json loading logic so we can share it between roku-deploy and brighterscript, but another quicker option would be to just copy the bsconfig.json loading logic from brighterscript into roku-deploy.

Would it be simpler to just call loadConfigFile
https://github.com/rokucommunity/brighterscript/blob/06a53ed9381d0882765c34ef4c53435928e9d394/src/util.ts#L167

That would end up with a weird circular dependency, because brighterscript depends on roku-deploy, and roku-deploy would then depend on brighterscript. npm might resolve them fine, but I think it would be better to keep the dependency one-way (brighterscript->roku-deploy).


repository name should be changed

In golang, packages are given lower case, single-word names.
(ref: https://golang.org/doc/effective_go.html#names)
So, Cleisthenes should be change to cleisthenes.

fixed! thx


Add examples from README to examples/

That way, they are easy to compile for interested users and
run automatically as part of the test-suite.

Looks like these lines need to be bumped: https://github.com/sfackler/rust-native-tls/blob/master/.travis.yml#L6 https://github.com/sfackler/rust-native-tls/blob/master/.travis.yml#L10
https://travis-ci.org/sfackler/rust-native-tls/jobs/204093322

@sfackler Thanks for the hints!
On an unrelated note: do you think it's possible to allow untrusted hosts, like those which are self-signed for the purpose of local testing, using this library?

It is not yet - the functionality needs to be added to schannel-rs first. It's definitely something I do want to support though!

Perfect! Is  there a tracking issue I could subscribe to? I am trying to connect to a cassandra instance via TLS using tokio-cassandra, and am investigating libraries that support this. rustls would be been my favorite, but unfortunately its ciphers are too new :).

#3

@sfackler - could you restart the travis jobs? It seems they failed for different reasons.

Restarted Travis - The OpenSSL version will need to be updated in the appveyor config.

Thank you - the issue seems to be reproducible though.
@sfackler I am afraid I don't know what is going on here. The only change that might have caused this breakage on travis is to remove the v1.9 restriction for Rust. Appveyor needs a different version, as you say, but I don't know why this branch needs it while master apparently still works.
Looking into this it seems the last build ran successfully two months ago!
If you restarted that one, I would hope that similar issues crop up. Those could then be fixed in master, and I just rebase this PR onto it - et voilà.
What do you think :)?

You should be able to rebase now

@sfackler Sorry for the great delay! The rebase was performed, and this branch looks like it can be merged now.

Thanks!


"SWS/S&M: Map selected tracks MIDI input to channel x" doesn´t work during recording

Thanks for the report, I can reproduce it.
@Devs:
This line is preventing mapping channels during recording:
https://github.com/reaper-oss/sws/blob/400c3c13949805afc37b87627a0c8664f436508f/SnM/SnM_ChunkParserPatcher.h#L260
So it has to be with all actions which involve chunk editing during recording.
Alas I have no clue why Jeffos added this check, but I suppose he had a good reason (for security reasons to not possibly crash the project during recording?).
Does anybody know more?

The GetPlayState test was added in 9f2726a056137a679ee91a62e74f2780b9b33747:

Global "Safety" update (temporary, I hope): many S&M actions will have no effect while recording.
See http://forum.cockos.com/showthread.php?t=52002 for tech. details

Prior to c1a4f8f987cfd743ad4d31546d6269671c9659d9 there was a comment saying:

// Commit: only if needed, i.e. if m_object has been set and if the chunk has been updated
// note: global protections here (temporary, I hope). In REAPER v3.66, it's safer NOT to
// patch while recording and to remove all ids before patching, see http://forum.cockos.com/showthread.php?t=52002

Nice, thanks for the additional info @cfillion !
The last Post in this 2010 thread makes me tend to remove the check but I'm sure Jeffos still had very good reasons to leave it in up until last year.
I would leave it in too, but maybe we could rename relevant actions adding "... (no-op during recording)". Not sure how many that would be though..

Asked Justin about it, let's see what he says...
https://www.askjf.com/index.php?q=3889s

Thank you.
I realized the Call Hierarchy of ParsePatch(), which I guess is the main method for chunk patching, is huge. Jeffos (and others) made good use of his swiss army knife tool.
These are a couple of things I found after a quick search that are effected by this check:

Midi CC lane actions and Reascript functions
Send/Receive/Cue Buss actions and Reascript functions
many Item & Track FX actions
Reassign MIDI learned channels of ...
Show/Hide take vol/pan/mute/pitch envelope
actions involving Track Templates
functions used by SNM Resources and Live Configs
...

Thanks for checking veto.
Seeing it affects so much I'd say safety first and leave the check in (so the issue would be a "Live with it.")
But that's just me of course...

I'm pretty much a noob but I still tend to leave it in too (at least for the older actions/functions).
Maybe a long or separate beta would make sense too.
Concerning the OP, if he doesn't like the JSFX approach, I'm ready to do it by Reascript for him.

Indeed, I´d be very interested if you could do a Reascript for that? That´d be fantastic.
Jurek (OP)

Am 26.10.2017 um 10:54 schrieb veto-gh<EMAIL_ADDRESS>I'm pretty much a noob but I still tend to leave it in too (at least for the older actions/functions).
Maybe a long or separate beta would make sense too.
Concerning the OP, if he doesn't like the JSFX approach, I'm ready to do it by Reascript for him.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/reaper-oss/sws/issues/913#issuecomment-339598154, or mute the thread https://github.com/notifications/unsubscribe-auth/AfhQY2Karsd19Dsm3tq59CtaJcQ9sjLXks5swEi8gaJpZM4QD6Bo.

Hi Jurek,
for different channels duplicate the script, change the file name and edit the source (look for the "user settings" part).
If you set CHANNEL to "nil" (without quotes) you can map MIDI to the source channel.
Map selected tracks MIDI input to channel 1.lua.zip

Hi Jurek,
for different channels duplicate the script, change the file name and edit the source (look for the "user settings" part).
If you set CHANNEL to 0 you can map MIDI to the source channel.
File: Map selected tracks MIDI input to channel 1.lua.zip

Hey, I just had a chance to test it, works like a charm! Thank you so much!
Jurek

Am 02.11.2017 um 21:30 schrieb veto-gh<EMAIL_ADDRESS>Hi Jurek,
for different channels duplicate the script, change the file name and edit the source (look for the "user settings" part).
If you set CHANNEL to 0 you can map MIDI to the source channel.
File: Map selected tracks MIDI input to channel 1.lua.zip https://github.com/reaper-oss/sws/files/1439283/Map.selected.tracks.MIDI.input.to.channel.1.lua.zip
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/reaper-oss/sws/issues/913#issuecomment-341548789, or mute the thread https://github.com/notifications/unsubscribe-auth/AfhQY86A0nNmm5FfgxkZZVY38KreaioNks5syiZ5gaJpZM4QD6Bo.

Since OP's original issue sounds solved I took the liberty to rename the title to reflect the broader topic going on here.


AttributeError: 'NoneType' object has no attribute '_add_suffix'

I'm working on a machine-learning project where I'm writing my custom code on top of PySCF to generate some features:
The following code works perfect for most of the time, I'd say 90% of times.
        MO = ao2mo.general(
            self.mf._eri, (self.mf.mo_coeff[:, self.ccsd.frozen :],) * 4, compact=False
        ).reshape(self.nmo, self.nmo, self.nmo, self.nmo)

But suddenly it starts to give me this error:
  File "/lustre/isaac/scratch/sakram/Research/DDQC/ddcc/ccsd_manager.py", line 339, in __init__
    MO = ao2mo.general(
         ^^^^^^^^^^^^^^
  File "/lustre/isaac/scratch/sakram/.conda/envs/main/lib/python3.12/site-packages/pyscf/ao2mo/__init__.py", line 309, in general
    return mod.general_iofree(eri_or_mol, mo_coeffs, intor, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/lustre/isaac/scratch/sakram/.conda/envs/main/lib/python3.12/site-packages/pyscf/ao2mo/outcore.py", line 675, in general_iofree
    general(mol, mo_coeffs, feri, dataname='eri_mo',
  File "/lustre/isaac/scratch/sakram/.conda/envs/main/lib/python3.12/site-packages/pyscf/ao2mo/outcore.py", line 217, in general
    intor, comp = gto.moleintor._get_intor_and_comp(mol._add_suffix(intor), comp)
                                                    ^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute '_add_suffix'

I know it doesn't make any sense because it works most of the time.
Additional Info:
I'm trying to run KNNs with faiss and during predictions, I'm getting the above error but the same code worked perfectly fine during the data generation.
One more thing this code is completely fine with smaller molecules but crashes for larger ones.
Another weird stuff: If I run the same molecule outside of faiss predictions it works fine.
I'm primarily dealing with coupled-cluster amplitudes. I know they're huge in number. But I have a powerful system with Nvidia h100 + 512GB memory.

That seems like your ERIs are becoming large enough in memory that they're stored on disk. If you want to make sure they're always on RAM, my solution is to set mol.max_memory to be very high. Otherwise, if you actually need them to be on disk when they exceed the default memory limit, then use ao2mo.kernel(mf, ...) instead of ao2mo.general(mf._eri, ...).

@obackhouse Thank you; it worked perfectly. Could you please guide me on how to monitor this memory usage (whether it's on RAM or storage and how much it is using) within PySCF?

You can set the memory limit with the aforementioned Mole option, or with the respective config option. If you want to estimate the memory usage of the integrals themselves then you can just consider that the integrals have dimension 4 and 64 bits per element
nmo = mo_coeff[:, self.ccsd.frozen :].shape[1]
footprint_GB = (nmo ** 4) * 64 / 8e9


[Add polygon] Nothing changes when first vertex is added

There is no indication that a vertex was added when Add Point is first clicked.
@shobhitagarwal1612 Could you PTAL when you have a sec?

A couple of ideas:

Setting the default text of button as "Start" which changes to "Add point" after clicking it?

Showing selected vertex counter at the top

Adding a marker on the start position which gets removed after completing the polygon drawing.

@gino-m Thoughts?

Hi @shobhitagarwal1612 I'd be in favor of option #3, since it's consistent with the rest of the flow.
We will probably need to add ephemeral round markers (markers that are shown only in add/edit polygon) to the map anyway for two reasons: 1) Maps SDK only shows the round markers as end caps on the poly line while drawing, not on all vertices as expected and 2) the edit flow will need round markers on all vertices as affordances for selecting a vertex to edit.
Wdyt?

That sounds good to me. Since we need handle rendering of transient polygons separately, i feel this should be the order of changes:

Separate the rendering of saved polygons and temporary polygons.
Create feature from the temporary polygon at the end rather than from the beginning to prevent recreation of feature on every vertex update. Also decouple selected project and layer from PolygomDrawingView, if possible.
Add logic to add a marker for each temporary vertex (except the last one).

Wdyt?

Sounds reasonable. To achieve (1), would we decouple Feature from the maps interface, and pass points and polygons instead?


Colors of vanilla teams not working

When player in team with custom color, the nickname in messages still white

How do you set the nickname color?

By changing team color. Without this mod nickname color changes as usual.


Pool class improvements

Add proper unit tests for the Pool class, then change the implementation to use an std::priority_queue instead of an std::set.
This data type uses less memory and is generally faster for this specific case.

Note: I fixed an issue with duplicate edges (and added a relevant unit test for them), as well as measured the speed up with the Fuchsia build (which uses pool), of about 8% (!).

Well, I can't  understand the Windows hang, do not have a Windows machine to reproduce it, and it seems I cannot reproduce my performance differences today vs master (I suspect I messed up the compilation flags between the two branches, or a fluke due to background processes that randomly start on our corporate workstations and eat a decent amount of CPU), so I prefer to close this PR for now. Maybe for another time :)


What infrastructure is in place for handling dependencies between pipelines?

Complex bioinformatics workflows usually have dependencies from one step in a workflow to the next.  Is there functionality in the pipelines API to handle this?  If not, any suggestions on implementation of such workflow dependencies, ideally with caching of previously computed results?

Hi Sean,
I can be done in batches where you run/save data with one pipeline, and then pick it up with another since the I/O throughput with Google Storage is very fast.
Like you, I come from the same background, and I suggested a possible approach a while ago here:
https://github.com/googlegenomics/pipelines-api-examples/issues/13#issuecomment-194038122
But the purpose of Pipeline API in Google Genomics is different, and it's easier to just quote Matt:

The idea of the pipelines API as it stands is to provide this very simple, but powerful, building block. It will enable many different pipeline runners (like cromwell) to add a "run in cloud" feature to their existing workflow definition files without the need to explicitly provision a fixed cluster (like Grid Engine or Mesos).

There are other connected pipelines implementatins one can use with Google like Dataflow, which come closer to what you looking for, but that requires that the data be loaded into the Google Genomics API first - below is a link to the repository with examples:
https://github.com/googlegenomics/dataflow-java
If you have not used the Dataflow API before, below is how to construct a Dataflow pipeline:
https://cloud.google.com/dataflow/pipelines/constructing-your-pipeline
I still think that connected pipelines is a critical feature of the Pipeline API.
Hope it helps,
Paul

In addition to Broad Institute's Cromwell runner for WDL, a new open source project called Funnel builds on top of the Pipelines API to support complex workflows defined using Common Workflow Language (CWL). Funnel is being developed by the folks who run the DREAM challenges. The authors presented progress at a workshop last week at Institute for Systems Biology.
Another idea is to write a tiny python wrapper that makes each call to the Pipelines API blocking. Then you can call it multiple times in a row, with different bioinformatics tools, to build a simple pipeline.
About Paul's suggestion of using Cloud Dataflow (and the Apache Beam python SDK), that's also a possibility. You can imagine using Pipelines API to run individual steps, and using Dataflow for orchestration.
Features like caching of intermediate results, and retries, and preemptible VMs to reduce cost, are all things that can be added, and are definitely desirable. They're outside of the current scope of the Pipelines API and are probably best built on top, along the lines of Cromwell and Funnel.
Cheers,
Jonathan

Thanks, Jonathan, for the clarification of scope for the pipeline API.  I agree that building on top of it, treating the pipeline API as a "raw executor", makes a lot of sense.  There are a number of really good workflow engines out there already. Adapting them to the Google Genomics pipeline API is probably just a matter of time for at least some of them.

Do you have any particular workflow engines in mind that you'd like to see
adapted to support the Pipelines API? Just curious which you like best.
On Tue, Aug 9, 2016 at 9:29 AM Sean Davis<EMAIL_ADDRESS>wrote:

Thanks, Jonathan, for the clarification of scope for the pipeline API. I
agree that building on top of it, treating the pipeline API as a "raw
executor", makes a lot of sense. There are a number of really good workflow
engines out there already. Adapting them to the Google Genomics pipeline
API is probably just a matter of time for at least some of them.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238609443,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAiXqQcGCOPYuFNMxv74RFzqLSOnKIXxks5qeKrIgaJpZM4JgFjs
.

Toil and nextflow are both getting a fair amount of love-and-care and have at least some support for CWL, abstract executors, and cloud files.  Snakemake also has a good following, but I see this in a slightly different space.  I also like the approach that https://github.com/GoogleCloudPlatform/appengine-pipelines uses (returns promises).  A pretty full list is available here:
https://github.com/pditommaso/awesome-pipeline

Thanks! We're definitely keen to help the Toil folks support Pipelines API
and Google Cloud Platform generally. What I really like about nextflow is
that it's python.
On Tue, Aug 9, 2016 at 9:58 AM Sean Davis<EMAIL_ADDRESS>wrote:

Toil and nextflow are both getting a fair amount of love-and-care and have
at least some support for CWL, abstract executors, and cloud files.
Snakemake also has a good following, but I see this in a slightly different
space. I also like the approach that
https://github.com/GoogleCloudPlatform/appengine-pipelines uses (returns
promises). A pretty full list is available here:
https://github.com/pditommaso/awesome-pipeline
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238618294,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAiXqe0TVYnTUJTby2sL00tvgZpfHTxjks5qeLGigaJpZM4JgFjs
.

I assume that you mean toil is python?  Nextflow is groovy-based, though it is really a mini language in a sense.

On Aug 9, 2016, at 1:07 PM, jbingham<EMAIL_ADDRESS>wrote:
Thanks! We're definitely keen to help the Toil folks support Pipelines API
and Google Cloud Platform generally. What I really like about nextflow is
that it's python.
On Tue, Aug 9, 2016 at 9:58 AM Sean Davis<EMAIL_ADDRESS>wrote:

Toil and nextflow are both getting a fair amount of love-and-care and have
at least some support for CWL, abstract executors, and cloud files.
Snakemake also has a good following, but I see this in a slightly different
space. I also like the approach that
https://github.com/GoogleCloudPlatform/appengine-pipelines uses (returns
promises). A pretty full list is available here:
https://github.com/pditommaso/awesome-pipeline
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238618294,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAiXqe0TVYnTUJTby2sL00tvgZpfHTxjks5qeLGigaJpZM4JgFjs
.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238620931, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFpEwJgUInjf_fbUdUgbfe8fXtoL7usks5qeLO0gaJpZM4JgFjs.

Yes, typo! I meant Toil is python.
On Tue, Aug 9, 2016 at 10:08 AM Sean Davis<EMAIL_ADDRESS>wrote:

I assume that you mean toil is python? Nextflow is groovy-based, though it
is really a mini language in a sense.

On Aug 9, 2016, at 1:07 PM, jbingham<EMAIL_ADDRESS>wrote:
Thanks! We're definitely keen to help the Toil folks support Pipelines
API
and Google Cloud Platform generally. What I really like about nextflow is
that it's python.
On Tue, Aug 9, 2016 at 9:58 AM Sean Davis<EMAIL_ADDRESS>wrote:

Toil and nextflow are both getting a fair amount of love-and-care and
have
at least some support for CWL, abstract executors, and cloud files.
Snakemake also has a good following, but I see this in a slightly
different
space. I also like the approach that
https://github.com/GoogleCloudPlatform/appengine-pipelines uses
(returns
promises). A pretty full list is available here:
https://github.com/pditommaso/awesome-pipeline
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<
https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238618294
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/AAiXqe0TVYnTUJTby2sL00tvgZpfHTxjks5qeLGigaJpZM4JgFjs

.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <
https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238620931>,
or mute the thread <
https://github.com/notifications/unsubscribe-auth/AAFpEwJgUInjf_fbUdUgbfe8fXtoL7usks5qeLO0gaJpZM4JgFjs
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/googlegenomics/pipelines-api-examples/issues/35#issuecomment-238621323,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAiXqUhMWS7J4DOifi5NaSfpvD-oDZweks5qeLQHgaJpZM4JgFjs
.


Issue with Pkg.add("Mosek")

Hello,
I am new to Julia and have just installed JuMP. I am looking to install Mosek.jl as well. I previously had Mosek (complete with license) on my computer. I tried "Pkg.add("Mosek") and got the error " ERROR: LoadError: Failed to get MOSEK download host".
After reading online, I uninstalled MOSEK completely, removed all environment variables manually from my system, deleted the packages in my .julia directory. I then proceeded to do Pkg.add("Mosek") again. And I get exactly the same issue as previously. It seems I have tried everything I can think of and still nothing works.
It would be great to get help on this issue. Please let me know if there is anything I can provide to make things easier.
Thanks!
Georgina

Hello,
I have been interacting with Mosek and it seems that the issue is between a mismatch of versions (the one expected by Mosek.jl is not the one that is delivered by Mosek). See this thread: https://mail.google.com/mail/u/0/#inbox/FMfcgxwChJlBLZgMDlncZJKcnTXVJRhL
They have been unable to resolve this issue. Would it be possible to get some help there?
Many thanks!
Georgina

Issue resolved here:
https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/mosek/gK5dDfiatHg/AUHd-jIuBQAJ
In conclusion: We use powershell to download and unpack the distro. It appears that that may be disabled for security reasons causing download and/or unpacking to fail. This is difficult to fix since we need an external program to download+unzip, and powershell is the only reliable option.
A work-around is to download and unpack the relevant distro to the default place ($HOME/mosek) before installing or building Mosek.jl.


Allow users to Use Sparse Solver

Sometimes, we might want to fit a model with fixed effects, but without projecting them out. I.e. a model as
pf.feols("Y ~ C(f1) + C(f2)", data = data)

In this case, it would be nice if users could use the sparse solver in scipy.sparse.linalg.lsqr.
We would have to add this option to the "solver" function argument of pf.feols() https://github.com/py-econometrics/pyfixest/blob/77e7080d04e103aadaca2f2d765f409982ce7c5a/pyfixest/estimation/estimation.py#L34
and propagate this to
https://github.com/py-econometrics/pyfixest/blob/77e7080d04e103aadaca2f2d765f409982ce7c5a/pyfixest/estimation/feols_.py#L427
Note that this will only speed up the model fit, but not necessarily the computation of standard errors (which are not so easily adapted to a sparse solver).

Actually, I realize that the sparse solver is already supported, but does not compute on sparse matrices.
To support this, we'd have to update the following lines
https://github.com/py-econometrics/pyfixest/blob/77e7080d04e103aadaca2f2d765f409982ce7c5a/pyfixest/estimation/feols_.py#L475
to
if _solver == "scipy.sparse.lsqr":

            self._tZX = csr_matrix(_Z.T) @ csr_matrix(_X)
            self._tZy = csr_matrix(_Z.T) @ csr_matrix(_Y)
else:
    ... as before


Limit fetching children to prevent hang with uninitialized data struc…

…tures

Hi @richardstanton, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Richard Stanton). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

This is a reasonable fix for build, but we should have a bug tracking a better long term fix (ex: adding a 'more' element).

I think I might also suggest adding an element to the end indicating that the limit is reached unless that is really tricky to do.

The limit in the vs debugger is 5000. What was the motivation behind using 1000 here? Was 5000 too big?
We had previously spoken with Eric about his experience with this same problem in the EE and he had also suggested we need an expandable more element. I'm not sure it fits for build though like Gregg says.

Other than that, LGTM

Gregg, I agree that we do need a more long term fix, I tried to add a "truncated" element to the end of the list and it was trickier than I thought. I will investigate a more long term solution. Jackson, I initially tried 5000 elements, but the timeout was too long so I reduced it to 1000. Even with 1000 it still takes about 7 seconds to expand.

Hi @richardstanton, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Richard Stanton). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

I learned how to add people to the pull request. I know some of you have already commented on this, but just for formality:
@gregg-miskelly @jacdavis @chuckries @paulmaybee

@dotnet-bot build please

@dotnet-bot build please

@dotnet-bot build please

@dotnet-bot build please

build machine appears to be in a bad state. I'm investigating.

@dotnet-bot test this please


React native cloudinary ready video player for hls , android and ios

React native cloudinary ready video player for hls , android and ios
that will be amazing

@hamdij0maa Building a video player for our React Native SDK is in our pipeline but there is no ETA for it.
Since our video player is based on video.js we cant use it in React Native and React Native does not have a built-in video player, which means you'll have to use a third-party video player such as:

https://www.npmjs.com/package/react-native-video-player
https://www.npmjs.com/package/expo-video-player

Here is a sample Cloudinary integration with a video at the moment in React Native:
import { SafeAreaView } from 'react-native';
import VideoPlayer from "expo-video-player";
import { CloudinaryVideo } from "@cloudinary/url-gen";
import { URLConfig } from "@cloudinary/url-gen";
import { CloudConfig } from "@cloudinary/url-gen";
import 'react-native-url-polyfill/auto';

// Set the Cloud configuration and URL configuration
let cloudConfig = new CloudConfig({ cloudName: 'demo' });
let urlConfig = new URLConfig({ secure: true });

export default function App() {

    // Instantiate and configure a CloudinaryVideo object.
    let myVideo = new CloudinaryVideo('dog', cloudConfig, urlConfig);

    // Render the image in a React component.
    return (
        <SafeAreaView>
            <VideoPlayer
                videoProps={{
                    source: { uri: myVideo.toURL() },
                }}
                style={{ width: 300, height: 200 }}
            />
        </SafeAreaView>
    )
};

You can also use Cloudinary video player with webview but you may have issues with this, here is a sample:
<WebView
   source={{uri: 'https://player.cloudinary.com/embed/?public_id=cld-imagine&cloud_name=demo&source[sourceTypes][0]=hls&source[transformation][streaming_profile]=auto'}}
   style={{marginTop: 20}}
/>

this is good for simple use cases, but for a list of videos, this will not work smoothly at all.
I am talking about an optimized video player for hls with caching ..

Hi Hamdi.
As mentioned by my colleague, we do have something in the works, but I'm afraid we don't have a timeline that we're able to provide at this stage. In the meantime, I would recommend trying Tommy G's workaround above, and keep an eye on our documentation pages over the coming months for an update regarding a video player for React Native.

sure, looking forward, thanks.
it will be amazing, if you need alpha/beta tester let me know i'll be glad to help


Move integration tests for function upload/download to apache

This change add function-worker into the PulsarClusterTestBase to create a test base for functions related tests PulsarFunctionsTestBase.
Move the existing upload/download tests to use it.

This PR is based on #2060.
change 55d024a is the gitsha for this PR.

retest this please


Support for Compass configuration properties and callbacks

Allow path to compass config file to be specified.
See
https://github.com/Jasig/sass-maven-plugin/issues/43
https://github.com/Jasig/sass-maven-plugin/issues/51
https://github.com/Jasig/sass-maven-plugin/issues/60

+1

This version is no longer maintained. Refer to more recent forks at https://github.com/Jasig/sass-maven-plugin/network for more up to date versions.


formatOptions from JPEGSaveOptions not working.

Foremost, I want to thank you for this incredible library. I have a simple question, I'm trying to save a JPEG file, but I'm having trouble setting the format to Progressive, here's my code:
from photoshop import Session
from photoshop.api._core import Photoshop
from photoshop.api.enumerations import MatteType
from photoshop.api.enumerations import FormatOptionsType

with Session(jpegpath, action="open") as ps:
	resolution_str = ps.active_document.resolution
	change_size = ps.active_document.resizeImage(new_width, new_height, resolution=resolution_str, automatic=8)
        save_as = ps.active_document.saveAs(copy_path, ps.JPEGSaveOptions(quality=12, embedColorProfile=True, matte=MatteType.NoMatte, formatOptions=FormatOptionsType.Progressive), asCopy=True)

However, I'm getting the following error:
Traceback (most recent call last):
  File "D:\Image_size.py", line 9, in <module>
    ps.JPEGSaveOptions(quality=12, embedColorProfile=True, matte=MatteType.NoMatte, formatOptions=FormatOptionsType.Progressive), asCopy=True)
TypeError: __init__() got an unexpected keyword argument 'formatOptions'

What am I doing wrong?

the argument 'formatOption' does't existe.
JPEGSaveOptions(quality=5, embedColorProfile=True, matte=MatteType.NoMatte)
ref = https://loonghao.github.io/photoshop-python-api/reference/photoshop/api/save_options/jpg/#photoshop.api.save_options.jpg.JPEGSaveOptions

You can't pass formatOptions as a parameter, you have to set it after the instance is created, so just break that line up like so:
jpeg_options = ps.JPEGSaveOptions(quality=12, embedColorProfile=True, matte=MatteType.NoMatte)
jpeg_options.formatOptions = FormatOptionsType.Progressive
save_as = ps.active_document.saveAs(copy_path, jpeg_options, asCopy=True)


Fix: Multiple Matchers Consuming Request Body Causing Stub Handler Failure

This PR has fixed a bug where the stub handler would fail when there are multiple matchers that use the request body for matching.
This issue occurs particularly in scenarios where the path and method match exactly, and the API type, such as jsonrpc ^1, is included within the request body. In cases of stubbing APIs that incorporate the API type or operation kind in the request body, there might be multiple matchers specified that consume the request body.

@qawatake Thank you!!!!


FileNotFoundError: [Errno 2] No such file or directory: 'scierc_models/ent-scib-ctx0/ent_pred_dev.json'

Hello,
I keep getting the error mentioned on the title while running this script :
python run_relation.py   --task scierc   --do_train --train_file scierc_data/processed_data/json/train.json   --do_eval --eval_with_gold   --model  allenai/scibert_scivocab_uncased   --do_lower_case   --train_batch_size 8   --eval_batch_size 32   --learning_rate 2e-5   --num_train_epochs 3   --context_window 100   --max_seq_length 128    --entity_output_dir scierc_models/ent-scib-ctx0   --output_dir scierc_outputs

I have also tried with --eval_test argument and without using either --eval_test and --eval_with_gold, both throw the same error.
Earlier I had managed to run the first two evaluation scripts provided with the repo, seamlessly. They produced ent_pred_test.json and predictions.json, respectively and as expected. :
# Run the pre-trained entity model, the result will be stored in ${scierc_ent_model}/ent_pred_test.json
python run_entity.py \
   --do_eval --eval_test \
   --context_window 0 \
   --task scierc \
   --data_dir ${scierc_dataset} \
   --model allenai/scibert_scivocab_uncased \
   --output_dir ${scierc_ent_model}

# Run the pre-trained full relation model
python run_relation.py \
 --task scierc \
 --do_eval --eval_test \
 --model allenai/scibert_scivocab_uncased \
 --do_lower_case \
 --context_window 0\
 --max_seq_length 128 \
 --entity_output_dir ${scierc_ent_model} \
 --output_dir ${scierc_rel_model}

Could you please tell me if I am doing anything wrong?

Hi!
ent_pred_dev.json is the prediction file returned by the entity model on the dev set. To generate it, you need to run the entity model without --eval_test.
For example,
python run_entity.py \
   --do_eval \
   --context_window 0 \
   --task scierc \
   --data_dir ${scierc_dataset} \
   --model allenai/scibert_scivocab_uncased \
   --output_dir ${scierc_ent_model}

Let me know if there is still an issue!

I was also thinking about doing this. Thanks for the clarification :)
However, I am still confused about why a previous evaluation output file is required for the training task? Isn't evaluation done after training? Especially curious because I am trying to adapt the relation extraction model for the Bengali language. In that case, where do I get the evaluation ent_pred_dev.json?  Should I first run the run_entity.py model with the --do_train argument and then run again with the --do_eval argument to obtain the json?
Please, forgive me if my questions are a bit naive. Thanks!
ps: would really appreciate an issue or discussion on the pipeline of adapting this interesting work for other languages other than English.

We need ent_pred_dev.json during training because we need to evaluate the model on the dev set for model selection.
In our default setting, we use the predicted entities for model selection. This is why we run run_entity.py here to generate the prediction file.
If you want to do model selection based on the gold entities instead of the predicted entities, you can just use your gold .json file of dev set as ent_pred_dev.json and train the model with --eval_with_gold.
So to adapt the relation model on your dataset. You can either train an entity model first and run the entity model with --do_eval to obtain the ent_pred_dev.json; or you can just use the original gold .json file you have (copy and rename it as ent_pred_dev.json).

Thanks a lot! This has been helpful.