OPTIONS request handler missing Allow header

As per the MDN Docs, servers' responses to OPTIONS requests should include an Allow:  header that contains the allowed methods.

You are correct, but that is even if you are not using CORS. The Express.js server will add one for you, if you set up method for your path. This module is just for adding on CORS; your server should include Allow to OPTIONS requests without CORS at all too, which is all this module adds.

Can you show me a minimal reproducible example of an Express server that provides a GET endpoint, such that an OPTIONS request to that endpoint will yield a response with the Allow header?

$ node -e 'require("./index")().get("/foo",(req,res)=>res.send("Hello")).listen(3000)' &
[1] 622

$ curl -i -XOPTIONS http://localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Wed, 20 Sep 2023 19:46:40 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD

When I use the cors() middleware with minimal options:
$ node -e 'require("express")().use(require("cors")({origin: ["http://localhost:3000" ]})).get("/foo",(req,res)=>res.send("Hello")).listen(3000)' &
[3] 2226

$ curl -i -XOPTIONS http://localhost:3000/foo
HTTP/1.1 204 No Content
X-Powered-By: Express
Vary: Origin, Access-Control-Request-Headers
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Content-Length: 0
Date: Wed, 20 Sep 2023 20:09:21 GMT
Connection: keep-alive
Keep-Alive: timeout=5

no Allow header is present on the response.

Oh, interesting. So seems like there is some kind of bug in here that is suppression the Allow header or something 🤔

On a separate note; how would you suggest to register a 'catch-all' middleware such that it doesn't interfere with the built-in options request handling? For example:
$ node -e 'require("express")().get("/foo",(req,res)=>res.send("Hello")).use((req, res) => res.send("unrecognised")).listen(3000)' &
[2] 2260

$ curl -i -XOPTIONS http://localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Content-Type: text/html; charset=utf-8
Content-Length: 12
ETag: W/"c-GlU8XMDfqpSNdpbavBLUDkNCG0k"
Date: Wed, 20 Sep 2023 20:40:22 GMT
Connection: keep-alive
Keep-Alive: timeout=5

unrecognised

The auto options response in Express only takes effect if your code does not write a response. Since you wrote a response that said unrecognized, Express considers it as if you overrode the default OPTIONS handling with your own custom one, in which you would need to also send an Allow response header to whatever you want it set to.
General questions for how to use Express should ideally be in the Express.js issue tracker or discussion board, instead of the issue tracker of the cors middleware module, as there are more folks watching it to help instead of just myself and maybe a handful of others here.

Enabling cors for a single route, as cors npm doc shows doesn't mess the headers up.
node -e 'require("express")().get("/foo", require("cors")(), (req,res) =>res.send("Hello")).listen(3000)' &
[1] 10062

curl -i -X OPTIONS localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Mon, 25 Sep 2023 22:19:03 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD%

Enabling cors on a single route but with pre-flight enabled messes the headers too though.
node -e 'require("express")().options("/foo", require("cors")()).get("/foo", require("cors")(), (req,res) => res.send("Hello")).listen(3000)' &
[1] 10632

curl -i localhost:3000/foo -X OPTIONS
HTTP/1.1 204 No Content
X-Powered-By: Express
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Vary: Access-Control-Request-Headers
Content-Length: 0
Date: Mon, 25 Sep 2023 22:38:39 GMT
Connection: keep-alive
Keep-Alive: timeout=5

Also checked changing the status code from the cors options and express response just in case, but that isn't the cause either.
I know nothing about the express framework inner architecture but might be something the cors module is overwriting, given that cors has also request and response objects and similar functionality for some things like headers.

This line leads to this block executing which always end()s the response and does not call next().
Express' default OPTIONS request handling implementation is here, in the pillarjs/router repository. It is documented accurately:

for options requests, respond with a default if nothing else responds

the cors() middleware has already responded, so the default OPTIONS handling which generates the Allow header never executes.

Typo: preflight enabled makes cors call next and not end the res, preflight disabled (the default config) is what always ends the response, making it impossible to add nothing to It.
Based in what you explained, in the 'Simple Usage' example that you showed (cors globally enabled), if we change the default preflightContinue (false) to true, the response gets passed to the next handler and doesn't end, getting the wanted behaviour.
node -e 'require("express")().use(require("cors")({preflightContinue:true})).get("/foo", (req, res) => res.send("bar")).listen(3000)' &
[1] 5378

curl -i localhost:3000/foo -X OPTIONS
HTTP/1.1 200 OK
X-Powered-By: Express
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Vary: Access-Control-Request-Headers
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Tue, 26 Sep 2023 21:27:05 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD%

If I recall correctly, allowed methods were showing in the single route cors example because after cors its called, the (req, res) handler gets called "on top", setting the allow header in the function sendOptionsResponse that you noted.
Im not very sure how, but possibly the cors middleware doesn't execute the same way or even correctly, because if you check the single route test I did, it didn't had the common access control origin and access control methods headers that cors sets.

node -e 'require("express")().get("/foo", require("cors")(), (req,res) =>res.send("Hello")).listen(3000)' &
[1] 10062

curl -i -X OPTIONS localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Mon, 25 Sep 2023 22:19:03 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD%

So maybe is a bigger issue than it looks because the single route example I did is from the common-usage in the npm page.

Also had the issue that the Access-Control-Allow-Origin header was not present on the request headers.
After reading up a lot on the topic and also having a careful look at the README I finally figured it out.
This is the final code which works for me:
const allowedOrigins: string[] | string = "*"
expressApp.use(
    cors({
      origin: allowedOrigins,
      preflightContinue: true,
    })
  )
  expressApp.options(
    '*',
    cors({
      origin: allowedOrigins,
    })
  )

Three important things I learned on this topic:

If you wanna apply a wildcard, you have to pass "*" as a string, and not as an array like ["*"]
If you wanna apply specific domains/hosts as origin property, when testing and e.g. sending an OPTIONS request via Postman you have to make sure to pass a matching Origin header to the request, otherwise the Access-Control-Allow-Origin header won't show up.
Make sure you mount/run the cors middleware before every other middleware. Otherwise e.g. auth middlewares could exit the request before cors could add some headers to the request.


Watch-znode script for docker image

Docker compose doesn't have any way to run initialization like k8s
does, so you need to role your own solution. This script allows you to
wait for a znode to be created in zookeeper, check if it exists and
create it. It can be used to ensure that pulsar brokers and bookies do
not come up before metadata format has run.

@merlimat I'd like to get this into the base image, so that it can be used to build a docker compose for the base image. it doesn't increase image size by much, even with dependencies.
apachepulsar/pulsar-test-latest-version             2.0.0-incubating-SNAPSHOT   a556ba5e68e2        3 days ago          889MB
apachepulsar/pulsar                                 2.0.0-incubating-SNAPSHOT   0bec7346169f        10 hours ago        884MB


Remove kde4workspace and kdetelepathy

Motivation for this change
As discussed with @ttuegel in https://github.com/NixOS/nixpkgs/pull/29219, this pr removes kde4_workspace and kde_telepathy as both are not maintained anymore.
Progress on: https://github.com/NixOS/nixpkgs/issues/28643
Things done

[x] Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)

[x] NixOS
[ ] macOS
[ ] Linux

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[x] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
[ ] Tested execution of all binary files (usually in ./result/bin/)
[x] Fits CONTRIBUTING.md.

Does this need to be backported to the release-17.09 branch?

I think so, because kde4_workspace did not compile without the changes of my other pr.


Document evaluation module

Page on evaluation measures in the documentation.

Explain that the compuation of reward needs intent annotations


"click" event will not capture middle-click events (Windows-only) in Chrome 55+

People are hitting this with React:
https://github.com/facebook/react/issues/8529
Should probably look at fixing this.

@silbinarywolf can you please confirm whether this remains an issue in the latest release/s? Basically just trying to determine whether this still needs attention.

@nglasl Yes it does. For handling middle clicks, you need to use "auxclick" now.
These guys hit a related issue:
https://github.com/googleanalytics/autotrack/issues/160


webpack.js.org/src/content/concepts/mode.md does not define precendence

Hey maybe it's defined elsewhere but if so I think it bears repeating in this section of the documentation.
It says that we can define the mode entry to dictate the mode to webpack, and that we can also define it in the command line using --mode= flag.
However it doesn't say which will be in effect if both the command line and webpack.config.js entry are defined.
Also does anyone know which wins? I assume command line will override config as it is more immediate, but I don't want to assume.

cc @ev1stensberg

Let me check & update the docs


[QUERY/Feature Request] Determining payload size on EventDataBatch before adding to batch.

I am wondering if there is an API to determine the payload size on the EventDataBatch.
We have noticed so far that there is some 16byte overhead per each message added meaning that the byte[].Length property cannot be used to determine the payload size on batch.
Are there any other peculiarities that should be taken into account besides the 16 byte overhead? If that is that, this is something that we could consider hardcoding at the moment.
The reason why I'd like to know the size in bytes on the batch beforehand is that I am consuming messages from a message queue and a single queue message contains multiple events. In order to prevent any data loss all events must fit onto the batch and reach the hub safe and sound before popping the message of the queue. For that I need to know the size of an event on batch before adding it to the batch as there is no api to remove events of the batch.
p.s. not sure what is the API design process, but as far as the code is concerned I could raise a PR with a couple of hints from you guys on where to look at.

Hi @hellooperatorpatchmethrough.  Thank you for reaching out and we regret that you're experiencing difficulties.  There is not currently a way to premeasure an event before adding to the batch.   There's a few parts to your inquiry, and I'm going to split into multiple responses to try and keep from throwing too large a wall of text at once.

Are there any other peculiarities that should be taken into account besides the 16 byte overhead? If that is that, this is something that we could consider hardcoding at the moment.

I would advise against assuming that your application can accurately measure the size of an event.  The overhead that you're seeing is not a constant guarantee and may change depending on the content of your event, the properties of your Event Hubs namespace and Event Hub instance, and implementation details of the protocol.

To accurately determine the size of an event, it must be measured in the format used by the active protocol as well as account for overhead of the batch envelope that will contain it.  Even were there a trustworthy sizeof equivalent in .NET to measure the model, it wouldn't be an accurate representation.  Best case, you'd end up wasting space because you'd have to reserve overhead padding defensively.  Worst, you'd end up with a miscalculation that causes publishing to fail.

The size limit for communication is not a property of the Event Hub, it is a property of the communications channel for the active protocol at the entity level.  This means that the service could choose to specify a different value for individual partitions of an Event Hub or for the gateway used for automatic routing and servicing of partition keys.   Pragmatically, this isn't how the service operates today, but the client cannot make that assumption and neither should an application.

Communication channels are ephemeral to a certain extent, where the client will potentially be opening, closing, and recreating links to the service to provide a stable and resilient experience for callers that doesn't waste resources.  As the service controls the size, any point where a new link is opened, the maximum size allowed for it may differ.   Here again, this isn't likely, but it is possible and so the client cannot make assumptions.

The reason that EventDataBatch exists is to provide a deterministic and accurate means to measure the size of a message sent to the service in the context of a single operation, minimizing the chance of failure.  Because the batch is created in the context of a link to the service, the size limit is the most accurate available.  It is still possible for that value to change and a batch be rejected, but it is as close to per-request validation as possible.

The reason why I'd like to know the size in bytes on the batch beforehand is that I am consuming messages from a message queue and a single queue message contains multiple events. In order to prevent any data loss all events must fit onto the batch and reach the hub safe and sound before popping the message of the queue. For that I need to know the size of an event on batch before adding it to the batch as there is no api to remove events of the batch.

I'd like to understand a bit more about the scenario, if you're willing.

Are you using a Peek/Lock approach to reading the queue?

Assuming yes, if you peek a message and attempt to add it to the batch but it does not fit, is there a reason why you wouldn't be able to just abort the read?  You don't mention a specific queue, but most will allow you to abandon the lock without removing the message.

What happens if you form an event batch and publishing is unsuccessful today?  I'm assuming that you complete the queue read upon successful publishing of a batch.  On failure, I'm assuming that you're doing something today that allows you to retry in a safe manner.   I'm curious why you wouldn't apply this same technique to a message that you attempt to add to a full batch.

p.s. not sure what is the API design process, but as far as the code is concerned I could raise a PR with a couple of hints from you guys on where to look at.

There's an involved process for changes to the public API that requires championing the proposed change across the different languages for an SDK as well as getting approval from a central architecture board.  While we very much appreciate contributions, we'd probably want to wait until we know this is something that we'd be able to move forward on.
The first step for doing so ties in with my previous response.  The more that I understand the scenario, the better I can make a case for consideration of a new API if there's no better approach that exists today.

@jsquire
Thank you for a such detailed response.
We are ongoing a modernization process in the organization and first and foremost we are upgrading our content delivery channels. To do so we are reading the intermediate data product produced by services that communicate via the good old MSMQ. MSMQ is being read by what we can call an event producer. MSMQ messages contain multiple events in a collection and these messages share the same metadata set. We expand the collection into multiple messages so we could cluster them to different partitions so the events that correspond to a same entity would be stored on a single partition. This allows us to scale out horizontally within a single process as the event consumer application mostly waits for IO to complete - data batch insertion on the data vault.
The MSMQ is being read in a single threaded context, there is no other process that has access to that queue. Messages are being peeked; message contents are received without popping the message of the queue. This is done up to the event batch size limit. Once the limit is reached, the event data batch is sent to the hub and after it was successfully sent out, a number of messages is dequeued at once. You can say it is done in "all or nothing" manner. This is all done using the MSMQ cursors.
Well to be honest, this is not something that we have implemented yet but this is in our plans once we resolve how to determine the message size beforehand placing it on the event data batch.

Assuming yes, if you peek a message and attempt to add it to the batch but it does not fit, is there a reason why you wouldn't be able to just abort the read? You don't mention a specific queue, but most will allow you to abandon the lock without removing the message.

This is worth to consider.  Hell, as the event hub is a "guaranteed delivery at least once" ingestion service, the mechanisms that deal with duplicates are already there.
...
From your response it is clear that it would be naive to attempt to capture the event size determination algorithm as there are too many things at play. We do not have such great need to publish events as optimally as possible because the bottleneck in our pipeline is storing data on the data vault. This means that slashing the batch size defensively by let's say 10% could result in a better performance than producing more duplicates which would lead to more wasted IO.

Apologies for the lack of feedback.  We've been discussing potential approaches internally to see if there was some form of premeasurement that we could expose that felt like it wouldn't crowd the public API surface or cause confusion around the recommended publishing patterns for those without your scenario.  Unfortunately, the decision at this point is that your scenario is unique enough that it there isn't justification for expanding the public API.
Since it seems that you had some thoughts on ways to work around this within the application and we've reached a conclusion on potential client changes,  I'm going to tag this as being addressed.  Please feel free to "/unresolve" if you'd like to continue discussion.


Prevent chainquery from exploding (Mark and Niko know)

[x] Get Niko passed his stuck block
[x] Investigate query load


Gulp Depdendency Issue

I'm getting this error when trying to install the package in my project via npm:
npm ERR! notarget No compatible version found: gulp@'undefined'
npm ERR! notarget Valid install targets:
npm ERR! notarget ["0.0.1","0.0.2","0.0.3","0.0.4","0.0.5","0.0.7","0.0.8","0.0.9","0.1.0","0.2.0","1.0.0","1.1.0","1.2.0","1.2.1","2.0.0","2.0.1","2.1.0","2.2.0","2.3.0","2.4.0","2.4.1","2.6.0","2.6.1","2.7.0","3.0.0","3.1.1","3.1.2","3.1.3","3.1.4","3.2.0","3.2.1","3.2.2","3.2.3","3.2.4","3.2.5","3.3.0","3.3.1","3.3.2","3.3.4","3.4.0","3.5.0","3.5.1","3.5.2","3.5.5","3.5.6","3.6.0","3.6.1","3.6.2","3.7.0","3.8.0","3.8.1","3.8.2","3.8.3","3.8.4","3.8.5","3.8.6","3.8.7","3.8.8","3.8.9","3.8.10","3.8.11","3.9.0"]
npm ERR! notarget
npm ERR! notarget This is most likely not a problem with npm itself.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.
npm ERR! notarget
npm ERR! notarget It was specified as a dependency of 'angular-multi-step-form'

Does this have anything to do with the project's dev dependency on the 4.0 branch of gulp?

@chrisguerrero thanks for reporting this. It is now fixed with 1.2.5.


Maximum update depth exceeded error with hover card

Bug report
Current Behavior

Been banging my head against the wall with this one, so maybe someone has an idea on how to fix this.
Here is the full error stack trace:
Unhandled Runtime Error
Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

Call Stack
throwIfInfiniteUpdateLoopDetected
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (26657:10)
throwIfInfiniteUpdateLoopDetected
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (7679:2)
getRootForUpdatedFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (7570:9)
enqueueConcurrentHookUpdate
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (12929:15)
onAnchorChange
<EMAIL_ADDRESS>(62:16)
create
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (21035:22)
commitHookEffectListMount
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23088:6)
commitHookPassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23193:10)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23304:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)
commitPassiveMountOnFiber
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23171:6)
recursivelyTraversePassiveMountEffects
node_modules/next/dist/compiled/react-dom/cjs/react-dom.development.js (23190:8)

I am getting this error primarily when I am using the Radix hover card with code that re-renders frequently (in my case, react-query). This can be seen in the minimal reproduction linked later in this post. As react-query's useQuery hook rerenders its parent, it causes the HoverCard to mount and rerender, causing the error. I've tried to memoize the hover card, but this did not fix the issue.
Expected behavior
Error should not occur
Reproducible example
CodeSandbox Reproduction
Steps to reproduce

Open Preview console in the CodeSandbox

Refresh the CodeSandbox window

Note the error in the console

Suggested solution
It seems to be related to the onAnchorChange function call, but i'm not sure precisely how to solve this or where the root problem is (the library or my code).
Additional context
No error is thrown if I only render a few buttons. Seems like React has a built in max limit before it throws an error.
For my use case where I need to render 50-100 items at once with a hover card for each item, this error is a breaking issue.
Your environment

Software
Name(s)
Version

Radix Package(s)
@radix-ui/react-hover-card
latest

React
n/a
latest

Browser
Arc
latest

Assistive tech

Node
n/a
latest

npm/yarn

latest

Operating System
Mac OS Sonoma
latest

I see some other warning:

It's unlikely to be an issue with the primitive, but rather with the specific setup.
I will close the issue now, but feel free to keep chatting if you have more info to present.

I see some other warning:
It's unlikely to be an issue with the primitive, but rather with the specific setup.
I will close the issue now, but feel free to keep chatting if you have more info to present.

Hi @benoitgrelard — thanks for responding.
I was going off of the official radix-ui/primitives CodeSandbox template provided when I made this bug report.
I switched to createroot and it seems to address the issue.
I am using NextJS in my own application (where I am encountering the issue), so I could not make the same createRoot change there. I'm wondering if it's a clue though that something in my setup may be causing the issue.

I'm getting this as well in Next - the only useful info to debug is that popper appears in the call stack:

I also have a popover that re-renders frequently, and then causes this error. This does seem like an issue with the primitive.

I am also getting this issue when using Popover

Same here, for me, it happens when I'm resizing the window.

Happened to me on a page where I was using 50+ popovers.

Something is definitely not right here.

I have a common component for navigation buttons with a tooltip. I've got probably 4-6 of these on screen at a time, and occasional re-renders cause this error with a huge depth.

Glad I'm not alone. This is not an actual fix but for my own purposes I've just wrapped my component in an error boundary for now so that it doesn't bubble up and crash the entire application.
Hopefully this can be properly fixed though.

I am also experiencing this with many HoverCards that re-render often. Would appreciate any pointers to fix this!

@benoitgrelard Would we be able to reopen this issue as it seems like many others are also experiencing the same bug. Thank you!

Got similar issue when using Radix-ui dropdown menu with Next.js. Please take a look at this! @benoitgrelard The console report Maximum update depth exceeded too.

Having the same issue with Nextjs + Radix + reactflow when a context menu wraps over the nodes and the nodes start being dragged

Did anyone found some workaround? I am getting this error also with the DropdownMenu.Trigger primitive with Next.js each time I do an api call

@benoitgrelard any ideas or will this issue be reopened as it seems to be a recurring issue?

I suddenly started getting this error out of no where. The popover worked fine a few months back and now all of a sudden I am getting this.

Also experiencing this when using Radix Dialog that has a material UI Popover or Select component inside:

Also experiencing this when using Radix Dialog that has a material UI Popover or Select component inside:

I am actually considering switching to Daisy UI instead of using Shadcn because of this. This is a UI breaking bug. And seems like there is no activity on this.

Also experiencing this when using Radix Dialog that has a material UI Popover or Select component inside:

I am actually considering switching to Daisy UI instead of using Shadcn because of this. This is a UI breaking bug. And seems like there is no activity on this.

At the first glance I thought was due to my misuses of useEffect, but after reviewing people's comments, looks like it's not my fault🥲. It takes me a while to end up here ...

Folks, I upgraded my react version to the latest minor version of major version 18. This seems to have been fixed since then. Give it a try.

any updates here? on react 19.0.0 (latest), still happening, so unfortunately no option to upgrade


Status bar background color washed-out when in debug session

See here:

Changing to the red/orange colour, thanks! (also matches Visual Studio itself, so good choice from me :P)

Accidentally Released Beta 6 without this change, pushed out 7 to get this, tell me what you think in both themes!


add support for new trigger build by project endpoint

fixes #35

Hi, great work on this - I need this exact feature. 😄 I decided to use your branch while waiting for this to get merged, but noticed I had to change /v1/ to /v1.1/ in the POST request to trigger the build in order for this to work. I get a 404 otherwise. The version change is also reflected in CircleCI's docs, which I guess must've changed since you implemented the feature.

@eaardal great catch, thank you! Will make that change now

@eaardal actually, it looks like the API version is set by the library's caller when they instantiate a Client object (in BaseURL): https://github.com/jszwedko/go-circleci/blob/master/circleci.go#L44 I'll just add a comment that 1.1 is required for that method...

@lexiross I pushed #39 with the split methods because I had some downtime. I'm curious if you have any thoughts on it, otherwise I'll aim to merge it within the next week or so.

@jszwedko ahh so sorry, work got a lil crazy and I totally let this slip 🤦♀ #39 looks great, thank you 👍

No worries! Thanks again for the initial PR.


Allow custom names / descriptions for repositories

Currently a repository is only identified via its URL, which is quite "builky" in the UI. Also, some way to attach free text information to repositories in needed, e.g. to better describe the "thing" that is hosted in that repository.
To solve both issues at once, I propose to create a free text "Name" property for a repository. That name should then be used as a product-wide unique identifier for the repository that is also used in the breadcrumb bar and elsewhere.
Less preferable, the URL could stay the identifier, but an additional "Description" property could be added for a repository.

Relates a bit to https://github.com/eclipse-apoapsis/ort-server/issues/1368.


change user's RoleID to AccessRole type

change user's RoleID to AccessRole type.
Maybe It's better to use int type.

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #48   +/-   ##
=======================================
  Coverage   96.41%   96.41%
=======================================
  Files          21       21
  Lines         502      502
=======================================
  Hits          484      484
  Misses         11       11
  Partials        7        7

Impacted Files
Coverage Δ

internal/user.go
100% <ø> (ø)
:arrow_up:

internal/rbac/rbac.go
100% <100%> (ø)
:arrow_up:

cmd/api/request/account.go
100% <100%> (ø)
:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a20e21...d6df7b8. Read the comment docs.


Install fails missing python

I have installed 3 chia forks on the system. All installed no problem. Tried Spare and I get install.sh line 115 python, command not found.
None of the other installers had this problem.
Ubuntu 20.04.
python 3.8 is installed.
seems to be a path issue which spare isn’t setting up correctly.

after I created a link to python3 the install still fails, after that fix it fails again. Feels like the installer was not tested on a vanilla Ubuntu...

python -m pip install --extra-index-url https://pypi.spare.net/simple/ miniupnpc==2.1
python -m pip install -e . --extra-index-url https://pypi.spare.net/simple/

The installer reffers to a parked domain which seems to be not even associated to spar.farm...

He did a find replace for chia -spare. In the bottom of install script are two spare.net references. They should be chia.net.
Nothing is working even after fixing that.

This worked for me. Run
sudo python3 setup.py develop

Not for me :(

Try this assuming you have chia-blockchain in the same folder.
git clone https://github.com/Spare-Network/spare-blockchain.git . ./chia-blockchain/activate  cd spare-blockchain sh install.sh . ./activate spare init


Script to post comments on Feedback PRs

As discussed with @AndrewPaulChester  just now, we want a script that goes through each repo in the repo.csv database and makes a comment in the repo Feedback PR to paste the marking results.
The input will be:

CSV file of the MARKING spreadsheet
Text file of the autograder marking

This way we get rid of bulk email via YAMM and links to Google Drive for the report, everythign stays within the feedback PR in the Github repos.
We will use the PyGithub Python library, here is an example how to add a a comment:
https://pygithub.readthedocs.io/en/latest/examples/PullRequest.html?highlight=pull request
Basically, we want to paste a form of this table which used to be sent by email to students:

let's do it!! :muscle:

@ssardina we want to use the issue comment feature, not the pr-review-comment feature.
You were right when you said that the review comments were the special ones attached to specific lines of code. Instead, since all PRs are issues, we just want to make a standard issue comment.

See https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina/pull/1 for a basic proof-of-concept I have working.

We discussed a few different ways to attach the report itself:

Attach as a file in the PR comment
Post as a second comment to the PR
Commit into the repo and link from the PR comment.

It does not appear that option 1 is possible through the open API.
Option 3 I don't really like - it feels a bit strange to commit feedback as a file directly into their repos.
I would vote for option 2 - The nice thing is that even though the report can be long, it is already in markdown, so posting it as a comment is actually nicer to read than as a straight file like they used to get in the folder.

I've added a script that should basically work. A few things to note:

My biggest concern is what if we have errors and have to selectively retry some students only. There is the capability to do this by digging through the logs and handling it manually, but if this isn't piped into a file then the info could be lost. I wonder if it would be better to have an explicit list of either successes or failures which could interact with the --repos argument.
Further formatting can be done to the table as required

OK done with the post, now going into this full mode...

My biggest concern is what if we have errors and have to selectively retry some students only. There is the capability to do this by digging through the logs and handling it manually, but if this isn't piped into a file then the info could be lost. I wonder if it would be better to have an explicit list of either successes or failures which could interact with the --repos argument.

mm I am not following. You mean we need to "remark" a student?
We can add an option to the script to restrict the posting to a particular GH username (repo), I have that with almost all scripts. We can also add an option to include a top message, like "REMARKING",
We can also do it manually as well, pasting ourselves, etc.. but the script can do it as well for 1 student right? or a list of students.
or did I get it wrong what you said?

Not quite, I'm not talking about remarking, but if the script itself fails for some students so they don't get the initial comments.
I'm imaging a scenario where we run into some rate limiting issue with the API so 30% of students have randomly errored in no particular pattern. We can copy paste those students from the logs, but would be hugely time consuming. If we store the errors just bare as a file that can then be piped into the --repos option like you mentioned. I'm 75% done with a patch to help with this.

@ssardina I've just pushed an update that has better error handling. I will try to pretty up the table section of the report a bit now, but if there's something more important let me know

OK great yes I agree with your strategy, we will send all the console report to file and we can also catch and report the error repos, I suspect that is what you did. I will now try it

@ssardina Done - see the latest comment for a preview https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina/pull/1
I think this is functional for p0. I can see many more improvements that could be done for p1 (e.g. refactoring the markdown for reuse between projects), but in the interest of time this seems good to me.
I'll be around for another hour or so, let me know if I can help in any other way.

Tahnks @AndrewPaulChester ! checking it

@AndrewPaulChester did you push the very late changes for pretty print?

While I'm thinking about it, other things we could consider improving in this process:

[ ] Tagging the student using @username format so they automatically get a notification.
[ ] Updating the tables: some of the rows have fairly cryptic names, with the added markdown flexibility we might be able to do better.
[ ] Refactoring the markdown table to allow reuse of the shared components between projects, while the question specific format can be easily swapped out.
[ ] Consider making a basic comment for students who did not successfully submit, either due to lack of certification, or lack of the correct tag. This would require a slight code change - currently we simply ingest the entire CSV file and put it into the template string immediately. If we want to programatically access the values in the spreadsheet columns we would need to load the csv as a dict/list of dicts and then generate the text later.

Done the first one!

OK fuly completed and done, I added a lot of improvements, the main being:

Get cases with no certification or no submission, with targeted messages
Get report file name from CSV file now (that needed extending grader system); bc some reports are xxx_ERROR.txt
Ability to give number limits on which repos to process, for example 10 to 20
repo name non-sensitive for case
more that I cannot remember!

I have already sent the first pack foir AI24 P0 with this:
python ../git-hw-submissions.git/gh_pr_feedback_comment.py repos.csv marking-p0.csv reports  -t ~/.ssh/keys/gh-token-ssardina.txt -s 0 -e 10 |& tee pr_feedback_0-10.log
Thu, 08 Aug 2024 02:09:41 INFO     Starting on Australia/Melbourne: 2024-08-08T12:09:41.654728+10:00

Thu, 08 Aug 2024 02:09:41 INFO     Namespace(REPO_CSV='repos.csv', MARKING_CSV='marking-p0.csv', REPORT_FOLDER='reports', repos=None, token_file='/home/ssardina/.ssh/keys/gh-token-ssardina.txt', start=0, end=10)
Thu, 08 Aug 2024 02:09:41 INFO     Processing repo 1/11: ssardina (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ssardina)...
Thu, 08 Aug 2024 02:09:41 ERROR          Repo RMIT-COSC1127-1125-AI24/p0-warmup-ssardina not found in marking-p0.csv.
Thu, 08 Aug 2024 02:09:41 INFO     Processing repo 2/11: msardina (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-msardina)...
Thu, 08 Aug 2024 02:09:41 ERROR          Repo RMIT-COSC1127-1125-AI24/p0-warmup-msardina not found in marking-p0.csv.
Thu, 08 Aug 2024 02:09:41 INFO     Processing repo 3/11: k-z-z (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-k-z-z)...
Thu, 08 Aug 2024 02:09:45 INFO     Processing repo 4/11: s3897720 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-s3897720)...
Thu, 08 Aug 2024 02:09:49 INFO     Processing repo 5/11: r-jinkies (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-R-Jinkies)...
Thu, 08 Aug 2024 02:09:53 INFO     Processing repo 6/11: zahoorleghari1 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-ZahoorLeghari1)...
Thu, 08 Aug 2024 02:09:57 INFO     Processing repo 7/11: salmaan1234 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-salmaan1234)...
Thu, 08 Aug 2024 02:10:01 INFO     Processing repo 8/11: lukegasbarro (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-lukegasbarro)...
Thu, 08 Aug 2024 02:10:05 INFO     Processing repo 9/11: s3976304 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-s3976304)...
Thu, 08 Aug 2024 02:10:09 INFO     Processing repo 10/11: bryanpham132 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-bryanpham132)...
Thu, 08 Aug 2024 02:10:14 INFO     Processing repo 11/11: asoftrac5 (https://github.com/RMIT-COSC1127-1125-AI24/p0-warmup-asoftrac5)...
Thu, 08 Aug 2024 02:10:18 INFO     Finished! Total repos: 11 - Errors: 2.
Thu, 08 Aug 2024 02:10:18 INFO     Repos with errors written to errors_pr.csv.

See the 2 errors were repo ssardina and msardina bc they do not have marking! All good.


missing per-user profile

Describe the bug
after reinstalling NixOS there is no per-user profile for my user
> echo $NIX_PATH
/home/evils/.nix-defexpr/channels:nixpkgs=/nix/var/nix/profiles/per-user/root/channels/nixos:nixos-config=/etc/nixos/configuration.nix:/nix/var/nix/profiles/per-user/root/channels
> nix-env -iA nixpkgs.hello
installing 'hello-2.12.1'
error: opening lock file '/nix/var/nix/profiles/per-user/evils/profile.lock': No such file or directory
> echo $?
1
> ls /nix/var/nix/profiles/per-user/
root
> whoami
evils
>

Steps To Reproduce
Steps to reproduce the behavior:

have very weird issue with nix run causing a runaway disk usage again
reboot and get completely locked out
reinstall NixOS with existing config and home partition
boot into reinstalled NixOS and attempt to install home-manager
HM channel isn't found, passing it in manually fails with profile.lock: No such file or directory (as shown above)

Expected behavior
it just works
Screenshots
see terminal output in description
Additional context
my config
my nixpkgs branch
Notify maintainers
is there a top-level maintainer for NixOS or the user profiles?
Metadata
- system: "x86_64-linux"
 - host os: Linux 6.6.0, NixOS, 23.11 (Tapir), 23.11.git.3c92ce3f6b55
 - multi-user?: yes
 - sandbox: yes
 - version: nix-env (Nix) 2.17.1
 - channels(root): "musnix, nixos"
 - nixpkgs: /home/evils/.nix-defexpr/channels/nixpkgs

rm ~/.nix-profile fixed the nix-env -iA hello example
and after another nix-channel --update home-manager finds its channel and installs successfully


pypy support

This probably needs a little more thought before we decide if we want to land it, but we can update this branch periodically for Travis pypy builds or whatever.

There are some bulk send tests that will probably fail on pypy (due to errors logged in Deferred.__del__()) until praekelt/vumi#704 lands.

:+1:

Bleh. The PYPY_GC_MAX_PINNED envvar was only added in 2.6.0, so it doesn't help here. :-(

Closed in favour of #1327.


Orion Bluetooth Hub and Bluetooth Locks report as unavailable in HA unless recently interacted with in Tuya app.

My Orion Bluetooth Hub and 2 x Orion Bluetooth Locks are unavailable until I interact with them using the Tuya app. Then, they become available and controllable in HA, but after a minute or so, revert to Unavailable again.
I've been experiencing the Unavailable issue for the past couple of versions. I only noticed today that interacting with the Tuya app somehow temporarily reawakened the connection to HA.
I've attached a debug log that capture a full startup, with the devices being reported offline. I then interacted with the hub and both locks in the Tuya app, and both became available in HA again, only to drop off shortly thereafter.
tuya_local.txt.zip

I decided to scrap / delete it all and build again. This time I forced the protocol version of the Hub and Locks to 3.4. Everything has been steadily holding since.


A better SponsorBlock

Please follow the guide below

You will be asked some questions, please read them carefully and answer honestly
Put an x into all the boxes [ ] relevant to your pull request (like that [x])
Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

[x] At least skimmed through adding new extractor tutorial and youtube-dl coding conventions sections
[x] Searched the bugtracker for similar pull requests
[x] Checked the code with flake8

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

[x] I am the original author of this code and I am willing to release it under Unlicense
[ ] I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

Changes are similar in purpose to SponSkrub, but the implementation is completely different.
What is the purpose of your pull request?

[ ] Bug fix
[x] Improvement
[ ] New extractor
[ ] New feature

Description of your pull request and other information
This is a complete replacement of SponSkrub. New features:

Proper cut implemented using concat FFmpeg demuxer, mentioned in https://github.com/faissaloo/SponSkrub/issues/32. According to https://sattlers.org/2019/09/30/ffmpeg-split-and-cut-video-segments/, small stutter/frame skipping mentioned in the issue can be fixed by forcing key frames, an option for which is also provided.
Implementation takes videoDuration from SponsorBlock responses into account.
Implementation is compatible with --skip-chapters.
Different chapter merge algorithm, which does not lead to "Chapter end time X before chapter start Y" reported in https://github.com/faissaloo/SponSkrub/issues/27.
SRT, ASS, and VTT subtitles (seems like FFmpeg does not fully support others) are cut along with the video, solving https://github.com/faissaloo/SponSkrub/issues/21.
Fully cross-platform (SponSkrub still doesn't have macOS binaries).
Requires no external dependencies.
Written in Python, not in D, which makes contributing much easier.

I was also planning to rewrite the sponskrub integration. However, my plan was quite different from your implementation. Here is what I was thinking:

Don't touch sponskrub. Implement this independantly
Implement chapter merger as a utility and sponsorblock API to youtube extractor itself. This automatically takes care of the chapter mode. User can just do --add-metadata --use-sponsorblock. This also makes it so that the sponsorblock data is available in the info-json if needed
Write a concat postprocessor since it has other uses as well. See https://github.com/ytdl-org/youtube-dl/pull/28153 and related issues
Add an option --remove-chapters that internally uses SplitChaptersPP and the concatPP to remove any chapter from the video. Now, using --remove-chapters sponsorblock.+ will take care of the sponskrub cut mode. This option automatically also allows user to specify the categories to be cut

Doing it this way would provide us with a lot more flexibility imo. Let me know what you think of this appoach

Add an option --remove-chapters that internally uses SplitChaptersPP and the concatPP to remove any chapter

Looking at your code in more detail, using your method of inpoint/outpoint is indeed a better approach than splitting and then concatting

Should I bring the entire SponSkrub implementation back, disable it by default and hide its options from help, or just create aliases --sponskrub -> --sponsorblock etc.?

Looks like SponsorBlock has plans to support other sites; server-side already accepts service query parameter, so moving SponsorBlock to YouTube extractor might not be a good idea.
I'll look at updating info-json.

Concat is a very raw operation; I'm afraid, a generic FFmpegConcatPostProcessor will have to handle SponsorBlock, handle playlists, and over time maybe other operations, which have only concatenation in common, making it difficult to maintain.
I can, however, move concat code to the base class FFmpegPostProcessor, so that if someone is going to implement ConcatPlaylistPostProcessor, he can reuse the implementation.

--remove-chapters sponsorblock.+ is not exactly what cut does. If there is an already existing Chapter with a sponsor in it, SponsorBlock will create chapters Chapter - 1, Sponsor, Chapter - 2. Cut however will simply decrease Chapter's duration, without splitting it into Chapter - 1, Chapter - 2, which --remove-chapters sponsorblock.+ implies.
Specifying categories to cut will take some time to implement, chapter merge algorithms need to be updated to handle it. Handling categories to cut would make removing arbitrary chapters possible, but to implement --remove-chapters, I would rather suggest to extract ChaptersPP from FFmpegMetadataPP (SponsorBlock already has to pass flags into it), and moving SponsorBlock code into ChaptersPP to have all chapter handling code in one place. What do you think about it?

Yes, leave sponskrub as-is for now. We'll worry about how to document it once this new implementation is complete

Looks like SponsorBlock has plans to support other sites; server-side already accepts service query parameter, so moving SponsorBlock to YouTube extractor might not be a good idea.

In that case, we should split the PP into SponsorBlockPP and RemoveChaptersPP. Then call the SponsorBlockPP as a pre-processor (like MetadataFromFieldPP)

Concat is a very raw operation; I'm afraid, a generic FFmpegConcatPostProcessor will have to handle SponsorBlock, handle playlists, and over time maybe other operations, which have only concatenation in common, making it difficult to maintain.

I am not familiar enough with ffmpeg to comment on this. I'll trust your judgement on this for now. If need be, we can generalize it later

I can, however, move concat code to the base class FFmpegPostProcessor, so that if someone is going to implement ConcatPlaylistPostProcessor, he can reuse the implementation.

Yes, please do so if possible

--remove-chapters sponsorblock.+ is not exactly what cut does. If there is an already existing Chapter with a sponsor in it, SponsorBlock will create chapters Chapter - 1, Sponsor, Chapter - 2. Cut however will simply decrease Chapter's duration, without splitting it into Chapter - 1, Chapter - 2, which --remove-chapters sponsorblock.+ implies.

It is though. Take this example: the video has chapters A, B with sponsor sections in the middle of A. The user calls yt-dlp with  something like --sponsorblock --remove-chapters "sponsorblock-.+". So SponsorBlock preprocessor will first call the API and merge the chapter data to give A-1, sponsorblock-sponsor, A-2, B. This is saved in the infojson. Now RemoveChaptersPP will run and remove the chapters that match the regex causing the video to end up with the chapters A-1, A-2, B. Ofcourse, unless the user provides --add-metadata, this chapter info is not written to file

Specifying categories to cut will take some time to implement, chapter merge algorithms need to be updated to handle it.

I don't understand. Nothing needs to change in the chapter merger. It is simply that when we cut sections out, we cut off only the chapters matching the regex given by user.

I would rather suggest to extract ChaptersPP from FFmpegMetadataPP (SponsorBlock already has to pass flags into it), and moving SponsorBlock code into ChaptersPP to have all chapter handling code in one place

If needed, we could seperate the chapter code into its own function. However, this is not necessary for this PR. Once we put the modified chapter data in the infodict using SponsorBlock pre-processor, FFmpegMetadataPP will automatically take care of adding these chapters

Yes, leave sponskrub as-is for now.

Ok, I'll bring it back, and move to Deprecated Options.

Yes, please do so if possible

Ok, I'll do.

It is though.

SponSkrub does in fact implement the cut the way you just described, but the current implementation is different. SponSkrub implementation is easier from the developer perspective,  but it is not necessarily better from the user perspective.
Please take a look at test_merge_with_sponsor_chapters_SponsorsWithinChapter. expected_no_cut has the original chapter split: ['c - 1', 's1', 'c - 2', 's2', 'c - 3', 's3', 'c - 4'], but expected_cut does not: ['c'], only the duration of the chapter has been reduced from 7 to 4.
I would rather want to have that feature present, I don't like to see chapters split for no reason. Please tell me if you dislike that particular feature of cut.

Nothing needs to change in the chapter merger.

Please take a look at Helper.append() and Helper.handle_intersection() inside of _merge_with_sponsor_chapters. Currently its implementation depends on the global self._cut. To leave some chapters and cut out others, the flag must be made per chapter.

In that case, we should split the PP into SponsorBlockPP and RemoveChaptersPP.
Once we put the modified chapter data in the infodict using SponsorBlock pre-processor, FFmpegMetadataPP will automatically take care of adding these chapters

FFmpegMetadataPP does in fact write chapter metadata even in this implementation. The only changes there were made to support writing chapters when user requested --sponsorblock, but did not request either --sponsorblock-cut or --append-metadata (you need to hide whitespace changes to see it, most changed lines are due to indentation). I think it's better from UX perspective to make --sponsorblock imply --add-chapters, instead of forcing people to specify them both. It also matches SponSkrub's implementation. What do you think about it?
However, it is not the reason why I suggested to extract ChaptersPP. These are the parts of the implementation:

Modifying chapters object by either splicing sponsor chapters in or by cutting them out.
Cut is implemented in a way described above: no unnecessary splits.
A new feature that would allow cutting arbitrary chapters, not only sponsors.
Cutting the video itself and the subtitles.
Writing chapter metadata.

Let's look at ways to combine these pieces together.
SponsorBlockPP only queries the API, modifies chapters taking only sponsors into account, and cuts sponsors out of the video. Then RemoveChaptersPP has to modify chapters the second time, and cut the video the second time. Chapter modification code has to be extracted to make it available to RemoveChaptersPP. FFmpegMetadataPP writes chapters to the file as it currently does.
This design tries to separate concerns. The separation of concern comes at the cost of having to modify chapters twice, and having to cut the video twice. At first, that double cut seems avoidable by having SponsorBlockPP pass some info['__sponsor_segments'] down to RemoveChaptersPP instead of doing the work itself. Now RemoveChaptersPP has to pay attention to SponsorBlock parameters to e.g. implement a cut the way it was described above. That passing of information between post-processors created a tight coupling between them nullifying the separation of concerns that the design tried to achieve in the first place.
Alternative design is to have ChaptersPP handling both --remove-chapters and SponsorBlock. It contains all the code to splice and cut the chapters (which by necessity implies cutting the video and the subtitles). SponsorBlock code now is just a couple of functions that conditionally attach sponsor chapters to the chapters explicitly requested to be removed. Cutting without splitting is possible since all the information is in the same place.
It is still possible to retain chapter writing in FFmpegMetadataPP. But it raises the obvious question: why all chapter handling is in one place, but the actual chapter writing is in another? Moving chapter writing code from FFmpegMetadataPP seems like a natural thing to do. It also makes it possible to have --sponsorblock imply --add-chapters without the necessity to pass an ugly info['__sponsor_blocked'] down the line. --add-metadata  of course would still imply running ChaptersPP to match the old behaviour.
All future chapter handling code would have a natural post-processor to tag along to. What do you think about creating ChaptersPP in a way described above? Again, nothing changes if chapter writing stays in FFmpegMetadataPP. But having separate post-processors for SponsorBlock and --remove-chapters doesn't look like a good design due to the reasons described above. And having --remove-chapters handled by SponsorBlockPP looks rather strange.

@pukkandan, looks like FFmpegMetadataPP exists in the original fork, so chapter writing code better be left in there to minimize merge conflicts. But as I said previously

nothing changes if chapter writing stays in FFmpegMetadataPP.

On the other hand, are you OK with having ModifyChaptersPP handling both --remove-chapters and SponsorBlock? Creating two tightly coupled post-processors having to communicate through info[__sponsorblock_segments] looks like bad design, they better be in the same place to maximize cohesion. And without that communication, implementation of non-splitting cut is impossible.

@nihil-admirari Before I answer your queries, I need a few things clarified. I think it would be easier to discuss this in real time. If you have a discord account, could you hop on to the yt-dlp server

Sorry, I don't have a Discord account. Is it OK, if, first, I finish the algorithm rewrite, and then, if you still have any questions, I may try to join Discord?

Small changes:

Brought SponSkrub back. Its options are hidden from the README, but everything else stays the same. New implementation is disabled by default.
Post-processor updates info-json after modifying chapters.
Concat implementation is moved to the superclass and is now available to every FFmpeg post-processor.
Minimized changes in FFmpegMetadataPP. Still --sponsorblock implies adding chapters even if --add-metadata wasn't explicitly requested.
Renamed plaintext to reveal_video_id.
HTTP now must be explicitly requested.
Since Python 2 has been deprecated, the code now uses Python >= 3.6 features: raise ... from ..., nonlocal, format strings, response encoding for JSON data and type annotations. encodeFilename is no longer used, io.open replaced with plain open.

Big change: implementing --remove-chapters and --sponsorblock-cut-categories turned out to be rather difficult. The algorithm was completely rewritten and is now based on a priority queue.
Please review.

huh, I thought I had already replied to your earlier comment. I must have typed it up and then forgot to submit 😢 Give me some time to collect my thought again on the various points. Then I will review

README.md update line from Makefile
generate an enormous diff. Is that line correct?

Yes. The big diff is caused due to the new option names being too long. Anyway, you should revert that and only create the corresponding options for now. We can make README.md once we have finalized everything else

Reverted changes in README + some small fixes.
Workflow checks failed due to
2021-06-07T09:41:51.6758889Z AssertionError: None is not true : Regexp didn't match: '(?:(?:#.*?|\\s*)\\n)*from __future__ import (?:[a-z_]+,\\s*)*unicode_literals' not found, unicode_literals import  missing in /home/runner/work/yt-dlp/yt-dlp/yt_dlp/postprocessor/modify_chapters.py

I've added unicode_literals import, but I'm not sure that test has any uses now, after all, Python 2 has been deprecated.

I've added unicode_literals import, but I'm not sure that test has any uses now, after all, Python 2 has been deprecated.

Yes it has no use, I just forgot to disable that test

Fixed lint error.
Python 3.4 on Windows fails with SyntaxErrors, which is expected considering that the code uses 3.6 format strings. Since only Python 3.6 will be supported in the future, can you please just retire Python 3.4?

You can ignore the 3.4 test. By the time this code is merged, it will be removed

@nihil-admirari  hey there, not sure if im jumping the gun on giving this a test, but when i try --sponsorblock --sponsorblock-cut-categories sponsor for https://www.youtube.com/watch?v=Dy312cUHumk i get a ERROR: Postprocessing: Conversion failed! and the video playback ends at the first sponsor block.
When i didnt cut i had this warning thrown Some SponsorBlock segments overlap incase that helps hint in a direction. i would try to debug it but
also on my quick usage i wonder if another flag of say --sponsorblock-cut should be added which will just cut what is queried so by default that would be all categories, hell id argue we should cut by default if --sponsorblock is provided, but i assume this is due to how sponskrub defaults were.

also on my quick usage i wonder if another flag of say --sponsorblock-cut should be added which will just cut what is queried so by default that would be all categories

I shortened command line options: dropped -categories from --sponsorblock-query/cut-categories, and added a shortcut all to select everything. Is it better now?

... when i try --sponsorblock --sponsorblock-cut-categories sponsor for https://www.youtube.com/watch?v=Dy312cUHumk i get a ERROR: Postprocessing: Conversion failed! and the video playback ends at the first sponsor block.

Please always provide -v logs in such cases, otherwise I'd have to guess what formats you've been trying to download. MP4 format -bv*[ext=mp4]+ba[ext=m4a] worked fine for me.
WEBM format bv*+ba threw:
Application provided invalid, non monotonically increasing dts to muxer in stream 0: 43417 >= 43410

It looks like a rather popular problem. https://ipcamtalk.com/threads/fix-non-monotonous-dts-errors-in-ffmpeg.30465/#post-286226 suggested to add -copytb 1, which I did. Unfortunately it didn't work. An additional -strict experimental didn't work either, and FFmpeg documentation warns against using it anyway:

Note: experimental decoders can pose a security risk, do not use this for decoding untrusted input.

Tested with the latest FFmpeg master from https://johnvansickle.com/ffmpeg/.
Unless someone knows a magic workaround, nothing can be done about it. You can try to open an issue in FFmpeg bug tracker, but it looks like there is a plenty of such bug reports already. I can only suggest you to switch to MP4, for which cutting does work.

@nihil-admirari  thanks for the additions all should do the job for what i was suggesting but atm it throws invalid SponsorBlock category: all --sponsorblock --sponsorblock-cut all, assume you will need to skip the category check if all is provided
Sorry! i didnt even realize there was a -v i should have looked harder, will in the future 👍 .
sorry, ive jsut come from the base yt-dl and my format settings were not working with yt-dlp, so i was just using whatever the default detected was. i can confirm it removes the sponsor as expected with the format you suggested now and with almost no overhead to the processing 😄 good job! once the all category issue is resolved i will give it a test with subtitles also.
Though it should be noted that others may run into similar issues as i did, maybe a note in the readme for sponsor block should be added warning about format issues once that is done? though like you've found it looks more like an ffmpeg issue than an issue with your post processor

Application provided invalid, non monotonically increasing dts to muxer in stream 0: 43417 >= 43410

This issue does not exist in ffmpeg versions <= 3.0.1. See https://github.com/ytdl-org/youtube-dl/issues/28042

atm it throws invalid SponsorBlock category: all

Have you pulled the latest changes from this branch? Caveat: because of force push (rebase on master), you may need to remove you local branch, then pull, and then checkout this branch again.

When i didnt cut i had this warning thrown Some SponsorBlock segments overlap

Actually, it was supposed to warn even when doing a cut. Fixed.

should be added warning about format issues once that is done

Added a warning.
I tried various FFmpeg options that were mentioned as workarounds in various places.

+discardcorrupt, +genpts, +igndts fail with error.
-bsf setts=dts=PREV_INDTS/PREV_OUTDTS and various other combinations fail too.
-bsf vp9_superframe/vp9_superframe_split/vp9_raw_reorder all fail.
-vsync drop proceeds to completion, but the resulting video is broken: VLC only shows static frames that replace each other infrequently.
+noparse +nofillin doubles the file size, but the resulting video is pure black screen.
Forced re-encoding with --remove-chapters-force-keyframes works (at least on the smallest video -S +size), but is terribly slow.

@nihil-admirari not sure what happened as i had the new flags, in any case all now works, tested one with subtitles looks to keep in sync. from my few test runs all looks good to me 👍

we should cut by default if --sponsorblock is provided, but i assume this is due to how sponskrub defaults were.

Indeed, cutting was disabled by default to match SponSkrub behaviour. It made sense for SponSkrub, which implements cut by re-encoding. In this implementation cutting is fast, and, I presume, the majority of people who use SponsorBlock would like to cut rather than contemplate the chapters. Changed to cut by default.

@phyzical, if you still feel like testing, you can find patched FFmpeg here: https://github.com/nihil-admirari/FFmpeg-With-VP9-Timestamp-Fix/releases. It should handle VP9 videos with corrupted timestamps.

@nihil-admirari no worries ill give it a crack later today after work 👍

i'm not actually sure how to build the patched ffmpeg, when i try the build script it just kept failing #9 1.936 + shopt -s globstar #9 1.937 /bin/sh: ./build.sh: line 3: shopt: not found

@phyzical, you don't need to build anything, releases' page already has the binaries. ffmpeg-N-* are builds of FFmpeg master, ffmpeg-n4.4-20-* are builds of the latest release branch. There are win32, win64, and linux64 binaries for each variant. Sorry, if my previous comment mislead you.

@nihil-admirari thanks for the guidance, but i must still be a bit too ignorant, when i try to use the linux binary i get ERROR: Preprocessing: ffmpeg not found. Please install or provide the path using --ffmpeg-location. to note im trying on an alpine based docker image.
i am copying across the binary to /tmp/ffmpeg then providing it via  --ffmpeg-location /tmp/ffmpeg to yt-dl command but seems to have no affect?

@phyzical, a couple of questions:

In --ffmpeg-location /tmp/ffmpeg is /tmp/ffmpeg a directory containing ffmpeg and ffprobe (you need that one too), or the ffmpeg binary itself? It must be a directory containing binaries.
Just to make sure: have executable bit been preserved when copying to /tmp?
Just to make sure: is /tmp mounted exec?
Can you try a different location? Mine ffmpeg is in --ffmpeg-location /opt/ffmpeg-n4.4-20-g2131bc585c-linux64-gpl-4.4/bin/.

If still nothing works:

to note im trying on an alpine based docker image.

Alpine uses musl. Provided binaries should be statically linked against glibc and should work on musl distros, but just in case they don't, could you please retest with a glibc-based distro?

@nihil-admirari
thanks for the reply,

ah yep i was only specifying the ffmpeg directly, but still didnt change the result sadly
yeah 100% set

yeah its on there

tried /opt/ffmpeg still no luck

But when i tried an ubuntu image sure enough it works, so something with the alpine image was going weird
And can confirm the custom ffmpeg fixes the issue with default formats when overlapping segments

can confirm the custom ffmpeg fixes the issue with default formats

That's great!

But when i tried an ubuntu image sure enough it works, so something with the alpine image was going weird

Here's FFmpeg-Builds author's comments (emphasis mine):

I've been looking into that the last couple days actually, and it's surprisingly painful.
Main issue is glibcs inability to actually be linked statically, so you end up with static builds that only work on super bleeding edge systems that use the same or newer of a glibc than your build system.

Alpine's wiki actually has a glibc entry. The only sane option (messing with chroot is pointless when you can simply use a different Docker image) is to try to install a compatibility layer: apk add gcompat. If it still does not work, the only option is to ask FFmpeg-Builds to provide musl-linked binaries: I've no idea how to actually build FFmpeg, my fork is just a trivial patch applied to what is provided by the upstream.

@pukkandan, do you have any questions about this pull request? Can it be merged?

Added support for preview category (see https://github.com/ajayyy/SponsorBlock/issues/444).
Removed copytb from concat options: it was useless in fixing FFmpeg DTS issue anyway.
To avoid resolving an enormous merge conflict in ffmpeg.py multiple times, all commits have been squashed into one.

hey, sorry that this has been delayed so much. So many other issues have been popping up recently and finalizing this is quite a big time commitment on my part.
I promise I'll make a priority to get this merged as soon as I am done with the upcoming release (a couple days at most)
thanks for sticking around despite everything

so I was just curious if any of the changes here will allow for auto-merging playlists like mtv, vh1, etc.?

no, not directly. This does implement a concat demuxer which we can use to implement that functionality

Awesome, that's what I thought when I saw the ffmpeg postprocessor changes. Cool, thank you for the information. :-)

But not having consistent names for the chapters makes this use-case impossible. This issue exists for the overlap handling as well

Sponsor chapters have [SponsorBlock]: prefix that never changes. Is it enough to set up a skip, or the part after the colon also matters?

Both in potplayer, and mkv's chapter-skip, the exact chapter name should be given. Not sure how it is in other players

Overlapped segments result in a large chapter with a combined title: Intro/Sponsor/Self-Promotion etc, even if overlap is small. Tiny chapters can be created only by cuts. They can be ignored by changing append_chapter slightly.

Oh, seems I misunderstood how the overlaps were being handled. But this is even worse imo
Take this example:
00-20 = A
05-10 = B
25-35 = C
30-50 = D
49-60 = E

What current implementation gives:
00-20 = A/B
25-60 = C/D/E

This is throwing away a lot of details, which is bad, especially since adjacent chapters are always supposed to overlap slightly. If we want to avoid any information loss, it should instead be:
00-05 = A
05-10 = A/B
10-20 = A
25-30 = C
30-35 = C/D
35-49 = D
49-50 = D/E
50-60 = E

(this is what I thought u were doing)
Mainly we have to think of 3 kinds of overlap.

The slight overlap that is expected b/w adjacent chapters (D/E) - For this, having a seperate D/E chapter is undesirable and the overlap section should be considered part of one or the other
A significant overlap b/w adjacent chapters (C/D) - If the data in the API was submittted correctly, this should never happen (except for non-music). However, this does in fact sometimes happen. In this case, either C/D or just either C or D are reasonable. If not using using C/D, and the chapters differ significantly in length, it makes intuitive sense to use the name of the shorter chapter
A chapter inside another (A/B) - For this, the inner chapter can be named either A/B or just B

A simple system that can all these situations is to simply consider overlaps as part of the smaller chapter. With that, the results would be:
00-05 = A
05-10 = B
10-20 = A
25-35 = C
35-49 = D
49-60 = E

Another method could be to only do this if the overlap is small (say <5sec) and use the combined chapter names for the rest. That would look like:
00-05 = A
05-10 = A/B
10-20 = A
25-30 = C
30-35 = C/D
35-49 = D
50-60 = E

This still does however have the issue with skipping in players that I mentioned

@nihil-admirari

Alteration handles it as well: CASE|((?i)no-case).

Nope, that doesn't work.
>>> re.search(r'CASE|((?i)no-case)', 'case')
<stdin>:1: DeprecationWarning: Flags not at the start of the expression 'CASE|((?i)no-case)'
<re.Match object; span=(0, 4), match='case'>

Nope, that doesn't work:

My bad, I forgot regex syntax. CASE|(?i:no-case) works.
re.search(r'CASE|(?i:no-case)', 'case')  # Nothing

re.search(r'CASE|(?i:no-case)', 'No-CaSe')
<re.Match object; span=(0, 7), match='No-CaSe'>

Is it really necessary to handle sponsor overlaps differently? Previous implementation required the re-encoding of an entire video, and that's why people used chapter mode, but now sponsors can be removed hassle-free: the primary use case for chapters is gone.

Both in potplayer, and mkv's chapter-skip, the exact chapter name should be given. Not sure how it is in other players

This still does however have the issue with skipping in players that I mentioned

Maybe PotPlayer devs can implement a skip by regex?
It's rather hard to correctly handle overlaps in all cases, differently from the way they are currently handled.

A simple system that can all these situations is to simply consider overlaps as part of the smaller chapter.

More than two sponsors can overlap simultaneously. How ties are to be broken?

Is it really necessary to handle sponsor overlaps differently? Previous implementation required the re-encoding of an entire video, and that's why people used chapter mode, but now sponsors can be removed hassle-free: the primary use case for chapters is gone.

I don't know about others, but the chapter feature of sponskrub was actually requested originally by me because I do not want any destructive operation on the videos I archive. If I want to later see the sponsor section for whatever reason, or if it was marked incorrectly, I want the option to be able to get back the data without redownloading

It's rather hard to correctly handle overlaps in all cases, differently from the way they are currently handled.

A simple system that can all these situations is to simply consider overlaps as part of the smaller chapter.

More than two sponsors can overlap simultaneously. How ties are to be broken?

I know how to implement this easily with multiple passes. I am not entirely sure how to do it in a single pass. Let me have a go at it

Implemented sponsor splits:
00-05 = A
05-10 = A/B
10-20 = A
25-30 = C
30-35 = C/D
35-49 = D
49-50 = D/E
50-60 = E

Tiny chapters (< 1 sec.) are appended to the previous chapter.

This still does however have the issue with skipping in players that I mentioned

Added a template for sponsor chapter names. I don't really understand the machinery of output template, and thus currently it's using str.format.

Added a template for sponsor chapter names

This was the perfect idea. I have modified it to use output template instead. Then, added code to merge adjacent chapters based on title rather than category. This now satisfies both our uses. By default, no data is lost in the chapter names, but by using --sponsorblock-chapter-title "SponsorBlock: %(name)s", I can get the chapters to use only the title of the smallest category
I've also made a few small changes. Have a look at the commit messages
If you are satisfied with how it is now, I believe this can be merged

If you are satisfied with how it is now, I believe this can be merged

I made some minor fixes. I'm OK with merging this, though I don't quite understand why the smallest sponsor contributing to overlap is the most representative of the overlap itself.

@nihil-admirari Thanks for sticking with me till the end of the PR even though it took some large delays and quite a few detours to get this done 😄


Rename instance label to aws_resource

The instance label typically stores an ARN or resource ID for a resource being monitored. This poses a problem when using the prometheus exporter with prometheus, because Prometheus automatically adds the instance label to metrics in a way that is particularly difficult to work around.
This PR renames the instance label to aws_resource, which should more accurately describe the label's purpose, and not collide with Prometheus' instance label.

Hmm... I think prometheus might automatically rename existing instance label to exported_instance and it's already being used in some of the dashboards:

https://github.com/search?q=repo%3Agravitational%2Faws-quota-checker exported_instance&type=code

https://github.com/prometheus/prometheus/blob/6683895620ac5d6e20b58ccbff8b20604e55881a/docs/configuration/configuration.md?plain=1#L201-L204

I think prometheus might automatically rename existing instance label to exported_instance and it's already being used in some of the dashboards

It depends on how this is deployed, and whether honor_labels is true or false (as you linked). IMO it's a better approach to not collide with a well-known Prometheus label, even if there is a workaround.
The Helm chart PR fixes this in the dashboard (tested and confirmed working), but I'd be glad to pull that change into this PR if you like, if this approach works for you.


Issue writing Dataframe to SQL Server Full+

I've seen many developers post about incredible slowness when writing pandas dataframe to a SQL Server table. Some people labeled the issue "chunk size doesn't work" or "data incompatibility slowness" and what not. After a lengthy research I found the following explanation from SQLAlchemy:
http://docs.sqlalchemy.org/en/rel_0_8/faq.html#i-m-inserting-400-000-rows-with-the-orm-and-it-s-really-slow
Briefly, I believe Dataframe.to_sql() method is using SQLAlchemy ORM which uses the Unit of Work method and ends up performing row maintenance work upon insertion of every row of the dataframe. This issue can be resolved if to_sql uses SQLAlchemy CORE instead (link above). Is there a workaround for using Dataframe.to_sql() with SQL Server (not lite) with acceptable performance? I'm really trying all I can to avoid exporting the dataframe to a file and import it to SQL with an SSIS package.
Thanks,
Sam

@vadoverde Thanks for looking into this! I also seen some recent complaining about it, but it is a difficult issue as (as far as I thought) could not really do anything about this on the pandas side.
I was in the belief we do use sqlalchemy Core and not ORM (but have to look later in some more detail to verify). You can see the insert code here: https://github.com/pydata/pandas/blob/master/pandas/io/sql.py#L703
Do you want to take a look as well?

Joris, thanks for the quick reply and the reference. You are using sqlalchemy Core and it looks like the Core has performance issues too. I just developed the Core solution suggested by sqlalchemy and the DB write is still slow! It's definitely a sqlalchemy issue.
I'm new to python (3 weeks) I've always worked with Microsoft technology, so I'll try all I can to help get this issue resolved as I needed for the solution I'm working on. I'll give you a status, but if you find any solution please let me know too.
Thanks,
Sam

What would be interesting is that you also compare it with a non-sqlalchemy implementation (so using a connection object of pyodbc (or another driver you are using) and using the execute method of that one instead of using an sqlalchemy engine). So to check if it is a sqlalchemy performance issue, or more an issue with the driver/connection with SQL Server itself.

I developed the following code to prove the issue is not caused by the driver. Not a pymssql issue at the least. The script below that inserts 37,830 records of two integers in 53 seconds when the proposed sqlalchemy Core solution takes 2 minutes and 40 seconds. Dataframe.to_sql() takes over 4 minutes. Here is my script:

import pymssql
from datetime import datetime
import pandas as pd
import numpy as np
#insert Account Relations into SQL table
tStart = datetime.now()
sqlSt = """WITH cte(AccountID, RelatedAccountID) AS
(
SELECT AccountID, RelatedAccountID FROM
(
VALUES
"""
for i in cba_simAccts.index:
if i == cba_simAccts.shape[0] - 1:
sqlSt += "(" + str(cba_simAccts.loc[i,"AccountID"]) + "," + str(int(cba_simAccts.loc[i,"RelatedAccountID"])) + ")"
else:
sqlSt += "(" + str(cba_simAccts.loc[i,"AccountID"]) + "," + str(int(cba_simAccts.loc[i,"RelatedAccountID"])) + ")"
sqlSt += ","
sqlSt += """
) as V(AccountID, RelatedAccountID)
)
INSERT INTO ML.RelAcct_Credit (AccountID, RelatedAccountID)
SELECT AccountID, RelatedAccountID
FROM cte"""
tEnd = datetime.now()
print (str(tEnd-tStart))
tStart = datetime.now()
conn = pymssql.connect(inp.dbServer, inp.dbUser, inp.dbPwd, inp.dbName)
cursor = conn.cursor()
cursor.execute(sqlSt)
conn.commit()
conn.close()
tEnd = datetime.now()
print (str(tEnd-tStart))

The resulting time deltas of this query are:
0:00:13.975398 ==> time needed by my machine to formulate the SQL statement
0:00:52.802280 ==> time needed to compile and execute the statement on the SQL server
cba_simAccts
In [304]: cba_simAccts.shape
Out[304]: (37830, 2)
In [305]: type(cba_simAccts)
Out[305]: pandas.core.frame.DataFrame
In [306]: cba_simAccts.dtypes
Out[306]:
AccountID             int64
RelatedAccountID    int64
dtype: object

Thanks for digging into this. Are you familiar with any of the profiling tools for python? At the very least we should be able to narrow the gap between us an SQLA.

Yes, indeed, thanks a lot for looking into this.
A question about the code you posted above. If I understand it correctly, here you insert data into a table from another table that is already in SQL? (the INSERT INTO ... SELECT ... FROM ... part)
Do you do the same with sqlalchemy? Because at least with pandas to_sql you transfer the data from outside the database into a table.

Sure thing. I haven't done profiling in Python yet. I did some profiling in SQL Server when issues the to_sql() statement and it appeared to me as if every row was being written individually.
Here is the code I used for SQLA.

from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import Table, Column, Integer, String,  create_engine, insert, schema, MetaData, insert
from sqlalchemy.orm import scoped_session, sessionmaker
Base = declarative_base()
DBSession = scoped_session(sessionmaker())
engine = None
def init_sqlalchemy(dbname="mssql+pymssql://username:password@SQLSERVERNAME:1433/DATABASENAME"):
global engine
engine = create_engine(dbname, echo=False)
DBSession.remove()
DBSession.configure(bind=engine, autoflush=False, expire_on_commit=False)
class RelAcct_Credit(Base):
tablename = 'RelAcct_Credit'
table_args = ({'extend_existing': True, 'schema':'ML'})
RelAcct_SKID = Column(Integer, primary_key = True, autoincrement = True)
AccountID = Column(Integer)
RelatedAccountID = Column(Integer)
tStart = datetime.now()
engine.execute(
RelAcct_Credit.table.insert(),
[{"AccountID": int(cba_simAccts.loc[i, 'AccountID']),
"RelatedAccountID": int(cba_simAccts.loc[i, 'RelatedAccountID'])} for i in cba_simAccts.index])
tEnd = datetime.now()
print (str(tEnd-tStart))

Time Delta:
0:02:35.721570

Can you time something in line of the following (will probably not be fully correct, but you get the idea):
conn = pymssql.connect(inp.dbServer, inp.dbUser, inp.dbPwd, inp.dbName)
cursor = conn.cursor()

cursor.executemany(
    "INSERT INTO ML.RelAcct_Credit (AccountID, RelatedAccountID) VALUES (%d %d)",
    [(int(cba_simAccts.loc[i, 'AccountID']), int(cba_simAccts.loc[i, 'RelatedAccountID'])) for i in cba_simAccts.index])

conn.commit()
conn.close()

This is in my view a fairer comparison, as this also transfers the data from python to the database.

Unfortunately, no dice. This query took me 0:14:27.387294 (14 minutes) on a wireless connection. The time for my solution above (with sqlSt) on wireless is 10 seconds slower:
0:00:14.630463
0:01:02.058205
I'm more convinced now that the sqlalchemy execute and executemany are writing data row by row and doing heavy communication back and forth between my machine and the server which is reflected by the drastic decrease in performance over wifi...

@vadoverde I am not fully understanding. Do you mean that the code snippet I showed above took 14 mins? (this is without pandas or sqlalchemy) Because that would be even slower as with sqlalchemy, you said above that this took 2min 35s.
And what are the other timings?

@jorisvandenbossche sorry about the confusion. Here are the details:
The code snippet you showed above took 14 minutes because I was connected to the network through wireless. I just ran your code snippet while hardwired to the network and it took 0:02:36.178616.

And the timing above of 0:02:35.721570 for sqlalchemy, that is with the same network? Because, then , my conclusion would be: it is not an issue with sqlalchemy, but with the speed of the driver (pyodbc, pymssql, ..), the network or sql server itself.

You're right, sqlalchemy is not the problem. pymssql's executemany() is inserting the data row by row. I developed a solution that sends the data to SQL server in XML format and inserts into a SQL table using OPENXML() SQL command and it took
0:00:02.101211.
SQL server though inserts this data in a few milliseconds, so the performance is still not ideal compared to SQL Server but I'm happy with 2 seconds compared 2 minutes 35 seconds that executemany() and to_sql() takes.
Joris, I appreciate all your help getting this issue resolved. I'd be happy to share the XML solution if you'd like.

@vadoverde No problem, and glad you found a solution
For other people it is indeed interesting to post your solution somewhere, maybe also post it at the recent thread on the sqlalchemy's mailing list.
For pandas itself this will be out of scope (I think we want to restrict ourselves to a solution based on sqlalchemy)
Further, maybe also worth raising this at the pymssql mailing list/issue tracker.

@vadoverde We are having similar issues. Is there a way you can share your XML solution?
Thanks,
Max

Max - here is a function I developed to translate the pandas array to XML format and writes it with a SQL statement. The reason it's fast because pymssql sends one XML file to SQL instead of writing tens of thousands of rows each in a separate batch.
https://gist.github.com/vadoverde/84f4d4a4c31416d2dfee

@vadoverde Many thanks indeed!
I haven't used any XML libraries, and it looks likes there are some imports (ET) outside the function. Can you clarify what those are?

@maximilianr - sorry I forgot to add those. Here are all my imports, ET being xml.etree.ElementTree
import pandas as pd
import xml.etree.ElementTree as ET
import xml
from datetime import datetime
import pymssql

@vadoverde Thanks again!

Anytime, hope it helps


[AutoRelease] t2-containerservice-2021-04-07-68775(Do not merge)

https://github.com/Azure/sdk-release-request/issues/1429
approve

/check-enforcer override


Testimonial Alignment Issue on AsyncAPI Website

Description
There is a noticeable misalignment in the testimonials section on the AsyncAPI website. Specifically, Eric Horesnyi's testimonial has excessive spacing between the testimonial text and his image, making it inconsistent with the other testimonials.
Steps to Reproduce

Navigate to the testimonials section of the AsyncAPI website.
Compare the spacing between the testimonial text and image for Eric Horesnyi to the other testimonials.

Expected Behavior
The spacing between the testimonial text and the image should be consistent across all testimonials.
Actual Behavior
Eric Horesnyi's testimonial has significantly more spacing between the testimonial and the image, breaking the visual flow and consistency.
Suggested Fix
Inspect the CSS styling for the testimonials and adjust the margin or padding values for Eric Horesnyi's testimonial to match the others.
Screenshot

Are you willing to work on this issue ?
Yes I am willing to submit a PR!

@akshatnema can you assign it to me?

@reck-coder123 I appreciate your contribution, but I noticed you've resolved the issue I created and was already working on. I kindly request you to delete your pull request. In the future, please consider raising your own issues and working on those instead.

We don't assign issues here. Feel free to create a PR @Pulkitxm