Update Details property of Recommended Actions API to resolve Swagger Model Validation

…rror
ARM (Control Plane) API Specification Update Pull Request

[!TIP]
Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!

[ ] New resource provider.
[ ] New API version for an existing resource provider. (If API spec is not defined in TypeSpec, the PR should have been created in adherence to OpenAPI specs PR creation guidance).
[ ] Update existing version for a new feature. (This is applicable only when you are revising a private preview API version.)
[ ] Update existing version to fix OpenAPI spec quality issues in S360.
[ ] Convert existing OpenAPI spec to TypeSpec spec (do not combine this with implementing changes for a new API version).
[x] Other, please clarify:

updating one property to resolve a swagger model validation error related to this check: https://github.com/Azure/azure-rest-api-specs/pull/30030/checks?check_run_id=28134711114

Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:

[x] I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data plane related specifications.
[x] I have reviewed following Resource Provider guidelines, including
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.

Additional information

 Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.

Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.

Getting help

First, please carefully read through this PR description, from top to bottom. Please fill out the Purpose of this PR and Due diligence checklist.
If you don't have permissions to remove or add labels to the PR, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories
To understand what you must do next to merge this PR, see the Next Steps to Merge comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.
For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure
and https://aka.ms/ci-fix.
For help with ARM review (PR workflow diagram Step 2), see https://aka.ms/azsdk/pr-arm-review.
If the PR CI checks appear to be stuck in queued state, please add a comment with contents /azp run.
This should result in a new comment denoting a PR validation pipeline has started and the checks should be updated after few minutes.
If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

Swagger ModelValidation is still failing as this PR aimed to solve such errors. Please fix these too.

Swagger ModelValidation is still failing as this PR aimed to solve such errors. Please fix these too.

ModelValidation passing now, the rest of changes are not part of my PR and my code change so they have been reverted.

Approving as these are existing ones


initialize variable stuff.handle

initialize stuff.handle, or function FoundDevice in usb_libusb.c would return immediately.

🤖 boop bop. returnRandomAssFunctoin immediatly


idea for new plugins

hi Giovanni, i don't know how  to contact directly, i have idea for new plugin for spamassassin but without skill to program i ask if you are interested :
a plugin to check url with dns filtering service, like <IP_ADDRESS>
quad 9 reply with NXDOMAIN for every domain in is list and is very effective to block >98% zero day domain, a lot more vs dnsbl. Also other services like dns filter or cisco umbrella maybe useful
Best tool would be to check url with customizable dns, to preserve site wide local resolver
function like this for example : eval:check_uridnsfilter('<IP_ADDRESS>')
Thanks in advance.
Massimo

Hi,
you can write to giovanni at paclan dot it.
Thanks.


CORS error when trying to configure a remote Ollama server

When i try to connect to a remote Ollama server, I get an error as shown below
Access to fetch at 'https://ollama-1/api/tags' from origin 'chrome-extension://gfnkjppnnkcicmmnfpmhmcjplpfdecfn' has been blocked by CORS policy

You'll need to set the OLLAMA_ORIGINS environment variable to chrome-extension://* when starting the Ollama server. Let me know if this resolves your issue.


Any chance to see clang-tidy as well?

This repo hooked me on using pre-commit CI however, I also need clang-tidy (to enforce code style, CamelCase, length of identifiers and such).
The other repositories with C++ hooks don't work with pre-commit CI and after talking with the pre-commit author, there is no way to pull in system packages.

let's continue the discussion here!  https://github.com/ssciwr/clang-format-wheel/issues/32


Improve documentation

add a note on the current status of the API.
clearly say what termdash is in the first few sentences of README.
start a cook-book of compiled code snippets as examples.

Started wiki and clearly marked public API surface.


README_first.ipynb needs updating

From Phase 2 to Phase 3

Update looks good, will double-check We

Yep very nice. Closing as done.


Download progress by downloadId

I want to start the download in one screen and I need the progress listeners on different screen by providing the downloadId. How can I get the download progress listener by downloadId .

@NainalChauhan can you share your code for singleton class please? I have same issue.

@Methdogg I am not using the singleton class.  I switched to other library.

@NainalChauhan which library do you use?

https://github.com/tonyofrancis/Fetch

@NainalChauhan thx


wrangler: fix Darwin build

Motivation for this change
MacOS build is broken
Things done
@Br1ght0ne when you have a moment

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

[ ] NixOS
[X] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
[X] Tested execution of all binary files (usually in ./result/bin/)
[ ] Determined the impact on package closure size (by running nix path-info -S before and after)
[ ] Ensured that relevant documentation is up to date
[X] Fits CONTRIBUTING.md.

Thanks everyone!


adding front end for photos and videos only filter feature

issue: Video library filter #750
what has been done :
have created 2 filter types for filter only videos and only photos
1)photos for filtering only photos
2)videos for filtering only videos

can only merge this pr once this pr is merged as it calls the backend apis created in that pr

@derneuere here is the front end for the backend apis

should i create 2 different prs for photo and video to pass the quality gate for duplication ?


Rainbow blocks fix for Lua (+ other languages with a little work)

This is a draft of a fix to the problem where rainbow-delimiters doesn't catch the same group multiple times. E.g., it couldn't catch elseif and else in Lua before.
I have time to edit and add improvements if you have any suggestions, or we could add it as another strategy if you don't want it to replace the old global?
I have only tested it in Lua and Rust for now (and before getting rainbow blocks to work better than before in other languages we need to edit the corresponding .scm files). But it works exactly as I would expect so far.
To use this it is important that each @container only has at most one @closing, but you can have as many @opening or @intermediate as you want.

Thank you for wanting to help out. Can you please explain how you worked around the issue of matching the same capture multiple times? My understanding is that if you have a query like (foo (bar) @xxx (baz) @xxx) Neovim's Tree-sitter API cannot handle the two @xxx captures and will only give me one. If I recall correctly that is why I have separate @opening, @intermediate and @closing captures.

The problem is not really Neovim from what I can tell. If you play around with the treesitter playground (https://github.com/nvim-treesitter/playground) all the queries captures multiple captures fine.

(Look at the query and lines 13 to 21.)
The problem is really with iter_matches (which is the obvious choice for this algorithm). (This is the problem discussed here: https://github.com/neovim/neovim/pull/17099 )
So I tried rewriting basically the same algorithm but using iter_captures instead of iter_matches, since iter_captures does have the behavior I would expect (unlike iter_matches).
Unlike iter_matches, which captures the @container as a whole and then only can give you one of each capture inside the @container (e.g. only one @intermediate above), iter_captures gets all the captures in the following order:
@container first and then the order the other captures show up in in the code. If there is another @container inside the first @container, then we start going through that. I.e., we go through all captures in the order they show up in the code, but each "large" capture starts with @container whereafter we go through the captures inside the @container.
The algorithm tries to mirror the previous algorithm (depth first search).
(1) We make an empty matches stack and an empty match_record and start going through all the captured nodes based on our queries (on the changed part as before).
(2) If the captures name is @container that means we are starting going through a new container.

If match_record.container is not yet set, then that means that we are not currently inside another @container, so we set match_record.container = node.

If match_record.container is set, then that means that our current node is inside match_record.container and we starting on a new container. So we (temporarily) push match_record to matches so we can pop it back out later when we are done going through the current node. And then we start a new match_record with container equal to the current node.

If the node's capture name is @closing that means that we are done with the current container (this is a new assumption I added!). So we set match_record.closing to be the current node and pop out the previous match_record from matches (if there is one).

If the previous match_record exists and is an ancestor of the current match_record, then we add the current match_record to the children of the previous match_record, which we are still inside (the one we temporarily pushed earlier). And we continue working with the previous match_record which is now the new current match_record since we are still working inside it.

Otherwise, if the previous match_record exists but is not an ancestor of the current match_record, then we are done with the current match_record (since we saw its @closing), so after popping out the previous match_record, we push both the current match_record and the previous match_record to matches (since it was not an ancestor, we are out of it too). [The order we push these doesn't matter.] Finally we create a new empty match_record in this case.

If there is no previous match_record, we cannot be inside another @container, so we just push the current match_record to matches and create a new empty match_record.

If the node's capture name is not @container or @closing, then we add to the corresponding field in match_record (@opening or @intermediate) if such a field exists. Since we haven't seen a @closing in the current @container yet, this will be assigned correctly simply by the order iter_captures goes through the captures.

All of this is really just trying to implement the old algorithm with the restrictions we get from using iter_caputes instead of iter_matches.
If we want this to replace the current global (and maybe local?) strategy, I think it would be a good idea to actually still keep the old code around and just comment it out (this pull request doesn't do this right now). If they ever fix iter_matches so it has the expected behavior upstream, I think the current implementation is nicer than what you see in this pull request. [It doesn't add the extra assumption that all @containers end with a single @closing.]
Another solution could be to keep a better version of iter_matches as part of rainbow-delimiters if you prefer that instead.

This might be an even better example showing that Neovim can catch all the test in the pattern although they all have the same name here.
(Line 13-21 again.)

Ah yes, the cursed issue where everyone who takes it over vanishes. I am familiar with iter_matches, but I did not consider using iter_captures.
I will have to go over your algorithm in detail when I have the time one of these days. But I don't like that @closing has to be a single node, this just kicks the can further down the road. How about this:

there are only two captures @container and @delimiter
As you have stated a new @capture starts a new context
For every @delimiter node check whether it is a child of the current topmost @container on the stack

If it is then keep going
Otherwise we are done with the current @container

This avoids the need for a @closing node altogether, but in return we have to perform this check for every single node. I don't know if this would make much of a performance impact or if it is just a matter of comparing a few numbers. What do you think of my proposal?

If we want this to replace the current global (and maybe local?) strategy, I think it would be a good idea to actually still keep the old code around and just comment it out (this pull request doesn't do this right now).

They would not work if we rename the captures in the queries. We could keep the old names and just treat them the same in the new strategies. But honestly I don't see much of a point in keeping the old strategies around. The algorithm is limited to one function and described in the HACKING file if we need it for posterity.
On a side note, how hard did you find it to get into the code? I rarely get non-trivial contributions, so I have no idea how readable and approachable my code really is.

First a note to the algorithm I wrote above: You can actually avoid doing the check for whether something is an ancestor (where I wrote NOTE above) if you assume that there is only one @closing, since you can just check instead if the previous match record has already been closed or not. I don't know if this will actually speed up things significantly or not, but I thought I would note it for now.

I will have to go over your algorithm in detail when I have the time one of these days. But I don't like that @closing has to be a single node, this just kicks the can further down the road. How about this:

there are only two captures @container and @delimiter

As you have stated a new @capture starts a new context

For every @delimiter node check whether it is a child of the current topmost @container on the stack

If it is then keep going
Otherwise we are done with the current @container

This avoids the need for a @closing node altogether, but in return we have to perform this check for every single node. I don't know if this would make much of a performance impact or if it is just a matter of comparing a few numbers. What do you think of my proposal?

Your suggestion is definitely possible and it shouldn't be that hard to change what I currently have. I can try to have code for both algorithms written so we can test them a bit more (this sounds like something where we might want to actually benchmark the performance of these things, since I also don't really know the implications of checking if something is a parent of a treesitter node or not). With your suggestion we will probably still need the check whether the previous match record is an ancestor of the current match record though. [I think the parent check should be quick, but the ancestor check is probably not as quick.]

If we want this to replace the current global (and maybe local?) strategy, I think it would be a good idea to actually still keep the old code around and just comment it out (this pull request doesn't do this right now).

They would not work if we rename the captures in the queries. We could keep the old names and just treat them the same in the new strategies. But honestly I don't see much of a point in keeping the old strategies around. The algorithm is limited to one function and described in the HACKING file if we need it for posterity.

I will start out by keeping the old capture names around for now, just because we would need to edit all the queries otherwise, and it will also make it easier to compare the two implementations. If we decide on changing the names to @delimiter this will be easy to change later.

On a side note, how hard did you find it to get into the code? I rarely get non-trivial contributions, so I have no idea how readable and approachable my code really is.

I think it was pretty readable, but I mostly looked at the queries and strategies (+ a little at the general implementation). I am pretty new to Lua, so some of the meta tables stuff was a bit confusing, but that probably has more to do with me not being used to Lua meta tables yet than your actual code. Also I have never used Vader, so for testing I have just been manually checking whether it works as expected in my day to day programming + a few of the test files available.
To be a bit more precise, I think your Stack implementation and the strategy code is very easy to follow, but I was a bit confused about how the namespaces worked in lib.lua. After looking at it more, I guess part of it is because vim.api.nvim_create_namespace() doesn't behave as I would have liked. (I would have liked it to be easier to have per-language namespaces that I could search for when looking at Extmarks.)

I have added an alternative update_range function (currently called update_range2) that is easy to change so it works with just @delimiter and @container (all that is needed is changing the names of capture groups in new_match_record and highlight_matches). I have only tested functionality and not performance so far, but it seems to work fine and I can't tell any difference in performance on my computer. Though we should probably do some actual performance tests comparing update_range1 with update_range2.

I have a large local Rust file (5.5MB) that I tried to use to compare the two algorithms.
On my computer with this file, both algorithms seem to highlight ok, and the main part of update_range1 takes about 0.6secs on average while the main part of update_range2 takes about 1.2secs. This doesn't seem like a huge difference, but update_range1 was consistently about twice as fast as update_range2.
Also, it should be noted that this file has almost no nested highlights levels (it is basically all level 2 in one long list), so ts.is_ancestor only needs to do very little work.
In general, it seems that keeping track of @closing tags and ensuring that there is exactly one @closing for every @container will be faster, but for more common size files there is no difference. (On smaller normal size files they both take about 0.01-0.05secs for me.)
Also note that this was timed with os.clock() in Lua, which I can read is not the best for real world timing of algorithms.

There is also a C++ file under test/stress/markdown/lorem-ipsum.md which you can try. It has a number of languages embedded in Markdown, but it might be too short to deliver any meaningful results.
If we want to go with the @closing capture then how about we treat it purely as a sentinel node? By that I mean the content which we want to highlight is captured with @delimiter, but in addition to that we also capture the last delimiter with @closing.  Here is what it would look like for Lua function definitions where the sentinel is also the last delimiter:
(function_declaration
  "function" @delimiter
  "end" @delimiter @closing) @container

Here is a contrived HTML example in which for whatever reason I only want to highlight the name of the closing tag, so the delimiter is tag_name and the sentinel is end_tag:
(element
  (start_tag
    "<" @delimiter
    (tag_name) @delimiter
    ">" @delimiter)
  (end_tag
    (tag_name) @delimiter) @closing) @container

This will require adjusting queries, but the change is straight-forward. I can do that myself, no need to waste your time.

There is also a C++ file under test/stress/markdown/lorem-ipsum.md which you can try. It has a number of languages embedded in Markdown, but it might be too short to deliver any meaningful results.

Both algorithms take 0.00secs in my timing, so it is definitely too short to see anything.  (Though the timing is not that accurate since right now it times separately for each language in the file.)

If we want to go with the @closing capture then how about we treat it purely as a sentinel node? By that I mean the content which we want to highlight is captured with @delimiter, but in addition to that we also capture the last delimiter with @closing. Here is what it would look like for Lua function definitions where the sentinel is also the last delimiter:
(function_declaration
  "function" @delimiter
  "end" @delimiter @closing) @container

That would definitely be a possibility too.

Here is a contrived HTML example in which for whatever reason I only want to highlight the name of the closing tag, so the delimiter is tag_name and the sentinel is end_tag:
(element
  (start_tag
    "<" @delimiter
    (tag_name) @delimiter
    ">" @delimiter)
  (end_tag
    (tag_name) @delimiter) @closing) @container

This will require adjusting queries, but the change is straight-forward. I can do that myself, no need to waste your time.

I can easily edit the code to work like that very quickly if this is what we want to go with. I think this is a good idea since the code with @closing is also simpler (so hopefully easier to maintain), and it makes sense that the capture starts with @container and ends with @closing. And based on my (admittedly very simple) testing it does seem to be faster than calling ts.is_ancestor().

BTW, am not particularly attached to the capture names, so if you have a better suggestions don't hesitate. For example, maybe @sentinel would be better than @closing, because the name @closing makes me think there needs to be a corresponding @opening somewhere.

I think @sentinel does better describe the idea, but it is a pretty uncommon word. Then again, it might not be bad to use unique and descriptive words, so for now let's go with @sentinel.

I have updated the algorithm so it should work with @delimiter and @sentinel as described now. I have checked that it works as I would expect with Lua and Rust so far (and I even managed to speed it up a little [maybe ~10%] by pushing and popping less).

I have tried to update the local strategy in a similar way now too. I think it works as expected, but I only really use the global strategy normally, so I am not entirely sure if it used to behave slightly different in some cases.

I have been testing injected languages in test/stress/markdown/lorem-ipsum.md, but the following lines in the original code seems to cause problems:
https://github.com/HiPhish/rainbow-delimiters.nvim/blob/652345bd1aa333f60c9cbb1259f77155786e5514/lua/rainbow-delimiters/strategy/global.lua#L139C1-L149C8
For now I have commented out those lines and don't see anything going wrong, but I assume that was put there for a reason. @HiPhish do you remember an example where this code is clearly needed?
Also note: I updated the queries for the corresponding languages I needed to test with that markdown file. It seems to work as expected after commenting out the above lines.

Yes, I remember why this exists. Let's say you have a Markdown file with embedded Lua:
Here is some Lua code:

~~~lua
-- This is Lua
print({{{}}})
~~~

Back to Markdown

Now if we move (not delete and put) the Lua code one line down it will carry with it the extmarks it had and we will have Lua delimiter highlighting within Markdown. This is because extmarks move with the text. My solution is to have per-language namespaces and delete all extmarks which do not belong to the current language. In the above example the line has Lua extmarks, so when I move it to the Markdown region all non-Markdown extmarks are removed.

Yes, I remember why this exists. Let's say you have a Markdown file with embedded Lua:
Here is some Lua code:

~~~lua
-- This is Lua
print({{{}}})
~~~

Back to Markdown

Now if we move (not delete and put) the Lua code one line down it will carry with it the extmarks it had and we will have Lua delimiter highlighting within Markdown. This is because extmarks move with the text. My solution is to have per-language namespaces and delete all extmarks which do not belong to the current language. In the above example the line has Lua extmarks, so when I move it to the Markdown region all non-Markdown extmarks are removed.

I see. I am able to reproduce this sometimes (in some languages). But the problems I have while enabling that code is worse.
Without the code, I can get the problem like this:
https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/d7e646cc-55dc-44e6-8015-4d895d1da93f
With the code, I can't update the highlighting as usual (and sometimes the highlighting starts out wrong). If I try to do :e<CR>, then highlighting will go away while visible, and I need to scroll away and do :e<CR> and then scroll back to get it to work.
https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/95a7c4a5-0918-48d2-9b31-35fd8ed8bb4e
I will try to see if I can find a better way to do this that solves both issues.

Yes, I have noticed that the changes can be quite weird at time. The number<PHONE_NUMBER> is the maximum possible 32-bit unsigned integer value. Neovim uses the range (0,<PHONE_NUMBER>) to mean "the entire buffer".
Can you please mark your PR as WIP or something like that until it is ready for review? I really appreciate your effort and I would prefer not to waste either of our time by reviewing code that isn't done yet.

Yes, I have noticed that the changes can be quite weird at time. The number<PHONE_NUMBER> is the maximum possible 32-bit unsigned integer value. Neovim uses the range (0,<PHONE_NUMBER>) to mean "the entire buffer".

Other than that number, I think the weirder thing is that it sometimes sends the same language range multiple times. E.g. in my list above, both markdown lists cover the entire buffer and the vim lists are identical.

Can you please mark your PR as WIP or something like that until it is ready for review? I really appreciate your effort and I would prefer not to waste either of our time by reviewing code that isn't done yet.

I have changed the PR to "Draft" in GitHub for now while I work on this last thing. I don't plan to make more changes to update_range in the global strategy though, so if you want to start looking at the main part of this pull request, that part should be final now.

@HiPhish: I managed to fix all issues I had with moving code, using undo and any other problems I saw now. If you notice any other problems I can try to fix those too, but otherwise I think this code is ready for review now. (I don't plan to change anything unless someone notices a problem.)

Just a quick heads-up, I am in the process of reviewing this PR. Can you please tell me in which order captures are delivered by iter_capture?

I have performed a preliminary code review of some surface level stuff. I still need to understand the algorithm inside update_range of the global strategy. From what I gather there is the stack (matches) the current match record (match_record) that is being built up. We iterate through all the captures and differentiate based on the name of the capture:

delimiter is obvious

container has two cases:

If the current record has no container node, then this is its node
If the current match has been closed it belongs to a sibling match. Push the current match to the stack and start a new current match
Otherwise the container belongs to a new child match. Stash the current record on the stack and start a new record which has the previous one as its parent

sentinel closes the current record

If the current record has no parent push it to the stack and start a new record
Otherwise pop the parent off the stack and set it as the current record

The matches stack serves two purposes: eventually it will be a "list" of top-level records, but until then it serves as a stash for records while we collect their children. Is my understanding so far correct?

Yes, it sounds like you have the right idea.
Looking back at the code, we can change the check for whether there is a current container to whether there is a current match_record. This complicates the stylua types (since we need to consider nil match_records then), but it might be more clear?

Regarding the order of matches, my understanding is that all captures are returned in a depth-first order. So given this HTML snippet:
<div id="d1">
	<div id="d2">
		<div id="d3">
		</div>
	</div>
	<div id="d4">
	</div>
</div>

We would get the captures in this order:

container d1
start d1
container d2
start d2
container  d3
start d3
end d3
sentinel d3
end tag d2
sentinel d2
container d4
start d4
end d4
sentineld4
end d1
sentinel d1

Is this the correct order?

Yes, that is the correct order from my understanding of iter_captures().

I like the variant with match_record = nil better. Then we don't even need the container field in the match record table. I feel there is a really elegant recursive mathematical definition hidden somewhere under that imperative code, but let's get this out of the door as is first or else it will never get done :)
Can you please check the "allow edits from maintainers" checkbox in your PR? I have a few improvements to discuss that I want to push to this PR as a commit. If I can commit it will be more efficient than if I can only tell you what to do.
On a related note, I have played around with the HTML query and I absolutely love the result. Finally I can have the tag name and the angle brackets highlighted, but not the attributes. I have literally wanted this for years, ever since I first learned about rainbow parentheses. Thank you so much for your work!

I like the variant with match_record = nil better. Then we don't even need the container field in the match record table. I feel there is a really elegant recursive mathematical definition hidden somewhere under that imperative code, but let's get this out of the door as is first or else it will never get done :)

I have updated the code so it uses match_record = nil and removed container now.

Can you please check the "allow edits from maintainers" checkbox in your PR? I have a few improvements to discuss that I want to push to this PR as a commit. If I can commit it will be more efficient than if I can only tell you what to do.

From my side it looks like this is already checked. Can you not edit now? I will try to see if I need to click "Yes" to something in some settings somewhere.

On a related note, I have played around with the HTML query and I absolutely love the result. Finally I can have the tag name and the angle brackets highlighted, but not the attributes. I have literally wanted this for years, ever since I first learned about rainbow parentheses. Thank you so much for your work!

I have also liked rainbow brackets for a long time and got used to it from emacs. This is the best plugin for it that I have seen in Neovim, so I am happy that we are able to improve on it too! (I was annoyed by Lua blocks highlighting in stead of html, but I also think that got much better with this update.)

I get ! [remote rejected] Danielkonge/master -> Danielkonge/master (permission denied) when I try to push to your remote (github.com:Danielkonge/rainbow-delimiters.nvim.git) on the master branch. Maybe you should have created a topic branch first?

OK, looks like I figured it out. I had to push the commit as git push Danielkonge-contrib HEAD:master where Danielkonge-contrib is the remote name I have chose for your repo. This will make collaboration much easier.

SQL parentheses look fine to be as long as there are no empty parentheses or parentheses containing only one element.

In a an expression line SELECT (1 + (2)); all the characters inside the outer parentheses are on the same level in the tree.
      term [0, 7] - [0, 16]
        value: "(" [0, 7] - [0, 8]
        value: binary_expression [0, 8] - [0, 15]
          left: literal [0, 8] - [0, 9]
          operator: "+" [0, 10] - [0, 11]
          right: "(" [0, 12] - [0, 13]
          right: literal [0, 13] - [0, 14]
          right: ")" [0, 14] - [0, 15]
        value: ")" [0, 15] - [0, 16]

I don't think there is anything we can do without a container node.

SQL parentheses look fine to be as long as there are no empty parentheses or parentheses containing only one element.

You mean after my update right? Because I was definitely having problems other than with nesting before that.

Yes, after the update.

Also, since writing something like ((())) is very uncommon outside tests like this, I think it might actually make sense to simplify the code a bit and just let it look bad on something like that.

Agreed. In SQLite at least SELECT (((1))); is legal query, but no one is ever going to write code like this.

I updated the local and global strategy so that any capture name that starts with 'ignore' will be ignored (we don't want to log those). So we can use ignore, ignore1, ignore_self_closing, etc. for any captures we want to use for predicates like #eq?, #any-eq?, etc.

I updated the local and global strategy so that any capture name that starts with 'ignore' will be ignored (we don't want to log those). So we can use ignore, ignore1, ignore_self_closing, etc. for any captures we want to use for predicates like #eq?, #any-eq?, etc.

Why not just ignore any capture that has a name other than container, delimiter and sentinel? Currently unknown capture names are logged, but the function will run on every change and spam the logs. If it is about validating queries for query authors, we can do so only once when the query is loaded first like this:
local known_captures = {delimiter = true, container = true, sentinel = true}

local q = vim.treesitter.query.get('lua', 'rainbow-delimiters')
for _, capture in ipairs(q.captures) do
    if not known_captures[capture] or not starts_with_ignore(capture) then
        log.error('Unknown capture name: ' .. capture)
    end
end

This validation can be added somewhere in the lib module, for example when registering a new namespace. Then the validation will only run once over the entire lifetime of the Neovim process.
I would not add validation to the scope of this PR, it is big enough as it is. We can open a new ticket for that.

Why not just ignore any capture that has a name other than container, delimiter and sentinel? Currently unknown capture names are logged, but the function will run on every change and spam the logs. If it is about validating queries for query authors, we can do so only once when the query is loaded first like this:
local known_captures = {delimiter = true, container = true, sentinel = true}

local q = vim.treesitter.query.get('lua', 'rainbow-delimiters')
for _, capture in ipairs(q.captures) do
    if not known_captures[capture] or not starts_with_ignore(capture) then
        log.error('Unknown capture name: ' .. capture)
    end
end

This validation can be added somewhere in the lib module, for example when registering a new namespace. Then the validation will only run once over the entire lifetime of the Neovim process.
I would not add validation to the scope of this PR, it is big enough as it is. We can open a new ticket for that.

I think you are right, so I have removed the unknown capture name cases now. I still have the case where we get a @delimiter or a @sentinel without having a match_record. As long as the queries (and code) are correct we should never end up in this case, so it shouldn't be spamming the logs. Do you want to change this case too?

Do you want to change this case too?

No, please leave it in. This is an error case which cannot be detected until the code actually runs, so having the error in the logs is valuable.

I have noticed a problem with nested languages: when I move a line up out of a nested block it will retain its rainbow highlighting. Here is a Markdown example
Some markdown.
~~~lua
print({{{{}}}})
print({{{{}}}})
~~~
More markdown

I place the cursor on line 3 and execute :move 1. The line will lose its Lua highlighting, but it will retain its rainbow highlighting. However, if I place the cursor on line 4 and execute :move 5 (i.e. downwards) it will lose the rainbow highlighting.

I have noticed a problem with nested languages: when I move a line up out of a nested block it will retain its rainbow highlighting. Here is a Markdown example
Some markdown.
~~~lua
print({{{{}}}})
print({{{{}}}})
~~~
More markdown

I place the cursor on line 3 and execute :move 1. The line will lose its Lua highlighting, but it will retain its rainbow highlighting. However, if I place the cursor on line 4 and execute :move 5 (i.e. downwards) it will lose the rainbow highlighting.

I can reproduce this if I copy exactly your short markdown file there, but it is not a problem in the stress markdown file, so I am not sure what causes this. I will try to look at the changes returned some more.

I think I figured out the problem. It seems to be an off by one error from what I can tell.
Changing
vim.api.nvim_buf_clear_namespace(bufnr, nsid, change[1], change[3])

to
vim.api.nvim_buf_clear_namespace(bufnr, nsid, change[1], change[3] + 1)

fixes the problem.
https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/bfd1df5e-2ba9-4559-ace4-a6e1cb9d2136
But this then breaks html highlighting. I originally had the lower one, but it gives this highlighting:

instead of:

It seems to be a problem with clearing out the old highlights caused by different behavior in the different parsers. I am not yet sure though, so I will try to look into this a bit more.

https://github.com/HiPhish/rainbow-delimiters.nvim/pull/43/commits/a8ff2a79ec95c5894d98135dd2a2bc68f14f25c6
This is a temporary fix that seems to work in almost all cases I have checked. The only way I have managed to break this is by having nested language injections (i.e., a language injection in a language injection), but that seems like a rare need. In those rare cases, I think it makes sense to refresh highlighting (that will fix it) if you move code.

I cleaned up the code a bit and noticed another problem with injections in Rust. When you use macros in Rust, it treats that as a language injection with the language being Rust, but when we inject the language itself, it will already be taken care of by the part of the code without a parent language, so in that case we should do nothing. This has been messing with Rust highlighting of (and around) macros for me for a long time, and the current fix I made finally gets around it.
I have tested out the code some more and haven't noticed any problems now except when you have nested language injections, which I think is pretty rare. In those cases updating highlighting or moving stuff can break the highlighting with the current code. E.g.:
Markdown text.
~~~lua
print 'This is injected Lua in Markdown (one level deep)'
vim.cmd[[
echo str([])
echo 'This is injected Vim in Lua in Markdown (two levels deep)'
]]
~~~

You have the patience of a saint. It will be a day or two before I get around to looking at the code, so I need to ask for a bit more patience.

Instead of ignore<anything> for the captures that we ignore for both debug and highlighting, do you think it would make more sense to use _<anything>? I.e., just ignore stuff starting with underscore. I feel like that might be a bit more expected behavior.

Yes, underscore is better. It is more in line with ignored variables in programming languages and harder to mistype.

In https://github.com/HiPhish/rainbow-delimiters.nvim/pull/43/commits/d4cc189f80dedf327aa2308033823cb515a830d1 I included some comments that I will delete soon, but I made a commit to look back on if needed.
I played around with using vim.api.nvim_get_extmarks and vim.api.nvim_del_extmark instead of vim.api.nvim_buf_clear_namespace since those allow us to work with exact rows and columns instead of just the rows, but I didn't manage to improve the behavior over what we have now (there was still a problem with nested injections), so I didn't switch to those functions in the code.

I just reordered some code to remove one level of if-nesting. Can you please double-check that the conditions are still the same?

I just reordered some code to remove one level of if-nesting. Can you please double-check that the conditions are still the same?

The logic looks correct to me. (Also, it behaves as expected in quick testing of what that code is supposed to solve.)

Where did you get the _parent_lang field of the parser from? It is not documented, and when I inspect a parser object there is no such field either. I used the following Markdown text:
Hello

```c
puts((((("This is an injected language")))));
{
    {
        {
            {
                {
                    return ((((((((((((2)))))))))))) + ((((((3))))));
                }
            }
        }
    }
}
```

Then I set p = vim.treesitter.get_parser() and executed lua =p to view the fields of the parser object. Each parser has a children field which maps contained languages to their respective parsers. But there is no _parent_lang field.

Where did you get the _parent_lang field of the parser from? It is not documented, and when I inspect a parser object there is no such field either. I used the following Markdown text:
Hello

```c
puts((((("This is an injected language")))));
{
    {
        {
            {
                {
                    return ((((((((((((2)))))))))))) + ((((((3))))));
                }
            }
        }
    }
}

Then I set `p = vim.treesitter.get_parser()` and executed `lua =p` to view the fields of the parser object. Each parser has a `children` field which maps contained languages to their respective parsers. But there is no `_parent_lang` field.

EDIT: I don't see any way of finding out whether the parser has a parent language. We can descend deeper into the tree, but not back up.

I found _parent_lang by printing p in on_changedtree and then going through :messages. It seems to be nil (so not shown) when there is no parent language, but it should show up for all injections.

If we want to avoid using _parent_lang and still have movement of text work with injections, I think we need to figure out what causes the wrong changes to be reported in certain cases (like the above c code). If that is fixed, then we could simplify the code a bit and even have it work with nested injections.

Since p._parent_lang seems to be a nightly feature, I have updated util.for_each_child so that we can keep track of the parent language ourselves instead. Everything seems to work as expected in the global strategy now (I haven't made the corresponding changes in the local strategy yet).
Please let me know if there is any problems with this update.

I think I have fixed the problem with nested injections now, but the solution is a bit hacky. I don't think it should affect the performance of the highlighting though, since the main part of the updated code will only be run when we are at least two injections deep.
Please let me know if you notice any problems.
Note: If we don't want to include this last change, the previous code before this change worked for everything other than nested injections in my testing.

I don't think we need the function for_each_child_dif_lang if we move the test into the thunk. I have done so in the previous commit, please check if the logic is the same.

Your changes now work for me as well. One more questions:
What does this mean? Can you please comment it?
				if changes[1] and changes[1][1] then
					update_range(bufnr, changes, tree, lang)
				end

The first test checks that changes is not an empty array, but why the second one? The items of changes are four-element range tables, so how can changes[1][1] be nil?

I think I have fixed the problem with nested injections now, but the solution is a bit hacky. I don't think it should affect the performance of the highlighting though, since the main part of the updated code will only be run when we are at least two injections deep.

I think it's fine, albeit quite ugly because you have to drag information about two languages with you. Maybe I can find a nicer way, but if not I'd leave it as it is. It may be ugly, but it works.

I don't think we need the function for_each_child_dif_lang if we move the test into the thunk. I have done so in the previous commit, please check if the logic is the same.

I was about to say that this might cause problems, but looking at it again it seems that the return just applies to the inner thunk function, so it should work as expected, yes.
Though, to clarify the type of the thunk function I changed it from function(child_parser) to function(child_parser, _, _).

Your changes now work for me as well. One more questions:
What does this mean? Can you please comment it?
				if changes[1] and changes[1][1] then
					update_range(bufnr, changes, tree, lang)
				end

The first test checks that changes is not an empty array, but why the second one? The items of changes are four-element range tables, so how can changes[1][1] be nil?

I don't think there is any reason to check changes[1][1] in the current code, so I removed it.
I think at some point I might have had something like {{}} for changes, which is why I had that check.

I think I have fixed the problem with nested injections now, but the solution is a bit hacky. I don't think it should affect the performance of the highlighting though, since the main part of the updated code will only be run when we are at least two injections deep.

I think it's fine, albeit quite ugly because you have to drag information about two languages with you. Maybe I can find a nicer way, but if not I'd leave it as it is. It may be ugly, but it works.

If we could get an upstream update that reported changes correctly (or at least what I would consider to be the correct way) all the time instead of sometimes reporting the whole injection block for some injected languages, then it should be possible to simplify the code quite a bit.

In this block:
				-- If a line has been moved from another region it will still carry with it
				-- the extmarks from the old region.  We need to clear all extmarks which
				-- do not belong to the current language
				for _, change in ipairs(changes) do
					if change[1] then
						for key, nsid in pairs(lib.nsids) do
							if key ~= lang then
								-- HACK: changes in the main language sometimes need to overwrite
								-- highlighting on one more line
								local line_end = change[3] + (parent_lang and 0 or 1)
								vim.api.nvim_buf_clear_namespace(bufnr, nsid, change[1], line_end)
							end
						end
					end
				end

the check if change[1] then seems superfluous as well to me. Am I missing something?

Let's see if I understand the problem correctly. Let's say I have this Markdown code:
Hello

```c
puts((((("This is an injected language")))));

puts((((("This is an injected language")))));
```

If I place the cursor on line 4 and execute :move 1 I get the following changes:

markdown { { 0, 5, 5, 1, 45, 51 } }
c { { 4, 0, 58, 8, 0, 62 } }

This is fine and exactly what I would expect. Now suppose I place my cursor on line 2 (containing the moved text) and execute :move 4 to move it back into the C block. Then I get the following changes:

markdown { { 0, 5, 5, 1, 0, 6 }, { 3, 0, 12, 9, 0, 108 } }
c { { 3, 0, 12, 8, 0, 62 } }

The first Markdown change encompasses the first two lines, which is fine. However, the second Markdown change encompasses lines 4 to 10, which contain C code. Thus the extmarks in the C block get erased. Then the changes from the C block (line 4 to 9) are applied. But this should restore the extmarks again, should it not?
What is the faulty behaviour you are observing?

Let's see if I understand the problem correctly. Let's say I have this Markdown code:
Hello

```c
puts((((("This is an injected language")))));

puts((((("This is an injected language")))));
```

If I place the cursor on line 4 and execute :move 1 I get the following changes:

markdown { { 0, 5, 5, 1, 45, 51 } }
c { { 4, 0, 58, 8, 0, 62 } }

This is fine and exactly what I would expect. Now suppose I place my cursor on line 2 (containing the moved text) and execute :move 4 to move it back into the C block. Then I get the following changes:

markdown { { 0, 5, 5, 1, 0, 6 }, { 3, 0, 12, 9, 0, 108 } }
c { { 3, 0, 12, 8, 0, 62 } }

The first Markdown change encompasses the first two lines, which is fine. However, the second Markdown change encompasses lines 4 to 10, which contain C code. Thus the extmarks in the C block get erased. Then the changes from the C block (line 4 to 9) are applied. But this should restore the extmarks again, should it not?
What is the faulty behaviour you are observing?

Your changes look fine, but I don't always get changes like that.
In this block in the stress markdown file:
Fata quae: **falsi**: pares dea [agmine
hospite](http://www.quo-retro.net/sublimis) catenas? [Morte](http://corpus.net/)
ad putes. Dicentem Chariclo vidit!

```c
puts("This is an injected language")
{
    {
        {
            {
                {
                    return ((((((2)))))) + ((((3))))
                }
            }
        }
    }
}
```

If I move the line with puts up one line and then down one line again, I get the following changes:
markdown
{ { 42, 0, 1123, 43, 0, 1160 } }
c
{}
markdown
{ { 43, 0, 1128, 55, 0, 1318 } }
c
{ { 43, 0, 1128, 44, 0, 1165 } }

The first markdown and c changes are fine, but the second markdown changes cover the whole c block, so it clears the highlighting there. And then the c changes only (correctly) catches the change on the first line of the C block. I.e., I think the changes reported by the second markdown block is wrong.

In this block:
-- If a line has been moved from another region it will still carry with it
-- the extmarks from the old region.  We need to clear all extmarks which
-- do not belong to the current language
for _, change in ipairs(changes) do
	if change[1] then
		for key, nsid in pairs(lib.nsids) do
			if key ~= lang then
				-- HACK: changes in the main language sometimes need to overwrite
				-- highlighting on one more line
				local line_end = change[3] + (parent_lang and 0 or 1)
				vim.api.nvim_buf_clear_namespace(bufnr, nsid, change[1], line_end)
			end
		end
	end
end

the check if change[1] then seems superfluous as well to me. Am I missing something?

With the current code, I think you are right, I have removed it now.

I can reproduce the issue with the changes. I's say this wraps up things for the global strategy. If you have any changes to make to the local strategy let me know, otherwise I will start reviewing the code there. Once the local strategy is done I think we can finally finish all of this and merge it into master.
In the meantime I will go ahead and document the problem with the changes.  I am not sure if this is really an issue or just a really weird implementation quirk. I mean, highlighting should be running into the same problem as well, but somehow they are avoiding the issue.

I can reproduce the issue with the changes. I's say this wraps up things for the global strategy. If you have any changes to make to the local strategy let me know, otherwise I will start reviewing the code there. Once the local strategy is done I think we can finally finish all of this and merge it into master.
In the meantime I will go ahead and document the problem with the changes. I am not sure if this is really an issue or just a really weird implementation quirk. I mean, highlighting should be running into the same problem as well, but somehow they are avoiding the issue.

I have already updated the local strategy so it builds the match tree similarly to how the global strategy works, and based on my testing it works as expected now. I think it should be ready for review. Then we can make larger changes to it in another pull request later, if there is something that could improve the way it works in general.
Also: I noticed that we never actually used match_record.sentinel in the code now (since we use information about the match_record being nil or not instead now), so I removed that from the code.

I have fixed the local strategy so it should work with multiple injections of the same language now (this didn't work before based on my testing). I think there are other changes that we could make, but before doing that it is probably worth it to reconsider how the local strategy should work in more detail instead.
In other words, I think the local strategy code is ready for review now, and then we can consider changing the local strategy more later.

In the local strategy, what is the point of adding another level to match_trees? My guess is that it is for when there are multiple places where another language is injected. For example a Markdown buffer might contain multiple Lua code blocks and we need a way of distinguishing the different trees. Is that correct?

In the local strategy, what is the point of adding another level to match_trees? My guess is that it is for when there are multiple places where another language is injected. For example a Markdown buffer might contain multiple Lua code blocks and we need a way of distinguishing the different trees. Is that correct?

That is correct, yes. The old code just used the last code block of a given injected language, so it wouldn't highlight any other block of that language properly.
Also, a quick question: You write -- NOTE: according to the Lua 5.1 manual %p is not supported, yet it works, but I thought it was included: https://www.lua.org/manual/5.1/manual.html#5.4.1
Are there multiple different manuals? Or do you mean that %p has another meaning according to the manual? (punctuation)

Also, a quick question: You write -- NOTE: according to the Lua 5.1 manual %p is not supported, yet it works, but I thought it was included: https://www.lua.org/manual/5.1/manual.html#5.4.1
Are there multiple different manuals? Or do you mean that %p has another meaning according to the manual? (punctuation)

The Lua 5.1 manual is in :h luaref. You have linked to the regex pattern, which are different from string format options (they just happen to look the same). Under :h string.format it says the following:

The format string follows the same rules as the printf family of standard C functions. The only differences are that the options/modifiers *, l, L, n, p, and h are not supported and that there is an extra option, q.

In C the %p prints the memory address of a pointer. This is also what the %p in Neovim does. My guess is that it is provided as a non-standard extension by LuaJIT. On platforms where LuaJIT is supported that is most likely what will be running, but LuaJIT is not guaranteed.
But do we really need the memory address in the first place? Could we just use the table itself as the key? If we use a table as the key then only this particular unique table will work, which should have the same effect as using the address.

Also, a quick question: You write -- NOTE: according to the Lua 5.1 manual %p is not supported, yet it works, but I thought it was included: https://www.lua.org/manual/5.1/manual.html#5.4.1
Are there multiple different manuals? Or do you mean that %p has another meaning according to the manual? (punctuation)

The Lua 5.1 manual is in :h luaref. You have linked to the regex pattern, which are different from string format options (they just happen to look the same). Under :h string.format it says the following:

The format string follows the same rules as the printf family of standard C functions. The only differences are that the options/modifiers *, l, L, n, p, and h are not supported and that there is an extra option, q.

In C the %p prints the memory address of a pointer. This is also what the %p in Neovim does. My guess is that it is provided as a non-standard extension by LuaJIT. On platforms where LuaJIT is supported that is most likely what will be running, but LuaJIT is not guaranteed.
But do we really need the memory address in the first place? Could we just use the table itself as the key? If we use a table as the key then only this particular unique table will work, which should have the same effect as using the address.

I did try to just use the table at first, but I ran into some problems. Maybe I made a mistake somewhere else (I remember updating other parts of the code), I will try to see if I can change it and make it work with just tables now.

Alternatively you can apply tostring to the table. The string will always have the prefix table , but that shouldn't matter.

I changed it now and just using the table as the key works fine. I must have edited something else while introducing the get_id function that fixed the actual problem. (I think it was the match_trees[bufnr][sub_lang] = {} code.)

I think I am done now. It's getting very late in my time zone, so I don't want to start cleaning up the Git history now. I propose that tomorrow I rebase the PR into a few nice and clean commits and then we leave it as it for a day or two in case we find more edge cases.
Unrelated question: when you add type annotations like TSTree the Lua language server gives me a warning about the unknown type. Have you figured out how to teach the server about these custom Neovim types, or do you just live with the warning?

I think I am done now. It's getting very late in my time zone, so I don't want to start cleaning up the Git history now. I propose that tomorrow I rebase the PR into a few nice and clean commits and then we leave it as it for a day or two in case we find more edge cases.

That sounds like a good idea.

Unrelated question: when you add type annotations like TSTree the Lua language server gives me a warning about the unknown type. Have you figured out how to teach the server about these custom Neovim types, or do you just live with the warning?

LuaLS suggested that type to me itself and I don't get that warning. I am pretty sure that it is neodev giving me those annotations (I don't think I have anything else special in my Lua LSP setup other than Lua.diagnostics.unusedLocalExclude = { "_*" } to allow _name to be unused without a warning).
This is basically my current LSP setup (other than a lot of keymaps):

I have squashed the MR down to three commits. Unfortunately this also changed the author to myself. If you want to get credit for what has been mostly your work you can edit the commit messages to add a Co-authored-by trailer: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

I am pretty sure that it is neodev giving me those annotations

Yes, that was it, thank you.

I have squashed the MR down to three commits. Unfortunately this also changed the author to myself. If you want to get credit for what has been mostly your work you can edit the commit messages to add a Co-authored-by trailer: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

I added myself as a co-author now.

Do you prefer not to write vimdoc language injections like
>lua
if something then
  return true
else
  return false
end
<

similar to #46?
Otherwise, I think adding those + adding an empty file under vimdoc/rainbow-delimiter.scm in queries is a good idea. This makes it so that the injected languages will also use rainbow delimiters for highlighting, which I think makes a lot of sense for the docs in this case.
Note: I would just add an empty file since the vimdoc treesitter tree doesn't really allow us to write good queries for delimiters (other than {, } pairs which are already highlighted nicely).

I did not even know that >lang was a thing until a few days ago. I am definitely in favour of supporting it in the manual, just not for this PR. Let's get this out of the door first.
Anyway, I have added an empty file now. I also forgot to pull your changes, so you will have to re-add yourself for credit again. Sorry about that.

Also, we should probably update the version number since this breaks older queries.

I have not been versioning this plugin since everything has been in flux. But I think this is a good point to start finally versioning it.

I have not been versioning this plugin since everything has been in flux. But I think this is a good point to start finally versioning it.

Do you want to add a version number in the docs (you have it right at the beginning of doc/rainbow-delimiters.txt) before I redo the co-author notes on the commits?
(I assume the changelog can just be added later after this pull request, but we might as well add the correct version in the docs for when this is committed on master.)

I prefer to add version numbers directly to master. My workflow is to have a master branch that is always ready for use and tag specific milestone commits as versions. Features are developed in feature branches, then merged into master once they are ready for use. When I am satisfied with the feature set I tag a new release on master.
I admit it is somewhat arbitrary in this plugins what constitutes a release. Like, if I add a new pattern to an existing query, does this warrant a new release? It kind of does, even though the change is trivial. Usually I let it sit for a few days in case something comes up before I tag a version.

I prefer to add version numbers directly to master. My workflow is to have a master branch that is always ready for use and tag specific milestone commits as versions. Features are developed in feature branches, then merged into master once they are ready for use. When I am satisfied with the feature set I tag a new release on master.

That makes sense. You can just update the version number later, when this is on your master branch then.

I admit it is somewhat arbitrary in this plugins what constitutes a release. Like, if I add a new pattern to an existing query, does this warrant a new release? It kind of does, even though the change is trivial. Usually I let it sit for a few days in case something comes up before I tag a version.

For this plugin, I think it would make sense to update z in version x.y.z for a collection of smaller query update commits or each query update commit (depending on your preferences), and then update either x or y for something like this pull request (again, depending on your preferences, though I would probably just update y). I don't think the exact versioning choice is that important, it should just be clear that there is a version change with breaking changes like this. But, as mentioned, you can easily do that later when you commit this to the main branch.
Also, I have added myself as a coauthor again.

@Danielkonge I have been daily driving this for the last couple of days and as far as I am concerned we can merge it into master. Any objections from your side?

@Danielkonge I have been daily driving this for the last couple of days and as far as I am concerned we can merge it into master. Any objections from your side?

I have been using it too and haven't noticed any problems, so I think it is ready to be merged. So no objections from me.

Thank you very much. It has been an honour and a pleasure working with you. This inability to capture more than one node has been a thorn in my side from the start and I don't think I would have come up with a solution anytime soon. Your persistence with all the edge cases for embedded languages was especially amazing.
Speaking of embedded languages, this is really something that would benefit from more rigorous integration testing. I have Vader tests, but Vader has the problem that it runs all tests in the same Vim instance, so tests can step on each other's toes. I have been thinking about re-implementing Vader for Neovim such that every test runs in its own Neovim instance (via jobstart in a pseudo terminal), but this would require Neovim 0.10 because that version ships with lpeg. I really don't want to have to write a Vader parser by hand.

Thank you very much. It has been an honour and a pleasure working with you. This inability to capture more than one node has been a thorn in my side from the start and I don't think I would have come up with a solution anytime soon. Your persistence with all the edge cases for embedded languages was especially amazing.

Thank you for such nice words, you have also been a pleasure to work with.

Speaking of embedded languages, this is really something that would benefit from more rigorous integration testing. I have Vader tests, but Vader has the problem that it runs all tests in the same Vim instance, so tests can step on each other's toes. I have been thinking about re-implementing Vader for Neovim such that every test runs in its own Neovim instance (via jobstart in a pseudo terminal), but this would require Neovim 0.10 because that version ships with lpeg. I really don't want to have to write a Vader parser by hand.

Do you want to re-implement Vader to better support Vim and not just Neovim? Otherwise, I think the most common way I see other people do testing for Neovim plugins is by using plenary. I assume you are already aware of plenary, so is there something you think it is missing?

I have Plenary installed as a dependency for Telescope, but I never actually looked at what's inside. If it is good for integration testing I might as well use it instead of rolling my own. I do like the Vader file syntax, but I am not crazy about all the extra work of re-implementing it.

Do you want to re-implement Vader to better support Vim and not just Neovim?

I wanted to focus on just Neovim, I haven't used Vim for over half a decade at least. I don't even know if Vim can be run inside a pseudo terminal and controlled from the outside like Neovim can.


Update Wald Friedman

Needs a re-write. Duck-typed.

I think the duck typing with structs is the way to go in the first pass.  Since we will end up using named parameters (with duck typing) in the rewrite, it is the easiest way to get there.

Looks good to me. Mostly the usual fixes with linspace, undef, and broadcasting.


Pass tidal heating to Aragog and SPIDER

Passes tidal heating rate from PROTEUS to SPIDER and Aragog. Currently, this heating rate is just a constant value (W/kg) which is homogeneous throughout the mantle of the planet.
This can only be specified through the dummy orbit module (in the config file) for now.
Requires https://github.com/ExPlanetology/aragog/pull/34 to be merged first.
Closes https://github.com/FormingWorlds/PROTEUS/issues/281

This seems to work quite nicely. The plot below shows that for a non-zero tidal heat production, the planet settles into radiative equilibrium with non-zero F_atm and the surface temperature stops changing with time.


Rename 'target' to 'stage'

What feature are you missing?
The term 'target' used throughout the CLI is confusing. I propose changing the name to 'stage'. It aligns better with DevOps, with other tools (like AWS, Serverless etc.), and you solve the '-t = target vs -t = tail' issue.
How could this feature look like in detail? Tradeoffs?
Rename all occurences of target to stage.

A target is a neutral, unopinionated entity the CLI can deploy a service too.
We're speccing out a different concept that more closely resembles stages, which we'll introduce further down the road.
Definitely still up for discussion.

Closed in favour of #1329


Swift Update

Please update this one to swift. I really want it but it's in objective-c :(

??? You can easily use it from Swift

How?

You need to create a bridging header. If you google Xcode mixed project you can find out how to use obj c code from swift. It's very easy

ok but how do I use it with swift?

I have all the pod's installed and the bridging header. Now how do I use the framework with swift?

@HardcoreIOS answered https://github.com/Yalantis/Context-Menu.iOS/issues/34

How come you are closing everything?


Ability to Share Bing Conversation [enhancement]

Would it be OK if I used this in my Bing Chat History extension? I used the ShareGPT api for my ChatGPT chrome extension & I was wondering if I could use it for this new extension with the Bing Chat bot.

Hey @benf2004, are you still interested in implementing this?

This has already been implemented. To confirm, here is an example I just shared from Bing Chat using the Bing Chat History extension:
https://sharegpt.com/c/TUxWHqa

@domeccleston
I already did it on Bing Chat History


Command not found "az ml computetarget update"

Hi all,
I did not find this command after installing the ML CLI inside Azure Cloud Shell.
Because this command is mentioned in this doc.
Thank you for your help!

@jeary49252000
Thanks for your feedback! We will investigate and update as appropriate.


Fragile CI

Describe the bug
Well, as the title says, we have a number of factors in the CI that are making difficult to merge things. This is affecting the quality of the work, as manual retries are required, which causes friction in developers journey.
To Reproduce
Creating any kind of PR targeting main
Expected behavior
The CI passes, without needing to do manual retries. Ideally, all unit tests should go first and integration tests as last element.
Additional context
For the moment the factors we are taking into account here are:

Integration tests assertions. Maybe we are expecting exact values, but maybe we should use a range. But first of all clarify what we are testing there and evaluate if we need such checks.
We are executing all integration tests in parallel (all contracts). This could be causing race conditions in the nodes we are reaching for integration testing or being rate limited. In the past, we implemented #89 to minimize the impact of failures, but as we add contracts to the catalog, this could be not enough.
When adding a new contract, we need to remember to include it in the CI listings (1 2 3) . Its very easy to forget about this, and no system will warn us that the tests are not being executed, causing a false sensation of security and correctness.

Ideas are welcome @geofmureithi @mariopil

My suggestion here would be to remove the parallel execution of contracts integration tests. Actually, i would:

Execute all the unit tests, this should be a simple cargo test in the root of the project IMO.
Execute integration tests serialized.
Upload all needed artifacts.

Once having this solution in a draft pr, measure times and compare to check if its doable. I would say integration tests is not something developers should care too much in local, but defo something they should be able to execute if they want.
Let me know your thoughts.


Streaming sampling with reservoir

https://www.omniref.com/ruby/gems/stream_sampler/0.0.1/symbols/StreamSampler#line=18
there's a module https://github.com/kesla/reservoir-stream but that concats

stream-sample instead


Squiggles disappear when file is reopened

Create new python application
Paste the following in the opened editor:
class MyClass:
    pass

m = MyClasse()

Wait for colorization/squiggles to appear under MyClasse
Save and close
Reopen file from solution explorer
Result: colorization but no squiggles
You might also run into a buffer content mismatch assert, but I don't have reproduceable steps for that.

I suspect this may not repro now that we close documents properly. Reopening should trigger a new analysis, which will refresh the squiggles.


OpenFL + MLCube

Let's create a POC of open FL + MLCube.
Xin and Jurado - can you work on this together?

Hello Victor, I'll reach out to Xin and start discussing this

https://docs.google.com/document/d/10uQUouGYty36HMhW_CRxisSWpZJ6IhgEJFyDpFAmmEI/edit

We can start with this example:
https://github.com/mlcommons/mlcube_examples/tree/master/mnist
Make changes in place to that repository to:

Having MLCube take a params.json file with integer "number_batches" (can also support number of epochs).
Take in and put out a checkpoint

Xin & Jurado to connect here.

Check out these notes:
https://github.com/mlcommons/mlcube_examples/pull/23
https://github.com/mlcommons/mlcube_examples/pull/24
https://github.com/mlcommons/mlcube_examples/pull/25
These should be a good starting point

Fantastic meeting with OpenFL today shows that our demo looks great.
https://github.com/mlcommons/mlcube_examples/pull/26
Waiting to hear back from the openfl folks with feedback.

Great work!


Create release notes template

Something along the lines of:
Release X.x.x
=====

Major changes
-----

### Name
   - Description:
   - Issue(s):
   - Impact:
   - Migration plan:

Minor changes
-----
### Name
   - Description:
   - Issue(s):
   - Impact:

Patch changes
-----
### Name
   - Description:
   - Issue(s):

Each PR should be accompanied by an entry in the release notes. If the developer has not made this entry, the PR should be rejected. Documentation should be updated to this effect.

Rebecca: Submit PR.

Added an additional line:
Release X.x.x
-----

### Major changes

#### Name
   - Description:
   - Motivation/Rationale:
   - Impact:
   - Migration plan:
   - Issue(s):

### Minor changes

#### Name
   - Description:
   - Motivation/Rationale:
   - Impact:
   - Issue(s):

### Patch changes

#### Name
   - Description:
   - Motivation/Rationale:
   - Issue(s):


sdk/state: dont let escrow account sequence number go backwards

WHAT
if the channel ingests a transaction that would set the initiator escrow account seq num backwards, then return the ingestion process early and don't update state.
WHY
it can't be certain that transaction will be ingested in order in case two are submitted at the same time. So always just update the state from the transaction with the latest sequence number
closes https://github.com/stellar/experimental-payment-channels/issues/267

I have another thought. It seems somewhat fraught with difficulty to support receiving transactions out of order. How about we say the IngestTx must receive txs in order. And introduce a check that errors if that happens? I'm thinking about how we could introduce that check simply.

support receiving transactions out of order

I know previously we had this receiving transactions out of order, but now we're receiving them in order, or at least that's the way the Horizon ingester is implemented.

it looks like ingestTxMetaToUpdateBalances is the only ingest method that can go backwards atm 🤔 . Maybe a change in there?


[Bug][Worker] All worker nodes can receive the kill task message

Describe the bug
When the workflow instance is stopped, all workers can receive the kill task message, which will lead to the node that does not run the task and kill the process with the same PID on the worker node.
(当停止工作流实例时，所有的worker都能接受到kill task的消息，会导致没有运行该任务的Worker节点，误杀掉该节点上相同pid的进程)
To Reproduce
Steps to reproduce the behavior, for example:

run a process definition （运行一个工作流，可创建一个shell task执行sleep命令）
stop this process instance （停止对应的工作流定义）
you can see the kill task message in all worker logs（日志中可以看到所有的worker都有收到kill task的消息）

Expected behavior
Only the worker of run task receives the message （在所有的worker日志中都能看到kill task的消息）
Which version of Dolphin Scheduler:
-[1.2.0]

This problem should have been fixed in the 1.3.x branch, and the communication between master and worker should be changed to netty communication. You can look at the host specified in the MasterTaskExecThread class in the source code to communicate with worker, so when stopping a task, you can specify the worker where the task is to kill the task.

这个问题应该在1.3.x分支已经修复，master 和worker 通信改成netty通信，可以看一下源码MasterTaskExecThread 类里面已经指定任务所在的host 进行和worker通信，所以在停止任务时，可以指定任务所在的worker去kill任务.

already fixed in 1.3.x


About Multi-Stage Multi-Task Learning

Hi author!
Thank for your interesting great work.
But I am confuse about Multi-Stage Multi-Task Learning mentioned in paper

According to this picture in paper
I would like to know when we do multi-stage multi-task learning
For each task training, should we train the task until convergence and then move on to the next task, or do we just need to train some epochs (steps) and then we can switch to the next task?
Thank you.

Hi, thanks for your interest. Your second answer is what we did.


idtoken serverless sample did not work when running in Cloud Functions

I have been following the Cloud Functions documentation to invoke a cloud function from another cloud function.
The target URL I used is in the following format:
https://GCP_REGION-PROJECT_ID.cloudfunctions.net/my-function

Based on the sample code:
https://github.com/googleapis/google-auth-library-nodejs/blob/dd2d07a01daf9d7e766189b889fb36a9f6fb8e73/samples/idtokens-serverless.js#L36-L41
The value of the variables will be:

url: https://GCP_REGION-PROJECT_ID.cloudfunctions.net/my-function
targetAudience: https://GCP_REGION-PROJECT_ID.cloudfunctions.net

When calling the another cloud function, GaxiosError will be returned with HTTP 401 Unauthorized with error message The access token could not be verified.
After comparing with the JWT generated by metadata server, I found that the value of targetAudience should be set equal to the value of url. After doing so, I can call the another cloud function without error.
In conclusion, the sample should have the targetAudience set equal to url. However, I am not sure if the same applies if the target is Cloud Run.

@VeryCrazyDog thanks for digging into this, and figuring out how to make things work in Cloud Functions.
Mind providing an example of the code that worked for Cloud Functions, perhaps we can add a blurb specifically for Cloud Functions.

Below is the tested code that worked on Cloud Functions. Make sure the permission is properly granted to the caller service account. After deploy, invoke the function on Google Cloud Platform Console via the TESTING page with triggering event payload {"data": {}}.
'use strict'

const functions = require('firebase-functions')
const { GoogleAuth } = require('google-auth-library')

// Hardcoded configurations
const CALLER_SERVICE_ACCOUNT =<EMAIL_ADDRESS>const TARGET_URL = 'https://GCP_REGION-TARGET_PROJECT_ID.cloudfunctions.net/FUNCTION_NAME'

// Dependencies initialization
const auth = new GoogleAuth()

// Cloud function implementation
exports.idtokensServerless = functions.runWith({
  serviceAccount: CALLER_SERVICE_ACCOUNT
}).https.onCall(async (data, context) => {
  const url = TARGET_URL
  const targetAudience = TARGET_URL
  console.info(`Request ${url} with target audience ${targetAudience}`)
  const client = await auth.getIdTokenClient(targetAudience)
  const resp = await client.request({ url })
  console.info(resp.data)
})

The sample was updated in #1218 to no longer use .origin(). Using the full URL for the audience works on both GCF and Cloud Run.

I have opened another issue to cover this, because this is a common issue created by the documentation asking people to use the serverless example provided in this repo.
If the example is called serverless, it should provide examples for cloud run and cloud functions explicitly, otherwise people will have this problem, that quite frankly, is too often.
Also, another issue people confuses, is that the audience should be the url (like mentioned by VeryCrazyDong) but it is not quite right.
The audience need to match the trigger in the cloud functions, in some cases people uses query strings or subpaths to represent REST operations, and using the url will also cause authentication issues.
Audience Examples:
valid: https://GCP_REGION-TARGET_PROJECT_ID.cloudfunctions.net/FUNCTION_NAME
invalid: https://GCP_REGION-TARGET_PROJECT_ID.cloudfunctions.net/FUNCTION_NAME?query=string
invalid: https://GCP_REGION-TARGET_PROJECT_ID.cloudfunctions.net/FUNCTION_NAME/resource/id


QuickBooks print PDF does not display

Attach (recommended) or Link to PDF file here:
Configuration:

Web browser and its version: Chrome 77.0.3865.120 (Official Build) (64-bit)
Operating system and its version: Ubuntu 18.04
PDF.js version: 2.3.164
Is a browser extension: Yes

Steps to reproduce the problem:
This repros 100% with the PDF viewer, but not with the built in Chrome PDF viewer.

Register for a trial account on QuickBooks
Once on their main panel, click Reports on the left
Click any of the reports, such as Accounts receivable aging summary
Skip through the tutorial / help text
Click the print icon at the top right of the report

What is the expected behavior? (add screenshot)
Display the PDF report on QuickBooks

What went wrong? (add screenshot)
The PDF did not display (failed with a 400 error)

PDF.js v2.3.164 (build: 12ff2527)
Message: Unexpected server response (400) while retrieving PDF "https://c9.qbo.intuit.com/qbo9/v3/company/9130347358368066/reports/i/execute/EMP_CONTACT/getReport/preview/6183893060597334_-1785807796_1574116332588/PDF?title=Employee%20Contact%20List".

The request/response copied from the network tab in Chrome:
Request URL: https://c10.qbo.intuit.com/qbo10/v3/company/9130347358426446/reports/i/execute/PANDL_DET/getReport/preview/9303473582647614_-146620804_1574116611567/PDF?title=Profit%20and%20Loss%20Detail
Request Method: GET
Status Code: 400
Remote Address: <IP_ADDRESS>:443
Referrer Policy: no-referrer-when-downgrade
Access-Control-Expose-Headers: accept-ranges,content-encoding,content-length,content-disposition
cache-control: max-age=0, no-cache, no-store, must-revalidate, private
content-length: 0
date: Mon, 18 Nov 2019 22:36:51 GMT
expires: 0
qbo-version: 1922.184
server: openresty
service-time: total=3, db=1
status: 400
strict-transport-security: max-age=63072000; includeSubdomains;
x-xss-protection: 1; mode=block
Provisional headers are shown
Sec-Fetch-Mode: cors
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.120 Safari/537.36
title: Profit and Loss Detail

Link to a viewer (if hosted on a site other than mozilla.github.io/pdf.js or as Firefox/Chrome extension):
https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacbdfmadadm?hl=en

Does the url pdf.js tries to get the document from work outside of the pdf viewer? The 400 status points to the server having a problem with the url used by pdf.js

This repros 100% with the PDF viewer, but not with the built in Chrome PDF viewer.
1. Register for a trial account on QuickBooks

2. Once on their main panel, click Reports on the left

3. Click any of the reports, such as `Accounts receivable aging summary`

4. Skip through the tutorial / help text

5. Click the print icon at the top right of the report

This doesn't seem like a great reduced test-case; but anyway @Rob--W can you please take a look?

@llo-dia
I had the thought that maybe the way that the plugin was making the request was causing QuickBooks server to return a 404 (versus the way Chrome would internally request it). That's just a hypothesis that I've done no testing on. Another side note is that it works in FireFox with pdf.js.

@Snuffleupagus
How would you reduce it?

@TrevorSundberg The PDF Viewer extension works in Chrome as follows:

PDF response detected; request aborted and the viewer is loaded.
The viewer sends a new request for the PDF file.

Could you compare the request headers for the original request and the new request (e.g. via chrome://net-export + netlog viewer) and look for differences between the two? If the requests are identical, then it could be that the server refuses repeated requests.

@Rob--W
I'll take a look when I get time. That sounds like it could be exactly the problem. Since it's generated, they probably only expect one request and toss the generated pdf out after they get it.


Fix reference to deprecated View.propTypes.style

Fix reference to View.propTypes.style, as android and iOS release build crash on startup using react-native-toastify v0.3.0 and react-native v0.55.4.

Hello @kleisto ,
Thank you for your contribution.
Your fix has been merge and the package released and published.
🚀


Add Support for Quantized Model in VLLM - $500 Reward

We need to add support for the quantized model in the VLLM project. We need this to run a llama quantized model via vllm.  This involves implementing quantization techniques to optimize memory usage and runtime performance. A reward of $500 will be granted to the contributor who successfully completes this task.

Might be worth mentioning the 500$ reward in this issue #392

@petrasS3 There is fast implementation with merge request #762. Would this qualify? Just working on some cosmetic code-style improvements then it will be mergable.

omg this is amazing, did you test out TheBloke models and especially the GPTQ ones

i tried to implement using GPTQ but AWQ quantization was faster and better inference quality.
https://github.com/mit-han-lab/llm-awq/blob/main/scripts/llama_example.sh
here is a link to example of quantizing using AWQ, its very straightforward

Does this mean GPTQ models cannot be loaded with this? Yes, AWQ is faster, but there are not that many models for it.

Just having "load in 8-bit" support alone would be fine as a first step.

Did you try it with GPTQ?

Aha, I recommend our project LMDeploy, which support 4-bit weight  and 8-bit k/v cache quantization and inference.
Check the performance on Geforce RTX 4090 out. Totally worth 500$ :smile:

model
llm-awq
mlc-llm
lmdeploy

Llama-2-7B-chat
112.9
159.4
206.4

Llama-2-13B-chat
N/A
90.7
115.8

Memory (GB) comparison results between 4-bit and 16-bit model with context size 2048 and 4096 respectively,

model
16bit(2048)
4bit(2048)
16bit(4096)
4bit(4096)

Llama-2-7B-chat
15.1
6.3
16.2
7.5

Llama-2-13B-chat
OOM
10.3
OOM
12.0

Does this mean GPTQ models cannot be loaded with this? Yes, AWQ is faster, but there are not that many models for it.

I will be starting to upload AWQ models to HF soon, hopefully in the next few days.

Working with AWQ models will also get easier soon enough when #72 is merged in AWQ. The plan is to make it easier to use so we can improve it.

Aha, I recommend our project LMDeploy, which support 4-bit weight and 8-bit k/v cache quantization and inference. Check the performance on Geforce RTX 4090 out. Totally worth 500$ 😄
model	llm-awq	mlc-llm	lmdeploy
Llama-2-7B-chat	112.9	159.4	206.4
Llama-2-13B-chat	N/A	90.7	115.8
Memory (GB) comparison results between 4-bit and 16-bit model with context size 2048 and 4096 respectively,
model	16bit(2048)	4bit(2048)	16bit(4096)	4bit(4096)
Llama-2-7B-chat	15.1	6.3	16.2	7.5
Llama-2-13B-chat	OOM	10.3	OOM	12.0

Where can I find the docs ?

Does this mean GPTQ models cannot be loaded with this? Yes, AWQ is faster, but there are not that many models for it.

I will be starting to upload AWQ models to HF soon, hopefully in the next few days.

Will you share the code for quantizing AWQ ?

Does this mean GPTQ models cannot be loaded with this? Yes, AWQ is faster, but there are not that many models for it.

I will be starting to upload AWQ models to HF soon, hopefully in the next few days.

Will you share the code for quantizing AWQ ?

I'm expecting to just use the code from the main AWQ repo - it's documented here: https://github.com/InternLM/lmdeploy
Looks easy enough!

Aha, I recommend our project LMDeploy, which support 4-bit weight and 8-bit k/v cache quantization and inference. Check the performance on Geforce RTX 4090 out. Totally worth 500$ 😄
model	llm-awq	mlc-llm	lmdeploy
Llama-2-7B-chat	112.9	159.4	206.4
Llama-2-13B-chat	N/A	90.7	115.8
Memory (GB) comparison results between 4-bit and 16-bit model with context size 2048 and 4096 respectively,
model	16bit(2048)	4bit(2048)	16bit(4096)	4bit(4096)
Llama-2-7B-chat	15.1	6.3	16.2	7.5
Llama-2-13B-chat	OOM	10.3	OOM	12.0

Where can I find the docs ?

https://github.com/InternLM/lmdeploy/blob/main/docs/en/w4a16.md

Aha, I recommend our project LMDeploy, which support 4-bit weight and 8-bit k/v cache quantization and inference. Check the performance on Geforce RTX 4090 out. Totally worth 500$ 😄
model	llm-awq	mlc-llm	lmdeploy
Llama-2-7B-chat	112.9	159.4	206.4
Llama-2-13B-chat	N/A	90.7	115.8
Memory (GB) comparison results between 4-bit and 16-bit model with context size 2048 and 4096 respectively,
model	16bit(2048)	4bit(2048)	16bit(4096)	4bit(4096)
Llama-2-7B-chat	15.1	6.3	16.2	7.5
Llama-2-13B-chat	OOM	10.3	OOM	12.0

Where can I find the docs ?

https://github.com/InternLM/lmdeploy/blob/main/docs/en/w4a16.md

does it support the docker container because it keeps giving an error?
[ 96%] Built target triton-common-logging
[ 97%] Built target triton-common-thread-pool
[ 98%] Built target triton-common-table-printer
[ 99%] Linking CXX executable ../../../bin/llama_triton_example
/usr/bin/ld: CMakeFiles/llama_triton_example.dir/llama_triton_example.cc.o: in function `MPI::Op::Init(void (*)(void const*, void*, int, MPI::Datatype const&), bool)':
/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/op_inln.h:121: undefined reference to `ompi_mpi_cxx_op_intercept'
/usr/bin/ld: CMakeFiles/llama_triton_example.dir/llama_triton_example.cc.o: in function `MPI::Intracomm::Intracomm(ompi_communicator_t*)':
/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/intracomm_inln.h:23: undefined reference to `MPI::Comm::Comm()'
/usr/bin/ld: CMakeFiles/llama_triton_example.dir/llama_triton_example.cc.o: in function `MPI::Intracomm::Intracomm()':
/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/intracomm.h:25: undefined reference to `MPI::Comm::Comm()'
/usr/bin/ld: /usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/intracomm.h:25: undefined reference to `MPI::Comm::Comm()'
/usr/bin/ld: /usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/intracomm.h:25: undefined reference to `MPI::Comm::Comm()'
/usr/bin/ld: /usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/intracomm.h:25: undefined reference to `MPI::Comm::Comm()'
/usr/bin/ld: CMakeFiles/llama_triton_example.dir/llama_triton_example.cc.o:/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi/ompi/mpi/cxx/intracomm.h:25: more undefined references to `MPI::Comm::Comm()' follow
/usr/bin/ld: CMakeFiles/llama_triton_example.dir/llama_triton_example.cc.o:(.data.rel.ro._ZTVN3MPI8DatatypeE[_ZTVN3MPI8DatatypeE]+0x78): undefined reference to `MPI::Datatype::Free()'
/usr/bin/ld: CMakeFiles/llama_triton_example.dir/llama_triton_example.cc.o:(.data.rel.ro._ZTVN3MPI3WinE[_ZTVN3MPI3WinE]+0x48): undefined reference to `MPI::Win::Free()'
collect2: error: ld returned 1 exit status
make[2]: *** [examples/cpp/llama/CMakeFiles/llama_triton_example.dir/build.make:132: bin/llama_triton_example] Error 1
make[1]: *** [CMakeFiles/Makefile2:3040: examples/cpp/llama/CMakeFiles/llama_triton_example.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

It supports docker. Can you open an issue in lmdeploy and tell us how to reproduce it?

Aha, I recommend our project LMDeploy, which support 4-bit weight and 8-bit k/v cache quantization and inference. Check the performance on Geforce RTX 4090 out. Totally worth 500$ 😄
model	llm-awq	mlc-llm	lmdeploy
Llama-2-7B-chat	112.9	159.4	206.4
Llama-2-13B-chat	N/A	90.7	115.8
Memory (GB) comparison results between 4-bit and 16-bit model with context size 2048 and 4096 respectively,
model	16bit(2048)	4bit(2048)	16bit(4096)	4bit(4096)
Llama-2-7B-chat	15.1	6.3	16.2	7.5
Llama-2-13B-chat	OOM	10.3	OOM	12.0

Are these numbers measured at batch size 1? Also, how to reproduce these numbers?

What's the progress here? Do we use LMDeploy or will vllm have support for this?

What's the progress here? Do we use LMDeploy or will vllm have support for this?

It looks like #762 is close to being merged. I was sad to see "currently does not support tensor parallelism" but there is already a fork being worked on for that.
I only just learned of skypilot and started using it today. I think once that fork becomes a PR, AWQ quantized models can be deployed with skypilot as well?

LMDeploy (>=v0.0.6) supports tensor parallelism for 4-bit quantized LLM model inference

LMDeploy (>=v0.0.6) supports tensor parallelism for 4-bit quantized LLM model inference

What's the plan to support more models like MPT or Falcon?

The only models I use are either Llama2-GPTQ or Flan-T5, so I can't use vLLM whatsoever. 😢

What do you mean by you can’t use it?

The only models I use are either Llama2-GPTQ or Flan-T5, so I can't use vLLM whatsoever. 😢

What do you mean by you can’t use it?

vLLM doesn't support GPTQ models or T5 models, from what I can tell.

You should use AWQ models either way as they are better.

823 is greater than 47.

I already talked with TheBloke, more models will come for AWQ.

does anyone know if VLLM support quantised models yet? Reading through these comments it looks like AWQ might be supported?

Is it possible to now directly use the AWQ models from HF Hub now?

Does the AWQ support higher than 4 bits per weight? For example 8 bits like GGUF?

Does AWQ support higher than 4 bits per weight? For example 8 bits like GGUF?

AWQ supports 4 bits per weight. Probably 8 bits will come at a later time.

I tried the custom AWQ model with the latest vllm=0.2.0 - it allows to load a quantized model but without performance gain - I guess because it's dequantized during the load.
https://github.com/vllm-project/vllm/commit/2b1c116b5acdf3b738e310f98617875132214c37#diff-074bf1408c395d4d7dfaa07aaafaf6ebebb2b3bde50cdbbb868cc95d23daffb5

Closing as this is now resolved.

so who won the $500 🙂


Fix doc of verification.generate_qr_code return

https://github.com/matrix-org/matrix-rust-sdk/blob/main/crates/matrix-sdk-crypto/src/verification/requests.rs#L395
generate_qr_code can return both a QrVerification or None

To be honest, I don't know why the CI / test is failing. The job is flacky ?
My PR is purely on documentation


Firefly / Traefik external port incorrect

The firefly application doesn't seem to work out of the box.
After some playing around I made a bit of progress, but not yet working.

Using default configuration: I get an empty page when browsing to the URL. The logs mention something about a

RuntimeException: No application encryption key has been specified.

I noticed the configuration file in tasks/firefly.yml uses FF_ prefixes for the environment variables. But that won't work, right? When I remove the FF_ prefix, the behavior is different. Now instead of a blank page I get a page mentioning 'whoops, there was a fatal error, please check logs' (I would think: progress :)). The log is now mentioning this:

In PDOConnection.php line 27:
SQLSTATE[HY000] [1045] Access denied for user<EMAIL_ADDRESS>(using password: YES)

So question, has firefly already worked out-of-the box? Or are there some extra steps to be taken? (the documentation is not mentioning something specifically).
Thanks!

Some more issues I found:

by default a postgresql database is used for firefly (https://docs.firefly-iii.org/en/latest/installation/docker.html). I would think it's maybe easiest (configuration-wise) to also use a postgresql docker container? When I try this I get some other errors (so still not working though)
Also, the traefik.port is wrong I think, but I'm not yet sure what should be the correct one (still experimenting)

So question, has firefly already worked out-of-the box? Or are there some extra steps to be taken? (the documentation is not mentioning something specifically).
Yes, it works fine - I wouldn't have committed it otherwise ;-)

The environment variables were changed a month or so back - I think you need to pull the latest from master. See https://github.com/davestephens/ansible-nas/blob/master/tasks/firefly.yml.

I think I have it working now. I did use a postgresql docker container. Summary of changes that made it work for me:

change to pgsql container (postgres:10).
changed env for the firefly container: I use DB_HOST (db), DB_CONNECTION (pgsql), DB_DATABASE (firefly), DB_USERNAME (firefly), DB_PASSWORD (same password as set in the postgres container), DB_PORT (5432), and I also set TRUSTED_PROXIES (in case you want reverse proxy to work).

I'm not sure these changes are required, did you test with the latest code from master?

Ah; no I didn't. Ok, I will test with the latest changes, thanks for notifying. I'll let you know if that works.

Ok, with the latest code from master I can run firefly locally! Not from external yet. some traefik config issue still.

Agreed that you don't want this to be exposed (except maybe for some testing/experimenting remotely). In that case I'd put a warning / hint in the group_vars/all.yml file, sth like "don't expose this port unless you really want to, this is your personal financing, after all".


schedule in TVM

In NNVM，when Loading the Keras Model or other during the “execute on TVM” part, can TVM automatically use the suitable schedule according to the ctx?

Yes. But it is according to target (the argument for tvm.build) rather than context.
TVM has a target dispatching system in TOPI. Each target (cuda, opencl, llvm) overloads the schedule. When you call the generic schedule, tvm will automatically call the overloaded one.

Thx, And after
////////////////////////////////////
deploy_graph, lib, params = nnvm.compiler.build(
compute_graph, target="cuda", shape={"x": shape}, dtype="float32")
import tvm
import numpy as np
from tvm.contrib import graph_runtime, util
module = graph_runtime.create(deploy_graph, lib, tvm.gpu(0))
x_np = np.array([1, 2, 3, 4]).astype("float32")
y_np = np.array([4, 4, 4, 4]).astype("float32")
module.set_input(x=x_np, y=y_np)
////////////////////////////////////
when running "module.run()", can TVM automatically find and use the suitable schedule for this module?

yes, define and register the scheduler for the target "cuda" in topi and call the generic scheduler from NNVM scheduler registration. Based on the target used for build, TVM automatically pick the scheduler definition.
hope this will help you