Send the initialized notification on a worker thread

[jmartisk]

Fixes #1730

[sberyozkin]

@jmartisk Interesting that the SSE transport does not emit on a worker pool.

@btison, can you please try the SSE transport ? It 100% worked for me, but may be I was just a bit lucky...

[jmartisk]

Hm I think the legacy HTTP/SSE transport uses an event loop thread too. I'll check it again tomorrow

[jmartisk]

IIUC for the legacy transport it uses an event loop but doesn't block it. We use a RestEasy Reactive client which takes care of it internally. In Streamable HTTP, we directly use vert.x apis so we need to move to a worker thread manually, which is what this PR does.

[geoand]

Thanks!

I would also be nice to have tests for stuff like this at some point

[jmartisk]

There's no API that allows to easily hook into this and see what thread is executing. Is there any more general way for tests how to detect event loop blocking?

[geoand]

Not that I am aware of

[geoand]

Seems like CI is not happy about the new Guardrails @edeandrea. I wonder why CI in the PR was green and it's just failing after the merge...

[jmartisk]

Seems like CI is not happy about the new Guardrails @edeandrea. I wonder why CI in the PR was green and it's just failing after the merge...

Yeah the likely explanation for that is... #1731 was merged after it, but it wasn't rebased after merging the guardrails work. And there is apparently some kind of conflict between these two (not from a git perspective, but from functionality perspective)

[jmartisk]

So the way to avoid these surprises would be to always make sure everything is fully rebased before merging.

[geoand]

That's the thing, it was theoretically rebased...

[jmartisk]

Nope, if you look at the 1.4 upgrade branch from Mario: https://github.com/mariofusco/quarkus-langchain4j/commits/lc4j-140/
Its history doesn't contain the guardrails work. That's the problem, the CI on the 1.4 upgrade PR ran without taking into account the guardrails changes. Had it been rebased, we would've seen the failures right away.

[jmartisk]

Anyway either @mariofusco or @edeandrea should look into how to fix it :)

[sberyozkin]

Thanks for the clarification, @jmartisk , about why it works with the SSE transport

[edeandrea]

The guardrails branch was fully rebased against main before merging. The branch itself built fine on CI and locally. It also contained everything in the 1.4 branch, so something merged after it is causing the issue.

[edeandrea]

Seems like CI is not happy about the new Guardrails @edeandrea. I wonder why CI in the PR was green and it's just failing after the merge...

Yeah the likely explanation for that is... #1731 was merged after it, but it wasn't rebased after merging the guardrails work. And there is apparently some kind of conflict between these two (not from a git perspective, but from functionality perspective)

The 1.4 PR doesn't show guardrails because it was merged before the guardrails PR. Then that created a big mess on the guardrails branch, which I fixed and got everything green again before it was merged.

[geoand]

@mariofusco can you please take a look?

[geoand]

AFAICT, guardrails was the last thing that was merged and IIUC, it was not rebased onto the 1.4.0 change

[geoand]

In any case, this is my fault for not being proactive enough about monitoring these two changes closely enough

[jmartisk]

Right, I swapped the order of these two PRs in my explanation by error (the 1.4 upgrade was first, guardrails second) but the general explanation stays the same - the latter one (guardrails) wasn't fully rebased on top of the previous one (1.4)

@edeandrea already deleted his guardrails branch so it's hard to check, but I'm looking at the logs of the CI (of the guardrails PR) and it says (in the actions/checkout step):

  HEAD is now at 40668ab Merge cbb10d77bbfdfffe356f3817f9c172deee61b024 into eebf266c238eac1d732ed2dc7c0194958099fb2d

And eebf266c238eac1d732ed2dc7c0194958099fb2d is something that came in history BEFORE the 1.4 upgrade, so that confirms my theory.

[jmartisk]

The only way to avoid this is to really require PRs to be 'serializable' in the sense that you have to run the CI AND merge the PR only after it is fully rebased, and after every merge, every open PR has to be rebased and have its CI run again.

[geoand]

Yeah, I know, but I don't want to go down that route unless necessary. This could have easily been prevented had I been more diligent (in my defense, I was dealing with the IBM stuff).

[geoand]

BTW thanks a lot @jmartisk for digging into the git history and the CI logs!

[jmartisk]

It's an interesting experience, so far I'd only theorized that it may happen, but now I finally saw it in practice :)

[geoand]

I have seen it happen in the past, but it's always perplexing and it doesn't happen often (which is why I would like to avoid adding extra process)

[edeandrea]

Does main build successfully?

I know I could look for myself, but I'm in transit for the next several hours and am browsing on my phone.

[geoand]

Does main build successfully?

No, it does not

[edeandrea]

Ok I'll get on it. It might not be until later today/tonight until I can start looking at it. I'm in transit without wifi until I get to my hotel later.

[geoand]

Thanks!

@mariofusco is taking a quick look now, but if it's not trivial, we'll need your input whenever you can check. This isn't time critical, but I do hope to do a 1.2.0.CR1 release sometime tomorrow if possible.

[edeandrea]

@jmartisk / @geoand If I resurrect my guardrails branch and can rebase main onto it, can I open a new PR from that branch? I've never tried that before.

[geoand]

We definitely don't want to rebase main onto anything - the opposite is what is always needed

[edeandrea]

Thanks!

@mariofusco is taking a quick look now, but if it's not trivial, we'll need your input whenever you can check. This isn't time critical, but I do hope to do a 1.2.0.CR1 release sometime tomorrow if possible.

I actually think I can do this easily in a short period of time with some git-foo (all on a phone) :)

[edeandrea]

We definitely don't want to rebase main onto anything - the opposite is what is always needed

What I meant was I took my guardrails branch and rebased main on top of it. No conflicts and everything builds

[geoand]

All I can say is try it and let's see what happens

[edeandrea]

its saying that main and the guardrails branch are the same.

[geoand]

@mariofusco just opened #1738

[edeandrea]

My git history is showing that it contains the 1.4 stuff...

[edeandrea]

Ahh nevermind the merge commit is ahead but the content is before.

[geoand]

Please rebase this onto the latest main

[edeandrea]

Thanks everyone! Sorry for all the hoopla!

[geoand]

Np, thanks for the collaboration everyone!

[quarkus-bot]

Status for workflow Build (on pull request)

This is the status report for running Build (on pull request) on commit 2c1ec0d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.