Send ratelimit request to ratelimit service on response flow with hits_addend

Title: Send ratelimit request to ratelimit service on response flow with hits_addend for reporting purpose.
Description:
Context: We have a use case that involves internally using the OpenAI API and using Envoy to send the request to OpenAI servers. We want to rate-limit on the # token-consumed per each OpenAI API request on a user-level. The token consumed is calculated by the token used in request + response body. The total token consumed (request + response) will be sent as part of the response back from OpenAI API so we would like to take this token-consumed number and send to the ratelimit sidecar via the ratelimit filter (using the hits_addend field).
We think to achieve the above, three work items need to be completed:

Update ratelimit filter to support sending hits_addend

I've started by landing this PR so ratelimit grpc client supports hits_addend.

Update ratelimit filter so it can be configured to send request to sidecar on response flow.

cc: @sc0ttbeardsley @JuniorHsu @fishcakez
Any thoughts on the problem?
Do you think the proposed solution is a good approach or are there better ways to achieve?
[optional Relevant Links:]

Any extra documentation required to understand the issue.

cc: @esmet @mattklein123 @yanavlasov

Hi @PeterL328 , as discussed here, do you think rate limiting on the sum of Prompt text and count and The max_tokens (max number of generated token) in the request should be simple and enough here?
For item 3, for example I have a limit of 500 tokens/min, there is a request with 300 tokens in prompts and max_tokens: 300 (total token 600), the request should be rejected. However, if the max_tokens is 200 (total tokens: 500), it should proceed.

@PeterL328 Can you help me understand the proposal better, please? If I understand it correctly, you are proposing that each request has a cost which is computed by the server and sent back to Envoy in the response. The rate limit filter needs to report this cost to the rate limiting service, so it can properly update the amount of used cost for the descriptor. Is this correct?

Hi @yanavlasov
I've removed the 3rd bullet point in my original post since I believe it's not needed anymore. but the first two points still I think is required.
Yea the full request cost (calculated by counting the token in the request body and the token in the response body) is only known when we get the response back from OpenAI. We want to report this cost to the ratelimit service (sidecar).
Currently ratelimit filter will only report cost to the sidecar on request flow. I believe we want to report on the response flow.

hi @lizzzcai,
I've response to your question in the other thread. I think using the max_tokens is fine but it will not be accurate as I think that is just to set an upper bound of what OpenAI can use for the response body.

I see. I think what you are looking for is better handled by the new rate limit quota service, since it already has load reporting built in, but it is still not completed. Adding it to the RLS will make it even less efficient, since it will now cost 2 RPCs per request.
If you want to proceed with this, then it a new service needs to be defined (or existing service extended with a new method, not sure if this is supported for backward compatibility) with a new method to report load. Otherwise you have a response that needs to be discarded.
Also note that rate limiting service as defined today is unary. It means that load report will go a different server. If the load report needs to go to the same server that received the first request, then the API needs to be changed to streaming.

hi @lizzzcai, I've response to your question in the other thread. I think using the max_tokens is fine but it will not be accurate as I think that is just to set an upper bound of what OpenAI can use for the response body.

Thanks @PeterL328 for your response. Agree that the total consumed token in the response will be more accurate. In your use case, you also expect the reporting which will require accurate data. For my use case, I just need to rate limit the TPM(token per min) so the upper bound of the request should be enough for us.

We are particularly interested in that first work item:

Update ratelimit filter to support sending hits_addend

Which would enable configuring RateLimit messages like so:
      rate_limits:
        - actions:
            -
            -
          hits_addend: 4
          stage: 1
        - actions:

To enable counting a single "expensive" request as multiple hits.
A few quick questions:

Should we open a separate issue to track this [since it seems less complex than the full ask here]?
Since @PeterL328 has done some of the initial work under https://github.com/envoyproxy/envoy/pull/28939; is the remaining work something a new contributor might be able to handle? Is there another way we could help?

Hi @ChuckCrawford,

Should we open a separate issue to track this [since it seems less complex than the full ask here]?

I think it's probably better to open a new issue for you ask. This issue is more about discussing making calls to ratelimit sidecar on response flow.

Since @PeterL328 has done some of the initial work under https://github.com/envoyproxy/envoy/pull/28939; is the remaining work something a new contributor might be able to handle? Is there another way we could help?
You can wait for someone to pickup your issue but it may take long time. If you want it in open source sooner, probably can get started on your own. Whoever gets assigned to you issue maybe can help, you can also tag me for assistance.

@yanavlasov
Just to be clear, when you say service in your previous post. I assume you mean envoy filters not sidecar service correct?

I think what you are looking for is better handled by the new rate limit quota service, since it already has load reporting built in, but it is still not completed

I see, do you mean this one https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rate_limit_quota_filter.html?

Adding it to the RLS will make it even less efficient, since it will now cost 2 RPCs per request.

The second request we think will be made during the response flow (i.e during encoding) and maybe can be non-blocking since we discard the ratelimit response from the sidecar.
Is my understanding correct that both ratelimit filter and ratelimit quota filter can talk to the same ratelimit sidecar but additional work is required to support my use case with ratelimit quota filter?

@yanavlasov
Just to be clear, when you say service in your previous post. did you mean envoy filter or sidecar service? If you meant service not filter, is the rate limit quota service based off of ratelimit sidecar?

I meant sidecar service. The the rate limit quota service is not based on the https://github.com/envoyproxy/ratelimit it is a different API altogether. The sidecar service is not yet developed unfortunately.

I think what you are looking for is better handled by the new rate limit quota service, since it already has load reporting built in, but it is still not completed

I see, do you mean this one https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rate_limit_quota_filter.html?

Adding it to the RLS will make it even less efficient, since it will now cost 2 RPCs per request.

The second request we think will be made during the response flow (i.e during encoding) and maybe can be non-blocking since we discard the ratelimit response from the sidecar.

Sure. But this will need new API and I'm unsure the existing rate limit API can be changed in a backward compatible way.

Is my understanding correct that both ratelimit filter and ratelimit quota filter can talk to the same ratelimit sidecar but additional work is required to support my use case with ratelimit quota filter?

I think rate limit quota filter will need a new sidecar service. I'm unsure how much of the existing ratelimit sidecar can be reused.

@PeterL328 @lizzzcai @yanavlasov @ChuckCrawford
i'm the maintainer of LiteLLM we allow you to maximize your throughput/increase rate limits -  load balance between multiple deployments (Azure, OpenAI)
I believe litellm can be helpful here - and i'd love your feedback if we're missing something
Here's how to use it
Docs: https://docs.litellm.ai/docs/routing
from litellm import Router

model_list = [{ # list of model deployments
    "model_name": "gpt-3.5-turbo", # model alias
    "litellm_params": { # params for litellm completion/embedding call
        "model": "azure/chatgpt-v-2", # actual model name
        "api_key": os.getenv("AZURE_API_KEY"),
        "api_version": os.getenv("AZURE_API_VERSION"),
        "api_base": os.getenv("AZURE_API_BASE")
    }
}, {
    "model_name": "gpt-3.5-turbo",
    "litellm_params": { # params for litellm completion/embedding call
        "model": "azure/chatgpt-functioncalling",
        "api_key": os.getenv("AZURE_API_KEY"),
        "api_version": os.getenv("AZURE_API_VERSION"),
        "api_base": os.getenv("AZURE_API_BASE")
    }
}, {
    "model_name": "gpt-3.5-turbo",
    "litellm_params": { # params for litellm completion/embedding call
        "model": "vllm/TheBloke/Marcoroni-70B-v1-AWQ",
        "api_key": os.getenv("OPENAI_API_KEY"),
    }
}]

router = Router(model_list=model_list)

# openai.ChatCompletion.create replacement
response = router.completion(model="gpt-3.5-turbo",
                messages=[{"role": "user", "content": "Hey, how's it going?"}])

print(response)

Going to close the issue for now.
Internally we have an envoy filter that makes grpc calls to ratelimit sidecar on response flow but seems it will be tricky to open source it as it's a very specific need from us.
Maybe someone can open another issue to add support for hits addend in ratelimit envoy filter since kind of a separate issue.

@PeterL328 Peter, thanks for your contribution on this. Could you please give us a simple example on how to use the hits_addend to achieve TPM limite in rate limit sidecar? Thanks!

@RyoYang may https://github.com/envoyproxy/envoy/pull/34184 help you.


Auto documentation of Config Pydantic schema

Add autodoc-pydantic to automatically create documentation from our configuration schema.
See: https://autodoc-pydantic.readthedocs.io
Checklist:

[x] Commit Message Formatting: Commit titles and messages follow guidelines in the
conventional commits.
[ ] Changelog updated with breaking and/or notable changes for the next minor release.
[ ] Documentation has been updated, if necessary.
[ ] Unit tests have been added, if necessary.
[ ] Integration tests have been added, if necessary.

Demo: https://cheimes.fedorapeople.org/instructlab/config.html
To make the auto-generated docs useful:

move all default values from get_default_config into the model
add inline doc strings or `Field(description="doc string") to all fields.

@tiran I've picked this up and added you as a co-author for one of the commits here: https://github.com/instructlab/instructlab/pull/2830. Thank you for getting all of the docs infra set up for the project.


map unsubscribedFromEmails trait to unsubscribed_from_emails

Can you add a link to the docs in the code as a comment?

done!

@f2prateek @sperand-io are we going to spec the "unsubscribe" event? Because it's getting to a point where a few of our popular tools are getting ad hoc ways to unsubscribe via our API.
eg. vero, mailchimp
Also -- if we are going to add this feature, we should ask for this Intercom specific trait in the integrations object like we do for their secure mode user hash on the server side.
so something like:
analytics.identify('<%= current_user.id %>', {
    email   : '<%= current_user.email %>',
    createdAt : '<%= current_user.created %>'
}, {
    integrations: {
        Intercom : {
            unsubscribedFromEmails: true
        }
    }
});

Since this is adhoc, would not want this semantic trait to bleed and pollute other tools' data.

Yeah we definitely should.

@hankim813 do you have an idea of when the above suggested changes might be made? Just want to keep a customer of ours in the loop.
Let me know if there's anything else you might need from our end. Thanks!

@amburke I think we want to spec this event rather than continuing to patch individual integration to support this in this way.
We'd like to spec an event like Unsubscribe User event and map that across all the tools that support such a feature. This will definitely take some time... not sure on the ETA.
@f2prateek

I'm okay implementing via context['Intercom'].unsubscribedFromEmails for now (traits should still just be custom_attributes, but we should also spec some top-level context field for identify calls that signifies subscription status that all the integrations can respect. @hankim813

New PR to reflect changes here #15


Mention testcontainers-bom in "Maven dependencies" section of the docs

Issue #2134

Hello @rnorth. I accepted the suggested changes, see please.


Fix bug-report --duration flag never used

Please provide a description of this PR:
--duration flag is never used in the istioctl bug-report command. Although it has been parsed to since parameter, the since parameter never gets called. This PR is to fix this issue, and correctly handle the error situation when users try to input --duration and --start-time flags at the same time.
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[x] User Experience
[ ] Developer Infrastructure

Please check any characteristics that apply to this pull request.

[ ] Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

@monsteredc: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

release-notes_istio
139c4aff862056acc900060612afbba99b05e0ac
link
true
/test release-notes_istio

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jacob-delgado PTAL.

/cherry-pick release-1.12

@monsteredc: new pull request created: #36213

In response to this:

/cherry-pick release-1.12

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.


PR for Challenge 2

I have tried solving challenge 2. Please review. Thanks!
Related issue: #9

hey there Sarthak, I couldn't merge your branch as we are having conflicts on yarn.lock . Instead I cherry-picked the commit this pr was supposed to change and can be found here : ff87cc4


Pylance/Pyright not recognizing member of imported module inside package

Environment data

Language Server version: 2023.5.10
OS and version: Windows 10.0.22621
Python version: Python 3.10.9, Anaconda

Repro Steps

Create a test folder with the 3 files (run.py, mymodule.py and __init__.py) in the following structure,
which includes a folder named mypkg:

test
│   run.py
│
└───mypkg
    │   mymodule.py
    │   __init__.py

2 . Put the following code into the 3 files, try to run to show the interpreter resolution:

Expected behavior
To have the module resolved by Pylance/Pyright as Python interpreter does.
Actual behavior
The following warning is raised in the file run.py:
"mymodule" is not a known member of module "mypkg" Pylance [reportGeneralTypeIssues]

This behavior is intended.
Your code is relying on fragile assumptions about cross-module loader side effects, and pyright (the static code analyzer upon which pylance is built) doesn't model these by design. See this documentation for more detail.
In your code, run.py is importing mypkg, but it is not importing mypkg.mymodule. The mypkg.__init__.py file doesn't assign a local symbol mymodule any value, so it is not explicitly part of the mypkg namespace. The mymodule symbol happens to be implicitly written to the __init__.py global namespace as a side effect of the import mypkg.mymodule statement, but your code should not rely on this.
If you want mypkg.__init__.py to import the mymodule submodule and assign it to a local mymodule symbol, you should include from . import mymodule in your __init__.py file. Alternatively, you could add an import mypkg.mymodule statement to run.py. Either of these will eliminate the error message you're seeing.

Ok, understood.
Thank you for the detailed explanation!


Setting cores to 1 causes Java to report unexpected CPU count

If you set cores to 1 in your SparkApplication manifest (without coreLimit set), this will result in requests.cpu being set to 1 also, with no limits.cpu set. However, the OpenJDK JVM will treat requests.cpu of 1 as a special case of no CPU shares set, when no limit is set.
A consequence of this is that Java's Runtime.getRuntime().availableProcessors() will return the number of cores on the underlying node, rather than the expected value of 1. Spark and some common associated libraries (e.g. Iceberg) use availableProcessors() to set the size of various thread pools. Java itself also uses this to set the size of the default fork join thread pool, which is used in various places.
Ultimately this could lead to instability, as higher than expected parallelism might result in more memory being used than intended. A work-around is to set coreLimit to something (though you may not want a limit), which will result in limits.cpu being set. The JVM will respect that value when reporting number of processors. Another workaround is to set -XX:ActiveProcessorCount=1 (though then you need to remeber to keep that in sync with your cores setting).
One potential solution would be to set requests.cpu to 999m rather than 1 if cores is set to 1 and no coreLimit is set. Or allow setting requests.cpu explicitly. Another option is to automatically set -XX:ActiveProcessorCount=1.
Something to note is that it appears when using cgroup v2, the JVM will use a different special value. So this won't be an issue if using cgroup v2.

Just went through the code and see there is coreRequests available, so setting that solves this. Thanks!


Add warning to getting started guide about creating multiple instances

Creating multiple instances of services can lead to connection starvation due to HttpClient leaving connections open.
Add a prominent warning about this. If we can find a workaround to alleviate it for very common cases, so much the better.

There is no clear workaround for this, as discovered when working on https://github.com/GoogleCloudPlatform/google-cloud-dotnet/issues/407
I'll add a warning about this in the docs.

@jskeet Should a warning also be put somewhere on this page: https://developers.google.com/api-client-library/dotnet/get_started ?

That's probably a call for @SurferJeffAtGoogle to make.

Closing. Please re-open or file a new issue if a warning is required in any other docs.


refactor: deprecate old vals based secret store

What type of PR is this?

What this PR does / why we need it:
deprecate old vals based secret store for upcoming secret management
Which issue(s) this PR fixes:

Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?

Additional documentation e.g., design docs, usage docs, etc.:

Pull Request Test Coverage Report for Build<PHONE_NUMBER>

3 of 9 (33.33%)  changed or added relevant lines in 1 file are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.3%) to 75.659%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

pkg/engine/operation/graph/resource_node.go
3
9
33.33%

Files with Coverage Reduction
New Missed Lines
%

pkg/engine/operation/graph/resource_node.go
3
59.06%

Totals

Change from base Build<PHONE_NUMBER>:
0.3%

Covered Lines:
6947

Relevant Lines:
9182

💛  - Coveralls


Print a warning for unnamed queries

Related to #259. I think we should strongly encourage using named vs. unnamed queries.

I disagree that this should print a warning, since the overwhelming majority of current graphql servers don't use the operation name for anything at all. Perhaps we can have an Apollo server Network interface or something which prints this warning instead?

Also, this could be part of https://github.com/apollostack/eslint-plugin-graphql or similar.

Ok, agreed, closing for now.


Limit promotional codes by quantity, not by date.

I want to have one promocode to use it 30 times with different users.
Can I do it?

One promo code , can be used 30 times by anyone, is that right?

@ladaposamuel
Yes, I think this is what @OleksiiBrylin wants.
This will be definitely a cool feature to have.

Check out 82f4fc1 we have merged #38 pull request from @OleksiiBrylin


UIOperation navigation Controller wrapping

Should we be wrapping all presented view controllers other than UIAlertController in a navigationController?
I would suggest that we add UISplitViewController, UITabBarController and of course UINavigationController to the list of unwrapped presented view controllers - or add an additional parameter to the method with a default value of true as to whether to wrap or not so that consumers can easily override.
I'd also maybe suggest renaming the operation? UIOperation sounds like an operation that will be performed on the main thread rather than an operation that performs navigation.
Finally, we might also want to add a dismissing/popping operation as well as showing/presenting.

Hi @seancatkinson - thanks for the suggestions & input!
Yes, you are correct that UIAlertController is not the only UIViewController subclass which should avoid being contained in a UINavigationController. Your list looks about right - but another argument/parameter is clearly the better way to go (and test) :+1: - a PR fixing this with tests would be great :+1:
Regarding the name, and really, the entire class... I actually meant to make UIOperation an internal class before I released v2.6 but it must have dropped off my radar. It was only added to support the presentation of controllers using AddressBook.framework, Contacts, and of course AlertOperation.
However, if you have some Operation subclasses which you think are general purpose enough to be useful to others, send me a PR and we can discuss it further.
Personally, I'm a little bit hesitant to add any more UI related functionality to the framework. This is mostly because it's a pretty slippery slope of replicating UIKit behaviors - and isn't really a direction I want this project to go in.

Closing this now as #196 has been merged which addresses this issue.


TRADFRI Remote Control E1524 On/Off/Toggle-Switch not working

No reaction on pressing On/Off (Toggle Button on Trafri Remote Control
Using deconz:
Button 1 was created, but homekit shows no reaction
The other four buttons react as aspected.
  Should be fixed in deCONZ v2.12.1 and later.

Thank you Erik for that fast response. In deCONZ:  2.12.03 / 12.7.2021 with Firmware: 26660700 using a ConBee II the error occourse.
I‘m not familiar now the version counter is used.
Regards
Peter

Am 26.07.2021 um 12:47 schrieb Erik Baauw @.***>:

See dresden-elektronik/deconz-rest-plugin#4900. Should be fixed in deCONZ v2.12.1 and later.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

Can you double-check, e.g. with dc_eventlog or in the deCONZ logging (I think with --dbg-info=2 that deCONZ actually sends web socket notifications when you press the On/Off button?

Hi Eric,
i’m sorry, i cannot get the requested information.
i can’t get my raspberry to write a deconz logfile.

Am 27.07.2021 um 10:44 schrieb Erik Baauw @.***>:
Can you double-check, e.g. with dc_eventlog or in the deCONZ logging (I think with --dbg-info=2 that deCONZ actually sends web socket notifications when you press the On/Off button?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/ebaauw/homebridge-hue/issues/1005#issuecomment-887328488, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEE4WPDMTUXDRWL5H345ZRLTZZWVHANCNFSM5A6ZYZHA.

And Help | Debug View in the GUI?
Alternatively use dc_eventlog or capture and attach a debug log file (making sure Homebridge debug mode has been enabled).

Hi Eric,
is this the information you are looking for?
{
"22:45:20:439": {
"e": "changed",
"id": "58",
"r": "sensors",
"state": {
"buttonevent": 1002,
"lastupdated": "2021-07-27T20:45:20.521"
},
"t": "event",
"uniqueid": "00:0b:57:ff:fe:31:45:e9-01-1000"
},
"22:45:24:361": {
"e": "changed",
"id": "58",
"r": "sensors",
"state": {
"buttonevent": 2002,
"lastupdated": "2021-07-27T20:45:24.440"
},
"t": "event",
"uniqueid": "00:0b:57:ff:fe:31:45:e9-01-1000"
},
"22:45:26:119": {
"e": "changed",
"id": "58",
"r": "sensors",
"state": {
"buttonevent": 3002,
"lastupdated": "2021-07-27T20:45:26.202"
},
"t": "event",
"uniqueid": "00:0b:57:ff:fe:31:45:e9-01-1000"
},
"22:45:28:31": {
"e": "changed",
"id": "58",
"r": "sensors",
"state": {
"buttonevent": 5002,
"lastupdated": "2021-07-27T20:45:28.113"
},
"t": "event",
"uniqueid": "00:0b:57:ff:fe:31:45:e9-01-1000"
},
"22:45:29:275": {
"e": "changed",
"id": "58",
"r": "sensors",
"state": {
"buttonevent": 4002,
"lastupdated": "2021-07-27T20:45:29.357"
},
"t": "event",
"uniqueid": "00:0b:57:ff:fe:31:45:e9-01-1000"
}
}

Am 27.07.2021 um 22:03 schrieb Erik Baauw @.***>:
And Help | Debug View in the GUI?
Alternatively use dc_eventlog or capture and attach a debug log file (making sure Homebridge debug mode has been enabled).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/ebaauw/homebridge-hue/issues/1005#issuecomment-887796790, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEE4WPHNCPBRKV4HDYEJMX3TZ4GHRANCNFSM5A6ZYZHA.

The first event is from the On/Off button.  You should see a corresponding message in the Homebridge log.

After upgrade deconz to v2.12.3 everything is fine again.
Thank you for your support.
Best regards from Germany

Am 28.07.2021 um 00:53 schrieb Erik Baauw @.***>:
The first event is from the On/Off button. You should see a corresponding message in the Homebridge log.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/ebaauw/homebridge-hue/issues/1005#issuecomment-887885871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEE4WPGA5AVHLY2E575T5DLTZ42HFANCNFSM5A6ZYZHA.


Delete duplicate Lung dataset in Tabula Muris Senis

Tabula Muris Senis has a duplicate Lung dataset that needs to be deleted when revisions are available:

Capturing a response from @Bento007 in conversation with @hthomas-czi and myself:

a manual script would be the easiest route. In the future we could add duplicate dataset detection using hashing for a collection.

This deletion is a special case which should either be caught during upload of the dataset (per Trent's remark) or during review of the private collection with the publisher. Unless we find other cases, I don't want to add complexity to private revisions of public collections to support this specific case.

Issue moved to chanzuckerberg/corpora-data-portal #1066 via ZenHub


Bug in Boxchart

Hello! Thank you for the excellent software, which has helped me a lot.
Recently when I was using ZoomPlot on a boxchart, I met an error.
But I don't know how to solve it. Could you help me? Thanks  : )
%test.m

t1=[rand(10,10),zeros(10,10)];
t2=[rand(10,10),zeros(10,10)];

figure
hold on;
boxchart(t1);
boxchart(t2);

zp=BaseZoom();
zp.run;

when try to draw a subAxesArea using mouse, the error occurs.
Use the left mouse button to draw a rectangle.
for the sub axes...
Warning: Error occurred while executing the listener callback
for event WindowMousePress defined for class
matlab.ui.Figure:
Error using categorical/horzcat
Unable to concatenate a double array and a categorical array.

Error in
images.roi.internal.DrawingCanvas/setConstraintLimits

Error in images.roi.Rectangle/setConstraintLimits

Error in
images.roi.internal.ROI/resetConstraintsAndFigureMode

Error in images.roi.Rectangle/wireUpListeners

Error in images.roi.internal.ROI/waitForButtonPressToBegin

Error in images.roi.internal.ROI
> In uiwait (line 81)
In images.roi.internal/ROI/draw
In drawrectangle (line 217)
In BaseZoom/addSubAxes (line 268)
In BaseZoom/run (line 188)
In test (line 11)

Sorry, I just saw the closed issue.
The ZoomPlot is not supported for boxchart.


Could anyone explain why warmup method runs the model first, rather than just allocate pagedAttention cache?.

I'm working on TGI server. But I'm curious about why warmup method runs the model first, rather than just allocate pagedAttention cache? by just calculating, since model config is given, and the size of activation and parameters are known when calling warmup.
The code is here,
        try:
            cache_manager = set_cache_manager(
                batch.blocks,
                self.num_layers,
                self.num_kv_heads,
                self.head_size,
                self.sliding_window is not None,
                self.dtype,
                self.device,
            )
            _, batch, _ = self.generate_token(batch)
        except torch.cuda.OutOfMemoryError as e:
            raise RuntimeError(
                f"Not enough memory to handle {len(batch.input_ids)} prefill tokens. "
                f"You need to decrease `--max-batch-prefill-tokens`"
            ) from e

and then
        num_blocks = (
            int(free_memory // total_cache_size)
            # Add batch.blocks as we allocated it above, so it is included in the peak memory.
            + cache_manager.num_blocks
        )

        del batch
        del cache_manager

        set_cache_manager(
            num_blocks,
            self.num_layers,
            self.num_kv_heads,
            self.head_size,
            self.sliding_window is not None,
            self.dtype,
            self.device,
        )

        return int(num_blocks * BLOCK_SIZE)

Thanks for your time and considerations!

You could approximate the vram taken by the model activations using the config but I find it simpler to just run the model and ask the driver the memory taken.
It also makes sure that the model indeed runs and that the forward is not bugged.


Add maintainers to recipe

Checklist

[x] Used a personal fork of the feedstock to propose changes
[ ] Bumped the build number (if the version is unchanged)
[ ] Reset the build number to 0 (if the version changed)
[ ] Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

@guarin I'm hoping this will automatically notify you and give you merge privileges once this is merged, but let me know if that's not the case.

@conda-forge-admin, please rerender


Tool crashes when input file extension is not .txt

Environment Information
Operation System: macOS Monterey
Node version: v16.17.0
Bug Description
The tool crashes when the -i option is used and the source file supplied is not in .txt format
See below:
[Error: ENOTDIR: not a directory, scandir '/Users/taimoordawami/Desktop/Silver Blaze.log'] {
  errno: -20,
  code: 'ENOTDIR',
  syscall: 'scandir',
  path: '/Desktop/Silver Blaze.log'
}

fix in #8


Project aliases and flags

Is your feature request related to a problem? Please describe.
I would like to easily be able to choose which project a command is being executed for
Describe the solution you'd like
In firebase-tools, you can create project aliases and use project flags. https://firebase.google.com/docs/cli#project_aliases
So you could create an alias "dev" and "prod" and then easily execute commands for each project: i.e. firebase deploy --only functions -P prod
Additional context
I like to leave dev as the default for security, and then explicitly say when I want to use prod
So a normal call to firebase deploy --only functions will use dev alias by default, and then I would have to explicitly do -P prod to deploy to production

There are currently 3 ways that cli supports for specifying an arbitrary project.

Pass in --project-ref flag
Set SUPABASE_PROJECT_ID env var (or using .env file)
Link a project with supabase link

If you don't specify any of the above, cli will prompt you with a list of projects to choose to. You can then filter them down by name, org, or ref.
I believe these methods can sufficiently fulfil the needs of project alias.


[FEATURE] - More Accessory types, choosable separately for each device, e.g. "window blinds" for IR-controlled projector screens or window blinds

Is your feature request related to a problem? Please describe.
I'm always frustrated when I must tell Siri to "turn on" the IR-controlled window blinds or the IR-controlled projector screen.
Describe the solution you'd like
It would be nice, if I could say "Hey Siri, window blinds down / up" or "Hey Siri, screen up / down" or "Hey Siri, window blinds 50%" and so on) - That would be possible, I guess, if I could only chose accessory type "window blinds" for the affected devices ...
Describe alternatives you've considered
It would be nice, if accessory type could be set freely and separately for any of the devices that the harmony hub offers, to any accessory type HomeKit offers for IR-controlled devices on the market.
(As are:
Lights (This is already implemented and choosable in HomeKit ✅)
Switches (This is already implemented and choosable in HomeKit ✅)
Outlets (There are IR controlled devices on the market)
Thermostats (I guess not necessary, as I've seen no such IR-controlled devices on the market)
Window Blinds (There are IR controlled devices on the market)
Fans (This is already implemented and choosable in HomeKit ✅)
Air Conditioners (There are IR controlled devices on the market)
Humidifiers (There are IR controlled devices on the market)
Air Purifiers (There are IR controlled devices on the market)
Sensors (I guess not necessary, as I've seen no such IR-controlled devices on the market)
Locks (I guess not necessary, as I've seen no such IR-controlled devices on the market)
Cameras (I guess not necessary, as I've seen no such IR-controlled devices on the market)
Doorbells (I guess not necessary, as I've seen no such IR-controlled devices on the market)
Garage Doors (There are IR controlled devices on the market)
Sprinklers (Don't know if there exists any IR-controlled devices)
Speakers (Don't know if there exists any IR-controlled devices)
Routers (I guess not necessary, as I've seen no such IR-controlled devices on the market)
TVs (There are IR controlled devices on the market))
Additional context
The ability to chose accessory type would also be handy for IR-controlled air conditioners, humidifiers, garage doors, etc. pp. ...
This would be a great improvement! ;-)

Hi,
i suggest you use shortcut app to do so, creating a shortcut with homekit accessory should work. The structure of the plugin, which is quite difficult to maintain (would need a full rework but I d'ont have time to do so), would make such a thing not impossible but difficult, since each type of accessory needs custom events handling / properties to set.


Clean up definition and use of the term "field" in schema reference and elsewhere

We use the term "field" too broadly, sometimes referring to anything in the blocks as fields. We need to be more specific about "field" versus "value" versus "enum", etc.
See thread here: https://prisma-company.slack.com/archives/C01TM7UM83Y/p1646385986791359

👋 we're cleaning up our backlog of issues and are closing everything older than two years. If you still feel that this issue is relevant, please feel free to re-open and our team will take a look. Thank you!


Live Preview

When editing a content item, it'd be useful for editors to see a live preview of their changes side by side to the deafult content editing UI, a la Craft CMS.

Anyone who saw the demo on Tuesday will agree that this will be an awesome addition, so I'm keen to get the ball rolling.
With that in mind, I've dumped all my thoughts out below. It'd be great to get some feedback:
Initial design overview:

Edit Content Item UI gets a Live Preview button added to it, which will toggle the Live Preview on or off
The Live Preview itself is nothing more than an iFrame that displays an action similar to the PreviewContent action, in the sense that is renders as an yet unpublished content item.
Client side events (such as changing the value of an input field) trigger the saving of the preview content item, and the refresh of the iFrame (so that the latest changes are displayed)

Considerations:

Editor and Live Preview side by side may be very wide (especially on content items with Layout part). Maybe allow for the iFrame to also be 'popped out' into a new window/ tab. We could also allow for the admin menu to be collapsed to provide more room.
The Live Preview should be re-sizeable so that content editors can preview their changes in a responsive manner.
Live Preview must not impact any currently published or draft versions of the Content Item.
Do we care about conditions? For example, any condition that requires the user to be logged out will never be true when in Live Preview (because the user will be logged in to the admin panel).
Is this a separate feature that needs to be enabled? Or a part than can be attached to Content Items? Or is it a new Meta Data setting on the Content Item?
Do we care about display types? ie will users want to preview their changes in both Detail and Summary (and potentially other) modes?

Possible Technical Implementation:

Update Content Manager API to allow for Content Items to be saved on the fly without overwriting either the Published, or Draft versions (maybe with a new LivePreview option on VersionOptions?).
Update ItemController to add a new Action that will render the Live Preview version of the content item.
Update content editing screen with required UI & JS (button to toggle on & off, JS to post back to save on change).

Additional consideration: what happens to this UI when viewed on a device with a narrow viewport. Maybe the live preview is switched to using some sort of tab in that case.

I'm also thinking it'd be good to provide the URL to Live Preview so that users can manually (or via a QR code) enter it into a mobile device to test on that too.

In CraftCMS you can request a custom url to see the draft, which then uses a nonce to protect it. Should be easy, nonce API is already there.


FactGenerator: Enable more clang-tidy checks

... and apply the relevant auto-fixes.

After merging, should create a new issue for enabling the following:
# -readability-convert-member-functions-to-static,
# -readability-inconsistent-declaration-parameter-name,
# -readability-magic-numbers,
# -readability-suspicious-call-argument,


Dev ruta

First, thanks for opening the PR. You leave the description empty, so can you tell us what you and your 3 firends want to improve?
In my opinion, I think rename README.md to README_original.md and replace the original README with some coverage test detail is a very unappropriate decision.

4 individual contributors are found on the page, but pyglet has 126 contributors. Where are they?

Then, I found the pyglet.math is modified to add some coverage test flags. You should separate tests from source code.
Finally, I found you improve some tests, this is great. But I'm not good at it. So other's suggestion is vital, too.


tablet_max_pending_versions changed not effect

Version: 2.5.3-46bf084
tablet_max_pending_versions set as 10000, I get the error as below after 1615 rows inserted continuously:
ERROR 1064 (HY000) at line 1: Too many versions. tablet_id: 221090, version_count: 1230, limit: 1000, replica_state: 0: be:mi
CREATE TABLE `insert_select_test` (
  `insert_time` datetime NOT NULL COMMENT ""
) ENGINE=OLAP
PRIMARY KEY(`insert_time`)
COMMENT "OLAP"
DISTRIBUTED BY HASH(`insert_time`) BUCKETS 10
PROPERTIES (
"replication_num" = "3",
"in_memory" = "false",
"storage_format" = "DEFAULT",
"enable_persistent_index" = "true",
"compression" = "ZSTD"
);

Your loading is too frequent. Did you load data by INSERT INTO xx values()?


SimulateSystem improvements

This PR improves Homebrew::SimulateSystem by making its methods more useful in a few ways. First, it replaces the macos? and linux? methods with treat_as_macos? and treat_as_linux? methods. The difference is that these methods are true/false based on both the simulated OS and the actual OS (before it only accounted for the simulated one). For example, if I'm running on macOS and run Homebrew::SimulateSystem.os = :linux, then treat_as_linux? will be true because I want to treat my current system as a Linux system. If I'm on macOS and haven't set a simulated OS, then treat_as_linux? will be false and treat_as_macos? will be true (since I want to treat my current system as a macOS system like it actually is). These methods are designed to be a single call to determine how to treat the current system without needing to deal with the confusing combination of checks you'd need otherwise.
Also, this PR adds current_arch and current_os to return a symbol representing the current OS and arch, regardless of whether this comes from a simulation or is the actual value. For example, if I'm on ARM, current_arch will return :arm. If I then run Homebrew::SimulateSystem.arch = :intel, current_arch will now return :intel.
As always, I'm open to suggestions on the names of these methods.

I've switched the OnSystem module to use these new methods which makes them a bit cleaner. I've also extended the use of SimulateSystem to uses_from_macos and ignore_missing_libraries.

Review period will end on 2022-07-26 at 00:00:00 UTC.

Would love to try and move more stuff to extend/os somehow but that shouldn't block this PR and could be a cleanup effort when the project is otherwise mostly done.

Currently, I've been moving things out of extend/os since we can't really de-require and re-require these files during runtime when we are simulating other OSes. So the undef and re-def system doesn't work great. Any suggestions for how to handle that while keeping the logic separate? If so, I agree that feels cleaner. I may be able to play with it more eventually, but I agree it seems like a cleanup thing that we can do a little farther down the road

Any suggestions for how to handle that while keeping the logic separate?

Maybe relying on global instances of classes/modules rather than class/module-level functions? It's relatively easy to e.g. create a new global instance.
Alternatively, and this would likely be a huge pain, allow passing in an OS object or something to a Formula which is used for all its querying.

Maybe relying on global instances of classes/modules rather than class/module-level functions? It's relatively easy to e.g. create a new global instance.

Can you give an example of what you mean by this?

Maybe relying on global instances of classes/modules rather than class/module-level functions? It's relatively easy to e.g. create a new global instance.

Can you give an example of what you mean by this?

e.g. instead of checking OS.linux? we have some global Homebrew.os = OS.new and then do Homebrew.os.linux? and can redo Homebrew.os = OS.new at a later point.

e.g. instead of checking OS.linux? we have some global Homebrew.os = OS.new and then do Homebrew.os.linux? and can redo Homebrew.os = OS.new at a later point.

I'm not sure this solves the problem, though. Currently, in extend/os/software_spec we have this:
https://github.com/Homebrew/brew/blob/32fd23770541fd62c4029ed3f9d280dc231a81c6/Library/Homebrew/extend/os/software_spec.rb#L5-L9
Your suggestion as I understand it would change this file to something like this:
if Homebrew.os.mac?
  require "extend/os/mac/software_spec"
elsif Homebrew.os.linux?
  require "extend/os/linux/software_spec"
end

However, we still will only run the code in this file once (whenever we first run require "extend/os/software_spec"). Even if we change Homebrew.os later on, we still won't re-require this file so we'll never change which version of the SoftwareSpec module is the one that's required at the moment.

However, we still will only run the code in this file once (whenever we first run require "extend/os/software_spec"). Even if we change Homebrew.os later on, we still won't re-require this file so we'll never change which version of the SoftwareSpec module is the one that's required at the moment.

Yeh, I'm suggesting that anything like this that needs to vary behaviour based on OS needs to be done in a way that can be changed at runtime 😬. I'm not sure how practical that is at this point.

Review period ended.

It's been a few days and there hasn't been any more maintainer feedback, so if it's okay with you, @MikeMcQuaid, I'd like to merge this for now. There are definitely more improvements that can be made, but I think they're lower priority (and will require pretty substantial code changes) so I'd rather push those to the future.


[Backend] Create Admin Add Box Form

Backend task for https://github.com/ctc-uci/find-your-anchor-frontend/issues/17
Add route to add box to database.

/cib al admin add box form backend

Branch 7-al-admin-add-box-form-backend created!


Setup FitNet Project to Access Places API

Summary:
This pull request updates gym data by fetching information from the Google Places API. It adds functionality to fetch gym details, including price level, services offered, and availability of trainers, and updates the Gym model with this data. Additionally, it includes improvements for error handling, logging, validation, and indempotency.
Related Issue(s):
No related issues.
Changes Made:

Implemented a script fetchGyms.js to fetch gym data from the Google Places API.
Added logic to populate price, services, and trainers_available fields in the Gym model based on API data.
Improved error handling to gracefully handle network errors and API request failures.
Enhanced logging to provide informative messages during script execution.
Added validation to ensure gyms with missing or invalid data are filtered out before insertion.
Utilized indempotency by using the place_id as apiId for gym data.
Updated the Gym model to reflect changes and ensure compatibility with fetched data.

Testing:
Not yet done. Ran into an error with the index.js I require backend assistance
Screenshots (if applicable):
No screenshots available.
Checklist:

[x]  Code follows project coding standards and guidelines.
[ ]  Changes have been tested thoroughly and pass all relevant tests.
[ ]  Documentation has been updated to reflect the changes made.
[ ]  All commits are atomic and have meaningful commit messages.

ERROR at /config/index.js line 36
I'm not too sure on what the code is supposed to achieve, hence why I've commented out the code. Seems like you imported the wrong file though, but would really love if you could look into the code, and clarify it so we can merge. @Ezekiel257

So i took a look at this. I was just studying sequelize and trying to setup myself up for work. Not even sure what was the aim of that. Look at the infrastructure, this has no use. The Index.js on the backend itself is the appropriate one. Can this be removed from the entire code base?

ERROR at /config/index.js line 36
I'm not too sure on what the code is supposed to achieve, hence why I've commented out the code. Seems like you imported the wrong file though, but would really love if you could look into the code, and clarify it so we can merge. @Ezekiel257

So i took a look at this. I was just studying sequelize and trying to setup myself up for work. Not even sure what was the aim of that. Look at the infrastructure, this has no use. The Index.js on the backend itself is the appropriate one. Can this be removed from the entire code base? @Benedict-arowo

I've removed it, and adjusted the main one a bit to fit our use cases.


probes falling back to proc parsing due to late eBPF events

We see this:
<probe> ERRO: 2017/06/27 05:02:25.336993 tcp tracer received event with timestamp<PHONE_NUMBER>49191 even though the last timestamp was<PHONE_NUMBER>49504. Stopping the eBPF tracker.

after running probes for hours, even days.
The message is produced by the logic introduced in #2334, even though we are running Ubuntu with a 4.4.0-81 kernel, which should not exhibit the problem described in that issue.
@alban any idea how we might go about tracking this down?
Also, as a workaround, is there any downside to simply restarting all the eBPF tracking when getting this error, i.e. going through the same initialisation as when initially starting the probes, including the initial proc walk? We'd need to guard against "flapping", obviously.

@alban has suggested offline:

if it is reproducible, I would add logs to print the cpu number for both events..
I would try to reproduce an issue with the tcptracer-bpf test program (without scope) in the dev environment with your kernel, using a script like https://github.com/weaveworks/scope/pull/2507#issuecomment-300735719 with different cpu# in different terminals

Could this be due to https://github.com/iovisor/gobpf/issues/42 ?

@rade How many cpus do you have on the machine where the bug was produced?
I am currently trying to reproduce this on GCE with this config:
$ uname -r
4.4.0-81-generic
$ grep PRETTY /etc/os-release
PRETTY_NAME="Ubuntu 14.04.5 LTS"
$ cat /sys/devices/system/cpu/{possible,present,online}
0
0
0

# uname -r
4.4.0-83-generic
# grep PRETTY /etc/os-release
PRETTY_NAME="Ubuntu 16.04.1 LTS"
# cat /sys/devices/system/cpu/{possible,present,online}
0-127
0-15
0-15

FYI, on our dev cluster of seven machines, all the probes were started on 2017/06/29 21:51. Four of them are still running in ebpf mode now, the other three encountered a timestamp discrepancy at

2017/06/30 07:50:49.966603
2017/07/01 22:42:24.916713
2017/07/04 02:03:31.517207

respectively.
So this error does appear to be quite rare.

I can reproduce the bug using tcptracer-bpf/tests/tracer in about 1 minute execution with the following:
n1-highcpu-16 (16 vCPUs, 14.4 GB memory)
$ uname -r
4.4.0-81-generic
$ grep PRETTY /etc/os-release
PRETTY_NAME="Ubuntu 14.04.5 LTS"
$ cat /sys/devices/system/cpu/{possible,present,online}
0-15
0-15
0-15

(I added more vCPUs)
I start nginx with sudo docker run -d  nginx and run the test script with:
$ cat test/test.sh
#!/bin/bash
cpu=$1
for i in $(seq 1 10000) ; do
  echo -n "$i "
  for j in $(seq 1 500) ; do
    taskset --cpu-list $cpu wget -O /dev/null http://<IP_ADDRESS> 2>/dev/null
  done
done
echo

I open several terminals and run test/test.sh 0, test/test.sh 1,test/test.sh 2 etc. to load several cpus. I can reproduce the bug when there is at least 3 test scripts running in parallel.

great! should be easy to fix then ;)

I cannot reproduce this on my laptop but only on GCE with the configuration above. Trying to see what the difference could be, I notice that GCE has a clock skew daemon but disabling it didn't help.
I wrote a program (bpf_clocks) to compare the kernel clock bpf_ktime_get_ns() and the userspace clock clock_gettime(CLOCK_MONOTONIC) since they need to be the same for the ordering to work. I tested a couple of time and both clocks seemed to be the same. I want to automate this test a bit more to run it plenty of times over a couple of minutes.
Next thing to check is if the clock is consistent between cpus or if one cpu can have a different clock than another. I suspect each cpu can give different results because of two comments I read:

"other CPUs are likely to be able observe [time going backward]" in __ktime_get_fast_ns
"There is no promise that the timestamp counters of multiple CPUs on a single motherboard will be synchronized" (source)

I am not sure how to write a test to check if it is the cause of the problem. But that's what I would like to check next.

@alban do you want me to give you access to one of our clusters where the problem occurs?

@rade no need for now because I can reproduce the bug without.
Status update: still exploring...
I continued with my bpf_clocks program to compare the kernel and userspace clocks and looked at more instances and I didn't notice discrepancies between the clocks.
I wrote another program to look at the clocks between cpus based on https://github.com/karthick18/ticket_spinlock (with this patch) and I also didn't notice discrepancies.

I am exploring a different possible race: the execution of one eBPF function (such as kprobe__tcp_set_state()) is not instantaneous and there could be some time between the time it takes a timestamp with bpf_ktime_get_ns() and the time the perf event is sent to the ring buffer with bpf_perf_event_output().
With virtual CPUs on platforms like GCE and AWS, there is no guarantees that a CPU is not suspended by the hypervisor, so different execution of eBPF functions can take variable amount of time. We could have the following scenario:

cpu#1 takes a timetamp
cpu#2 takes a timestamp and sends the event with the timestamp to the ring buffer
userspace checks the clock with clock_gettime() and reads the event from cpu#2.
cpu#1 sends the event to the ring buffer
later, userspace receives the event from cpu#1 but it notices its timestamp is before the event from cpu#2 and it cannot reorder it because the event from cpu#2 has already been forwarded to the user since it was before the clock_gettime() barrier.

In order to check if this scenario really happens, I added a new type of event (named type TCP_EVENT_TYPE_CLOCK) emitted after each tcp connect, accept or close (https://github.com/kinvolk/tcptracer-bpf/commit/baa1e79d356c622222568e74fc49de9ee4e1a996). I am inspecting the logs. I see that between 2 bpf_ktime_get_ns(), there could be between 800ns to 2000ns. It is not that small compared to 2 tcp events (often ~50000ns)

When looking at the timestamps in the logs, I was not able to observe the race described in my comment above so not sure if it is a correct explanation.
I gave Ubuntu 16.10 with Linux 4.8.0-59-generic a try and the bug is still reproducible there. So it does not seem due to an old kernel.

I didn't reach a conclusion on the root cause so I will go on with the workaround:

Also, as a workaround, is there any downside to simply restarting all the eBPF tracking when getting this error, i.e. going through the same initialisation as when initially starting the probes, including the initial proc walk? We'd need to guard against "flapping", obviously.

That's quite a big hack but I don't have a better suggestion ...


Bootloader marks frames used for the ramdisk as MemoryRegionKind::Usable when using Uefi

Frames in which the ramdisk is stored may be reported as usable in a memory region, when using Uefi.
Tested on hardware (i9-13900K) and qemu.
Workaround:

get physical address range of ramdisk (should be contiguous)
prevent frames in this range from being used

Thanks a lot for reporting! I prepared a fix in https://github.com/rust-osdev/bootloader/pull/408.


Cover existing functionality with unit tests

Coverage of sign in / up; friendships may become more in demand as the project grows

Endpoints to get covered:

Tests should contain positive, negative scenarios
To investigate: database mocking necessity


Setters should return current object.

We should return current object from all setters to make them compatible with fluent interface (allow chaining).

@avrahamgoldman this looks good to me, any thought before we merge it?

Nope, go for it. @zaporylie thanks for the PR! Sorry about the super late response.

Alright. Merging it. @zaporylie thanks for the PR.


Display the build date in the editor and when starting the engine

This can be used to quickly see how recent a development build is, without having to look up the commit date manually. When juggling around with various builds (e.g. for benchmarking), this can also be used to ensure that you're actually running the binary you intended to run.
The date stored is the date of the Git commit that is built, not the current date at the time of building the binary. This ensures binaries can remain reproducible.
The version timestamp can be accessed using the timestamp key of the Engine.get_version_info() return value.
I've tested all possible scenarios and they all build successfully:

git in PATH, .git/ folder present (the only scenario in which VERSION_TIMESTAMP is not 0).
git not in PATH, git/ folder present.
git in PATH, .git/ folder not present.
git not in PATH, git/ folder not present.

Preview
$ godot --quit
Godot Engine v4.0.alpha.custom_build.f78de4d1a (2022-03-12 02:35:20 UTC) - https://godotengine.org
Vulkan API 1.2.189 - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 1080

Rebased and tested again, it works as expected.

Needs to be rebased once more for the Engine docs rework on that part specifically.

Thanks!


Support keyboard input for creating/editing pickups and series

I think it would be nice to support keyboard input for time selection. Unfortunately, quasar doesn't support it yet, so we would need to fiddle a bit.

Given that no user mentioned this as problem, this issue can be closed.


When underwater, attenuate scene colour to model outscattering of light

Status
Ready to merge
What & why
Right now when camera is deep underwater and close to the seabed, the seabed still appears bright and it looks weird. I believe this is the primary issue reported in #318 . This change multiplies the scene colour by a term based on camera depth to model light getting scattered out and diminishing with depth.
Without this change - cam deep underwater near terrain:

With this change - light diminishes naturally and will go to black if deep enough:

How
Multiplies scene colour by term based on camera depth to model light getting scattered out and diminishing with depth.
Testing
Ran in win64 editor. Ran win64 standalone build.
Issues and limitations
None that i'm aware of. I suspected a few potential issues but it seems to work very well.

Although it does work well for the sun, it also affects other lights. It wouldn't be possible to do a deep underwater scene with a torch for example.

Good point! Hadn't thought about that.
I'm not sure if there's a solution to that, as all the lighting is baked in at this point.
I guess an alternative would be to use a script that actually scales down the directional light intensity. Might be just crazy enough to work. I'll post back when I've tried it.

Would the idea be to have a script which reduces the intensity of directional lights based on their distance from the camera based on the depth-fog-density?

That would be an interesting test. Maybe just the sunlight (?) as assigned to the oceanrenderer. If you want to try it out feel free, otherwise i will later.

Just reread your question, no it would just be reducing the directional light intensity based on camera depth compare to sea level. So moving the changes in this PR from the hlsl into a c# script:
				// Darken the background to model light outscattered as it passes from sun/env through water to the background surface
				half camDepth = max(_OceanCenterPosWorld.y - _WorldSpaceCameraPos.y, 0.0);
				sceneColour *= exp(-_DepthFogDensity.xyz * camDepth * DEPTH_OUTSCATTER_CONSTANT);

Well think i found a pretty horrible unity bug. the tangent vector was using a transform node with the space set to Absolute World. i didnt think this mattered as its set to Direction so position should not influence it.

I guess what it is doing is a translation to convert from absolute world to world, and then treating the result as a direction, which meant it would shift around depending on camera location.
Setting the space to World seemed to be enough for me, but ill try it again at SRP level now.

Not sure if you decided to pick this up @moosichu , apologies if so, but i had a bit more time and had a stab at this.
I multiply down the primary light intensity, and also the ambient light intensity. Both are required to ramp down the light.
I'm not sure how i feel about changing the light in the scene, but it seems to work and i cant put my finger on why its not a good idea, so maybe it will work.
The one thing i dont love is having to read the fog values off the material, which means if the value on the material is changed it wont be picked up. I also don't love that it runs per frame but i guess thats ok.
The last thing is that if underwater is not enabled then we wouldnt want this to run, potentially adding yet another user configurable option. This can probably be eliminated by moving the code to UnderwaterEffect.cs. I'll look at this if we decide to proceed with this approach.
I'll test in the 'RPs next.

Having it as an option is a good idea since most weather assets override the sun values and they will have their own way of managing it. Is it possible to expose the multiplier as a property? Then we could have our own component in crest examples which applies the values to sun and ambient. This might work better for downstream too due to lighting differences.

Ah good idea, I'll try

Ok the mechanism i was using to scale down the ambient lighting - RenderSettings.ambientIntensity - does nothing at all in URP. I looked through the lighting code and didnt spot any reference to a scalar on the lighting.
In this case i'm inclined to stick with the original solution - just scale the background colour - and call not supporting local lights a limitation. Its a shame but i dont see a way around it. Let me know what you think.

It works for me but only when a Skybox Material is set and Environmental Lighting > Source is set to Skybox. You should see the Intensity Multiplier slider when the settings are correct.
Also, we will have to disable the fog when underwater. Either by disabling it, or countering it in the underwater curtain. HDRP (and SRP once it supports it) can simply use the volume framework to blend out the fog once deep enough underwater. Fog would affect the underwater effects regardless of our solution for outer scattering.
The last issue is to do with baked lighting. If the lighting is baked when the skybox exposure is set to one, the ambient probe will have some colour even when the intensity multiplier is set to 0. There is nothing we can do about that it seems until Unity supports realtime GI in SRP.
Also, if we expose the depth fog colour, as well as the multiplier, then users will be able to add scripts to their lights to get the right colour.
I'm happy to work on this if you want to work on other things?

Thanks for the dilligence
Interesting that we're using Gradient as the source in that scene, im doubtful whether this should be the case. i'm guessing we should probably just switch it to skybox.

For HDRP we would have to blend out (or blend in 0 exposure sky cos of default settings or disable those default settings) the atmosphere volume in addition to the directional light.

Ok makes sense
I think this would be really cool. Lets chat on discord about short-term priorities.

I made it a seperate component. I have confirmed that it works with URP.
I added a spotlight to the end of the camera. Let me know if you want it kept or not.
As for HDRP, I have worked out a solution which works. It just requires an additional global volume which we will blend in which overrides indirect lighting (and anything else we need) as zero.

I was thinking one thing we could add is a light intensity parameter to the ocean material which we could increase with depth to counter the unnatural change in lighting. It didn't make much of a difference though.

Tested on birp, looks really cool.
I'm not sure we should leave a spotlight on the camera by default, for perf and misc. I'll think about it. We can probably demo it in other ways, like little glowing red mines?

I didnt understand the parameter note. What is the unnatural change? In any case sounds like it didnt make an impact.

Would it be worth moving the changes in OceanRenderer into the underwater script? I think i'd vote for this - if we're going to add a new script might as well put all the changes there, perhaps?
There could conceivably be something else that cares about the depth multiplier in the future, but i dont know what that would be, and we could always move it back.

I'm not sure we should leave a spotlight on the camera by default, for perf and misc. I'll think about it. We can probably demo it in other ways, like little glowing red mines?

Sounds good. Done.

Would it be worth moving the changes in OceanRenderer into the underwater script?

Sure thing. Done.

I didnt understand the parameter note. What is the unnatural change? In any case sounds like it didnt make an impact.

We are reducing the lighting which affects the ocean surface and depth fog. I thought it would make the surface darken sooner than it should. But the impact was minor (I was sticking between 1 and 2 multiplier).
I also ported the out-scattering constant over.

Happy for this to merge when you are. Nice one

Your additions looks great. Merged


cant fully see section

hi! great script
I can't figure out how to fix this, I tryed almost everything.
skrollr cuts out the end of the section, so if my screen window is smaller than the content itself, it will cut it down.
forceHeight is set to false
I attach a screenshot cause my english is poor, and I'm not sure you will understand me if I just write.. :)
this is the end of the page and it prevents me to scroll more

this issue occours every time the window height is smaller than the content

is there anyway to solve this?
thank you
ps: those are not images - they are big texts

Screenshots are worthless if I don't have access to the source. I can't guess what's clipping the content.


Dependencies on a method name is not invoking

ALL software version info
param.version : 1.13.0
Description of expected behavior and the observed behavior
Following the tutorial in the official reference
https://pyviz-dev.github.io/param/user_guide/Dependencies_and_Watchers.html#dependency-specs
Under dependency specs, it is stated that a method can depends on another method by another method's name.
However, cb4 does not execute after invoking cb3
Complete, minimal, self-contained example code that reproduces the issue
# code goes here between backticks
import param

class C(param.Parameterized):
    _countries = {'Africa': ['Ghana', 'Togo', 'South Africa'],
                  'Asia'  : ['China', 'Thailand', 'Japan', 'Singapore'],
                  'Europe': ['Austria', 'Bulgaria', 'Greece', 'Switzerland']}

    continent = param.Selector(list(_countries.keys()), default='Asia')
    country = param.Selector(_countries['Asia'])

    @param.depends('continent', watch=True)
    def _update_countries(self):
        countries = self._countries[self.continent]
        self.param['country'].objects = countries
        if self.country not in countries:
            self.country = countries[0]

c = C()

class D(param.Parameterized):
    x = param.Number(7)
    s = param.String("never")
    i = param.Integer(-5)
    o = param.Selector(['red', 'green', 'blue'])
    n = param.ClassSelector(param.Parameterized, c, instantiate=False)

    @param.depends('x', 's', 'n.country', 's:constant', watch=True)
    def cb1(self):
        print(f"cb1 x={self.x} s={self.s} "
              f"param.s.constant={self.param.s.constant} n.country={self.n.country}")

    @param.depends('n.param', watch=True)
    def cb2(self):
        print(f"cb2 n={self.n}")

    @param.depends('x', 'i', watch=True)
    def cb3(self):
        print(f"cb3 x={self.x} i={self.i}")

    @param.depends('cb3', watch=True)
    def cb4(self):
        print(f"cb4 x={self.x} i={self.i}")

d = D()
d

Stack traceback and/or browser JavaScript console output
Screenshots or screencasts of the bug in action

Changing d.x works as expected but calling d.cb3 does not invoke d.cb4

This is not exactly what the doc says, as it adds that:

which means that it will now watch everything that method watches, and will then get invoked after that method is invoked

And this is exactly what happens in the code cell 128 above, cb4 has been marked to watch everything that cb3 depends on and is executed after cb3 is executed.

Do you think the docs could be improved? Or do you see that as a feature request?

I agree that it is reasonable to expect that the method would be invoked in this case, but the documentation already seems clear enough that it won't be.  I'd favor closing this unless there is a clear proposal for clarifying the docs. Implementing the behavior requested would also be reasonable, but would be a clear change in semantics that seems dangerous to make since it's been like this for so long.


Enable to use custom inference api which is deployed in external server; so that I can avoid downloading models to h2ogpt UI server and just pass input and get output from those api in UI; like azure open ai

I want to run h2ogpt just with inference api, without specifying basemodel name.
For example, I have my llama model deployed in external server and exposes api to inference it. Hence I want to consume it in h2ogpt as llm.
Python generate.py inference_api=https://host com/api/v1/llama/infer
Other example is that use HF inference api as is.

We support many inference servers like gradio, vllm, openai, azure, sagemaker, replicate, etc.  However, you'd have to add code to support some arbitrary endpoint.
If it's openai-like, then use vllm or vllm_chat tag.  But if it's some arbitrary endpoint, we wouldn't support OOTB.
If it's an endpoint that langchain already supports, it may be possible to add.
But you didn't specify the endpoint type.

Downloading models is not required if using inference server.  It doesn't even need to use the tokenizer from the base model.  If HF can't be reached.
E.g. you can pass not just --base_model but also --tokenizer_base_model and set the latter to some unknown model.  This will fail to be found, and the backup FakeTokenizer using tiktoken will be used.
E.g.
python generate.py --base_model=BaseModelNameRemoteUses --tokenizer_base_model=Foo/Bar --inference_server=vllm:<IP_ADDRESS>:5000

I have rest api with me and sure about what type of inference server it is.
Hence looking for the right cli arguments.  If any customisation is needed then which place I have make changes. As per my understanding I have to make changes in get_llm method but looking for some sample code to implement it in right way for these kind of scenario.

python generate.py --base_model=BaseModelNameRemoteUses --tokenizer_base_model=Foo/Bar --inference_server=vllm:<IP_ADDRESS>:5000

Using vllm: infront of my rest api getting error like bad request error authorisation header is correct but the token seems invalid

python generate.py --base_model=BaseModelNameRemoteUses --tokenizer_base_model=Foo/Bar --inference_server=vllm:<IP_ADDRESS>:5000

Using vllm: infront of my rest api getting error like bad request error authorisation header is correct but the token seems invalid

Here is my sample code which i written in get_llm method, it is working and able to get response from api. But thing is i couldn't see the output in UI. please correct if anything wrong in this.
Approach 1
class H2OBloomLlmProvider(TextGen):
    def _call(self, prompt: str, stop: List[str] | None = None,
              run_manager: CallbackManagerForLLMRun | None = None, **kwargs: Any) -> str:
        response = ''
        json_payload = {
            "inputs": prompt,
            "parameters": {
                "max_new_tokens": 128,
                "temperature": 1,
                "num_return_sequences":1,
                "do_sample": True
            }
        }
        try:
            model_response = requests.post(self.model_url, json=json_payload, verify=False,timeout=180)
            if model_response.status_code==200:
                if len(model_response.json())>0:
                    response = model_response.json()[0]['generated_text']
            else:
                raise Exception(f'Error in calling API {model_response.status_code}')
        except Exception as e:
            run_manager.on_llm_error(e)

        generations= [[Generation(text=response)]]
        run_manager.on_llm_end(LLMResult(generations=generations))
        return response

Approach 2
class H2OBloomLlmProvider(TextGen):

    def generate_prompt(
            self,
            prompts: List[PromptValue],
            stop: Optional[List[str]] = None,
            callbacks: Callbacks = None,
            **kwargs: Any,
    ) -> LLMResult:
        response = ''
        prompt_messages = " ".join([x.text for x in prompts])
        json_payload = {
            "inputs": prompt_messages,
            "parameters": {
                "max_new_tokens": 128,
                "temperature": 1,
                "num_return_sequences":1,
                "do_sample": True
            }
        }
        try:
            model_response = requests.post(self.model_url, json=json_payload, verify=False,timeout=180)
            if model_response.status_code==200:
                if len(model_response.json())>0:
                    response = model_response.json()[0]['generated_text']
            else:
                raise Exception(f'Error in calling API {model_response.status_code}')
        except Exception as e:
            pass

        generations= [[Generation(text=response)]]
        return LLMResult(generations=generations)

I'm not sure.  I have many cases of various inference server types in src/gpt_langchain.py.  I hope you can find something that is close and duplicate it.
langchain has  many ways to handle things.


Elasticsearch indices data source

Pull request to add an indices data source

@jdesnoes are you able to take a look at the CLA so we can move forward with this?

@jdesnoes are you able to take a look at the CLA so we can move forward with this?

Hello @tobio, I already signed the CLA the 18th of July, with my GitHub's username and email.
But it still looks unchecked on GitHub. And I just signed it again now but same.
Can you take a look? Thanks.

But it still looks unchecked on GitHub

I can see you've signed the CLA, however the email address used to author the commits here isn't associated with your GH account at all. If you can re-author the commits with the email shared as part of the CLA process (@so*ria.com) or any email that's linked to your GH account then it'll get matched up properly.

But it still looks unchecked on GitHub

I can see you've signed the CLA, however the email address used to author the commits here isn't associated with your GH account at all. If you can re-author the commits with the email shared as part of the CLA process (@so*ria.com) or any email that's linked to your GH account then it'll get matched up properly.

Thank you, that was the problem indeed, I fixed it and the CLA now appears signed.

Hello @tobio
I updated my PR with the last changes of the main branch.
Can you take a look and let me know if it's valid to be merged?
Thank you.

@jdesnoes sorry, I've been meaning to look at this in comparison to https://github.com/elastic/terraform-provider-elasticstack/pull/698 which moves the existing index resource to the plugin framework being used here. We should be able to re-use a lot of the model code in both of these PR's.
I think it makes sense to get this merged and then I can merge the two code bases together. Just a heads up, I think it makes sense to get both of these merged quite close together. This one may sit approved but unmerged for a little while whilst I get them both inline.
The actual code changes look good, thanks for adding this! Are you able to take a look at why the tests are failing?

@jdesnoes sorry, I've been meaning to look at this in comparison to #698 which moves the existing index resource to the plugin framework being used here. We should be able to re-use a lot of the model code in both of these PR's.
I think it makes sense to get this merged and then I can merge the two code bases together. Just a heads up, I think it makes sense to get both of these merged quite close together. This one may sit approved but unmerged for a little while whilst I get them both inline.
The actual code changes look good, thanks for adding this! Are you able to take a look at why the tests are failing?

Thanks for your answer @tobio
Indeed, considering the #698 PR purpose, there is some code that can be used in common with this current PR, mostly for the model.
Considering that, and to make the merge between both of those pull requests easier, I already updated a bit my code to make it more similar to yours, mainly the models and the schema.
I also fixed the unit tests.
So that, no problem if in the meanwhile this PR has to be sit approved but unmerged.

Thanks mate, we're working through the review process on the other one now, so we should get the both merged in the next couple of weeks.

Thanks again for your help and your review @tobio
You're definitely right, it makes sense to keep the name and the description of the official Elasticsearch API documentation.
Indeed, target looks the best option to cover all the cases as the search field name.
I just updated the pull request to take this in account, thanks.

I'll look at merging the duplicate code in a follow up PR. Thanks for taking the time to add this one in @jdesnoes!


Issue #000 feat: Added BatchEnrolmentSync action

Codecov Report

Merging #536 into course-batch-jobs will increase coverage by 0.29%.
The diff coverage is n/a.

@@                   Coverage Diff                   @@
##             course-batch-jobs     #536      +/-   ##
=======================================================
+ Coverage                32.29%   32.58%   +0.29%
- Complexity                1236     1243       +7
=======================================================
  Files                      170      170
  Lines                    15074    15074
  Branches                  2392     2392
=======================================================
+ Hits                      4868     4912      +44
+ Misses                    9469     9424      -45
- Partials                   737      738       +1

Impacted Files
Coverage Δ
Complexity Δ

.../ekstep/searchindex/processor/SearchProcessor.java
66.36% <0%> (+0.9%)
99% <0%> (ø)
:arrow_down:

...p/searchindex/elasticsearch/ElasticSearchUtil.java
47.94% <0%> (+4.93%)
52% <0%> (+3%)
:arrow_up:

...dex/transformer/AggregationsResultTransformer.java
100% <0%> (+100%)
4% <0%> (+4%)
:arrow_up:

Continue to review full report at Codecov.

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


fix Windows scaling issue

The webview window did not respond to display 'scaling' on Windows. The window was ok at 100% scale, but at 125%, 150%, etc., it was too small. This PR fixes that issue.

Thanks for doing this work!
I suggest taking a look at how we deal with version-dependent APIs in enable_dpi_awareness().
Also you may want to check the behavior in the following cases:

DPI setting changes while the app is running.
The window is moved to another monitor that has a different DPI setting.

See:

WM_GETDPISCALEDSIZE
WM_DPICHANGED

This method of checking for the function seemed straightforward. So, windows 10 & 11 users should be covered.
The window does scale during dpi setting changes while the app is running.
I have not verified correct behavior on a separate monitor (just don't have one on hand here). As we are just surgically changing the width and height, i would be surprised if this code changed the awareness related behavior (but i've been surprised before :) ). I can try to test at some point soon -

I have a few comments for the changed code:

It would be great if it could follow the same pattern as existing code.
It could be improved further by putting the new code in a reusable function.
It can't run on OS before Windows 10 because you're still statically importing GetDpiForWindow.
Doesn't compile with MinGW-w64 in an MSYS2 environment.

Thank you for the feedback. I need to especially focus on your third point to begin with. I have reproduced the problem with the mingw based failures - i don't understand that at this point, but i'll work on it -

Is this ready for another review?

Yes please. Hopefully this is much closer to being acceptable. I did test on windows 7 to be sure about your 3rd bullet, but i have not been able to double check scaling in a dual monitor situation. thanks -

Thanks for the update! Please see the result of my testing below.
Test cases

Case
Description
Expected

1
Start on primary display with 100% DPI.
Window not scaled.

2
Start on display with >100% DPI when primary display at 100%.
Window scaled up.

3
Start on primary display with >100% DPI.
Window scaled up.

4
Move window to displays of varying DPI.
Window scaled with display DPI.

5
Start on display with >100% DPI without calling *set_size().
Window scaled up.

Result for 5defd8c794dae26a1aad801faead10e31bac99f0 (master)

Case
OK?
Reason

1
yes
Window not scaled.

2
no
Window at 100% and only WebView2 scaled up.

3
no
Window at 100% and only WebView2 scaled up.

4
no
Window not scaled, WebView2 not scaled until manual window resize.

5
no
Window at 100% and only WebView2 scaled up.

Result for 40ab81e580c91ce6428d69fa4cee23f6d4b89121

Case
OK?
Reason

1
yes
Window not scaled.

2
yes
Window scaled up.

3
yes
Window scaled up.

4
no
Window not scaled, WebView2 not scaled until manual window resize.

5
no
Window at 100% and only WebView2 scaled up.

I forgot to test changing the DPI on the same monitor while the window is still on the monitor.

Updated the tables with the missing test case.

I'm open to merging this when test case 5 passes. 4 and 6 can be sorted out separately.

Can confirm that the program runs on Windows 7 although without window scaling. While I would like it to work on OS prior to Windows 10, it can be improved separately.

I hadn't really considered test 5 - good catch.
I think I'm getting different results for test 6. When i have an app open and change either the 'scale' or the 'display resolution', the app scales in the way we want. I'm running a windows 11 laptop -

I'll give it a try on Windows 11. The teste from before were with Windows 10.

Can confirm your result for test no. 6 on Windows 11 but the title bar doesn't scale.
100% -> 175%

175% -> 100%

I see the same with the title bar - i hadn't noticed.  Wow - so we have multiple layers of problems to solve. The win 10 vs 11 seems particularly odd to me -

If you are seeing all of this with some particular clarity, and feel like you have a solution in your head, i'm happy to let you take the reins and knock this out. I'm not in need of any 'credit'.
OTOH, i'm happy to keep hacking at this - i'm relearning some of my windows api skills - :)

All right, I'll take a look.

I submitted PR #982 which addresses everything so far except old OS support.

Awesome - thank you! I think this is a very significant improvement to the library -

Superseded by #982


Adding floyd warshall

This pull request change Floyd.cpp to floyd-warshall algorithm in the graph module. The floyd-warshall algorithm is used to find the shortest paths between all pairs of vertices in a weighted graph. This implementation provides an efficient way to solve the shortest path problem for all pairs of vertices.
1.Adding negative power ring judgement.
2.Improve the name---Floyd to floyd_warshall (both the function name and file name)
3.Some tiny improvements

Please use the given pull request format.

duplicate of #2708


V2 API Refactoring

All the major code changes are in the ParseRouteUtils.js file (i.e. parseDetailedRoute function).
V1 API returns the following:

departureTime: Date
arrivalTime: Date
startCoords: Coordinates
endCoords: Coordinates
boundingBox: Bounds
directions: [Direction]

V2 returns the following additional information:

travelDistance: Double // distance between start and end coords in miles
startName: String // starting location name
endName: String // destination name
totalDuration: Int // minutes between departureTime and arrivalTime
routeSummary: [RouteSummaryElement]

Each routeSummaryElement consists of:
{
    stopName: String,
    direction: RouteSummaryDirection,
    shouldStayOnBus: Bool,
}

Each routeSummaryDirection consists of:
{
    type: null || 'walk' || 'bus',
    routeNumber: null || Int,
}

type is null if the passenger arrived at the destination.
routeNumber will only an integer (i.e. routeNumber) if type is 'bus'.

Can you also squash the last few commits into the last one?
git rebase -i HEAD~4 and squash all of them except the oldest (it'll squash them into the oldest), and then push to this branch with git push --force


Update MAL Importer

More updates from MAL.

They keep adding schema.org, making our lives easier scraping them :dancer:


Missing namespace for package fails and error not in detail

When I installed harbor without creating the namespace, it just showed the package as "failed" with no detail. I had to look at the logs to see:
Package	Install failed	{"error": "upgrading helm chart generated-harbor: upgrading helm release generated-harbor: failed to create resource: namespaces \"harbor\" not found"}

This is a pretty common error and this should be in the detail.
generated-harbor     	            	3       	2022-11-03 20:44:10.588611992 +0000 UTC	failed  	harbor-helm-2.5.1-0d4e0476a740b48a232041597ded2031595d9409           	2.5.1

This was working for me in a recent test. May have been fixed by another PR.

Cannot reproduce


Blank Space in the Header acts as Button and redirects user to MY LINK page

Expected Behavior
The blank space in the header should not be clickable neither redirect the user to the mentioned URL. Such action should only occur when the user presses "MY LINK" button
Current Behavior
On clicking the header the user is redirected to the mentioned URL.
Steps to Reproduce
Please provide detailed steps for reproducing the issue.

Bring your mouse pointer close to the center of the webpage near the header
Click it
You will be redirected to the MY LINKS page

#184 fixes this in v2


style(traverse): declare the key const when needed

What kind of change does this PR introduce? (check at least one)

[ ] Bugfix
[ ] Feature
[x] Code style update
[ ] Refactor
[ ] Build-related changes
[ ] Other, please describe:

Does this PR introduce a breaking change? (check one)

[ ] Yes
[x] No

If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:

[x] It's submitted to the dev branch for v2.x (or to a previous version branch), not the master branch
[ ] When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
[ ] All tests are passing: https://github.com/vuejs/vue/blob/dev/.github/CONTRIBUTING.md#development-setup
[ ] New/updated tests are included

If adding a new feature, the PR's description includes:

[ ] A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Inside the traverse function, the keys variable is not needed to declare when the val is not an object. So I put the keys const declare in the object condition.

nice one


Microsoft.Build.Utilities.Core not found

Hi,
I'm getting the following error.
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: The "GenerateCodeFromAttributes" task failed unexpectedly.
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.Build.Utilities.Core, Version=<IP_ADDRESS>, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: File name: 'Microsoft.Build.Utilities.Core, Version=<IP_ADDRESS>, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.Reflection.RuntimeAssembly.GetType(RuntimeAssembly assembly, String name, Boolean throwOnError, Boolean ignoreCase, ObjectHandleOnStack type)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.Reflection.RuntimeAssembly.GetType(String name, Boolean throwOnError, Boolean ignoreCase)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.Activator.CreateInstance(String assemblyString, String typeName, Boolean ignoreCase, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes, Evidence securityInfo, StackCrawlMark& stackMark)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.Activator.CreateInstance(String assemblyName, String typeName)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.AppDomain.CreateInstance(String assemblyName, String typeName)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.AppDomain.CreateInstanceAndUnwrap(String assemblyName, String typeName)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at System.AppDomain.CreateInstanceAndUnwrap(String assemblyName, String typeName)
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at CodeGeneration.Roslyn.Tasks.GenerateCodeFromAttributes.Execute()
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: WRN: Assembly binding logging is turned OFF.
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: To enable assembly bind failure logging, set the registry value [HKLM\Software\Microsoft\Fusion!EnableLog] (DWORD) to 1.
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: Note: There is some performance penalty associated with assembly bind failure logging.
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018: To turn this feature off, remove the registry value [HKLM\Software\Microsoft\Fusion!EnableLog].
......\packages\CodeGeneration.Roslyn.BuildTime.0.1.57\build\CodeGeneration.Roslyn.BuildTime.targets(20,5): error MSB4018:

I have only Visual Studio 2017 RC installed, I think I do not have the 'Microsoft.Build.Utilities.Core, Version=<IP_ADDRESS>, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' in the GAC registered.
If you could make use of the nuget packges for 'Microsoft.Build.Utilities.Core' it would solve the problem I would guess.

GAC doesn't matter, the assembly is loaded through a relative path from within another folder in the same nuget package. The problem is nuget at some point changed how packages are unpacked so the relative paths are not correct anymore.
Originally nuget packages were unpacked local to every project. Sometime in VS 2015 they moved unpacked nuget packages to an external folder so multiple projects could share nuget packages. In VS 2015 you had to opt-in to that behavior, I suspect in VS 2017 its the default, breaking this library.
You can work around it by unpacking the nuget package manually and placing the missing file where the msbuild script expects it. But looking at the other issues in this repo the library probably still won't work with VS 2017 since the API changed.


Index comparison experiment

I reverted the pull request and added the commits manually to main in favor of a linear history.


templates: use yq's indentation style

https://github.com/lima-vm/lima/blob/2992029f479a60ae4f8b515c0a914dad07fbe967/pkg/yqutil/yqutil_test.go#L30-L55
Lots of users are expected to use --set (yq) soon, so I'd prefer to adopt yq's indentation style.
Revert:

https://github.com/lima-vm/lima/pull/512

I think the "style" of yq is mostly related to bugs in the go-yaml library, maybe they can be addressed ?

The idiosyncrasies here is that this document:
# comment
foo: "x"
bar: "y"
baz: null

map:
- foo: 1
- bar: 2
- baz: 3

Gets reformatted, when piped through yq as:
# comment
foo: "x"
bar: y
baz: null
map:
  - foo: 1
  - bar: 2
  - baz: 3

Even though the first document is valid YAML.
  2:1       warning  missing document start "---"  (document-start)
  7:1       error    wrong indentation: expected 2 but found 0  (indentation)

rules:
  indentation:
    indent-sequences: false
  document-start: disable

I think the "style" of yq is mostly related to bugs in the go-yaml library, maybe they can be addressed ?

Yes, please!

Or possibly the issue is with the gopkg.in/yaml.v3, it is hard to tell because it uses both (and then some)

I thought it was because of yaml.v3. The indentation change is a regression from yaml.v2, but the library is essentially unmaintained:
$ git log --pretty=format:"%h %ad %s" --date=short -5
f6f7691 2022-05-27 The Billion Dollar Mistake
00bbc09 2022-05-21 github: drop broken semgrep workflow
8f96da9 2022-05-21 Explicitly check the parser for errors on peek
539c8e7 2022-05-12 Fix decoding of merge tags.
496545a 2021-01-07 Improve inline comments on map keys (#656).

$ gh issue list

Showing 30 of 239 open issues in go-yaml/yaml
[...]

$ gh pr list

Showing 30 of 100 open pull requests in go-yaml/yaml
[...]

There are many issues and several PRs about this regression. kubernetes-sigs/yaml has decided to create a fork with the fix:
https://github.com/kubernetes-sigs/yaml/issues/72
But this is rather slow-moving:
https://github.com/kubernetes-sigs/yaml/pull/76
I was thinking about trying to use one of the existing forks for now, e.g. from
https://github.com/go-yaml/yaml/pull/924
But it may require a fork of the fork, to make the v2 behaviour the default, as we don't want to modify the yq code itself.
Note that according to the yq documentation it should be using the compact list form with -I2: https://mikefarah.gitbook.io/yq/usage/output-format#indent
But that is also broken due to the go-yaml regression:
https://github.com/mikefarah/yq/issues/825

The removal of empty lines might be a bug,

This is a separate problem from fixing list indentation. Strangely I couldn't find any open issues about this behaviour, but I didn't try very hard.

the indentation of bullet point lists isn't likely to change in a future release of yq.

Yeah, but we don't really care about yq itself, but about how our --set option will modify lima.yaml. So if we can get limactl to link with a modified version of go-yaml that fixes this regression, then we should still be good, even if yq releases continue to use the broken[^1] version.
[^1]: I know it is not really broken, it is just badly formatted.

I don't really like copying external libraries to internal, like kustomize did here. Better to use go mod replace.

if this forking path is taken (instead of changing lima indents).

replace() is often unmaintainable, so I'm wondering if we can just (temporarily?) accept yq's current implementation.

That is possible, can also wait for kyaml to appear...
One of the patches was actually totally unrelated, and only "worked" by rigging the test - so I think I will revert it. Too bad, I was hoping for it to fix the newlines in comments, for round-trip...

The newlines are treated as a separator (between comments), so they are not actually included in the comment (nor elsewhere)
That makes them missing in Node, and "disappear" when doing a round-trip yaml. It needs a similar boolean, to keep newlines in comments.

I'm wondering if we can just (temporarily?) accept yq's current implementation.

Why is this necessary? Users who use --set don't even see how lima.yaml is formatted, so if the indentation gets mangled at that point it is unfortunate, but won't affect most of them.
So we would preemptively mangle the formatting, just to keep the diff small for users that likely don't care or even notice?

Why

Not just for --set; I want to use yq (or something else similar) for automating bumping up the templates.
https://github.com/lima-vm/lima/issues/1347

I want to use yq (or something else similar) for automating bumping up the templates.

Thanks! I totally missed that, and I understand why this is necessary.
But it means we don't need to modify the yq code used by limactl; we could just have a fixed version of yq in a fork, and fetch the binary from its Github release page as part of the updater script.
And the fixed fork probably wouldn't have to be kept in sync with upstream yq changes either. Once it works, we can just keep it as-is for internal build purposes.