Use 'sieve' to check for distributed/concurrency related issues in kapp-controller

Describe the problem/challenge you have
It's very hard to prove the absence of bugs,  and classically this class of bugs is very hard to find/detect/reproduce
Describe the solution you'd like
luckily for us, some folks at vmware and UIUC are working on this "sieve" tool: https://github.com/sieve-project/sieve
Anything else you would like to add:
Note that sieve is marketed as still in very early stages;  imo this would be interesting to explore but if it's rocky we would have the options of reaching out directly for assistance and/or putting this thought onto the back-burner for a few months and coming back to see if the tool is more mature.
Note also that the sieve tool is described in the kube-con talk "Automated, Distributed Systems Testing for Kubernetes Controllers - Lalith Suresh, VMware & Xudong Sun, University of Illinois at Urbana-Champaign"

Vote on this request
This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.
👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"
We are also happy to receive and review Pull Requests if you want to help working on this issue.

Next step here would be to run sieve against kapp-controller and report issues surfaced in this issue. Maybe longer term this could be part of our CI process if we find the tool to be useful in surfacing issues. For now, we'll start by doing some initial research on the tool.
We may not get to this immediately, so we are open to contributions if anyone is interested.

@danielhelfand @joe-kimmel-vmw just saw this. We'd be happy to look into testing kapp-controller with Sieve in the coming weeks. (cc @embano1 @marshtompsxd)

Hi, @danielhelfand @joe-kimmel-vmw Thanks for your interest in Sieve.
We are currently working on porting and testing the kapp-controller using Sieve.
One necessary step in porting is to build the docker image from the source. However, I encountered some difficulties. I tried to use the Dockerfile in the repo to build the image by
docker build --no-cache -t xudongs/carvel-kapp-controller:latest .

But the building failed with the following error:
Step 19/38 : COPY . .
 ---> 86800275cf64
Step 20/38 : RUN CGO_ENABLED=0 GOOS=linux go build -mod=vendor -ldflags="-X 'main.Version=$KCTRL_VER' -buildid=" -trimpath -o controller ./cmd/main.go
 ---> Running in 969352fe57d4
go: inconsistent vendoring in /go/src/github.com/vmware-tanzu/carvel-kapp-controller:
<EMAIL_ADDRESS>is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
<EMAIL_ADDRESS>is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
<EMAIL_ADDRESS>is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
<EMAIL_ADDRESS>is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
        ......
        To ignore the vendor directory, use -mod=readonly or -mod=mod.
        To sync the vendor directory, run:
                go mod vendor
The command '/bin/sh -c CGO_ENABLED=0 GOOS=linux go build -mod=vendor -ldflags="-X 'main.Version=$KCTRL_VER' -buildid=" -trimpath -o controller ./cmd/main.go' returned a non-zero code: 1

Could you let me know how to address this error? Or did I build the image wrongly? If I am able to successfully build the image, I will finish porting and start testing the controller. Thanks!

@marshtompsxd  - Thanks for following up!
It's very interesting that you hit that error!  To be honest we don't usually invoke docker build directly - the build scripts that we run, and that are run in our github actions, all invoke kbld:
https://github.com/vmware-tanzu/carvel-kapp-controller/blob/develop/hack/deploy.sh#L5
the relevant subcommand there is ytt -f config/ | kbld -f- which renders the ytt templates and passes them through kbld, which in turn calls Docker build as configured by the templates.   Is it possible for you to use that flow to build the container?

Hi @joe-kimmel-vmw Thanks for the reply.
I tried the command and I think I managed to build the image. After the command succeeded I listed all the docker images:
REPOSITORY   TAG                                                                                IMAGE ID       CREATED          SIZE
kbld         kapp-controller-sha256-fac429e6eea664d64c864c3d8ece30d4406359926fbfefd3aaf4ba1d882d31be   fac429e6eea6   11 seconds ago   578MB
<none>       <none>                                                                                e1acdea340cb   30 seconds ago   1.47GB
<none>       <none>                                                                                f5d1ac9784b3   2 minutes ago    445MB
photon       4.0                                                                                b432359f4c98   4 days ago       36.9MB

I think the first one should be the controller image.

BTW, is it possible to specify the tag of the controller image when using the above command to build? I assume the kbld is the controller image the long tag seems to be random.

Thanks @marshtompsxd - very excited you got it to work!  Please let us know how we can support your investigations.
to your question

is it possible to specify the tag of the controller image when using the above command to build?

as you saw, kbld really wants to lock down a specific SHA for reproducibility. However, you can provide multiple tags to docker via the command line. in our case you would add the tag to the rawOptions list documented here
We're still confused also by your original error, as we would expect docker build to just work.  However if you're past it that's great -  thanks for your efforts and again let us know how we can support you!

Hi @joe-kimmel-vmw , thanks for your help and sorry for the late response.
We have ported the kapp controller with Sieve, but unfortunately the current version of Sieve cannot test it because kapp controller does not issue updates to the k8s cluster (Create, Delete, Update, Patch) through the controller-runtime APIs -- Sieve assumes all the updates go through the controller-runtime APIs.
To address this, we are currently implementing a new feature to make Sieve more generalized and independent from controller-runtime. We will let you when the feature is implemented and the test results.

hey @marshtompsxd just following up here (I know its been a fairly long period of time since last comment) - did you all manage to get that feature implemented? We would still like to run sieve against kapp-controller, so any further updates would be really helpful! Thanks

Hi @neil-hickey Thanks for following up!
Yes, we have already implemented the feature and now Sieve has no dependency on controller-runtime. We are working on porting kapp-controller right now.

Thanks @neil-hickey for the pointers! That should be a good start for us. I'll keep you posted how our testing goes.

Posting a quick update here. I ran a test workload with kapp-controller (deploying guestbook-go). Sieve automatically explored 9 test plans involving crash safety but did not report any issues. On closer inspection I see that most of the interaction with the k8s API is driven by the kapp binary and not kapp-controller. Our sieve instrumentation tool was only instrumenting kapp-controller, so missed out on the interaction between kapp and k8s API. I'm looking at extending sieve instrumentation to kapp as well to see if it can detect any bugs.

Nice! Yes, mostly kapp-controller looks at the CRDS and invoke a cycle of fetch, template, deploy where deploying is the most expensive and is done via kapp


fix(dashboard blocks): archive old transactions properly, some data of past few days is missing

Archive old mempool txns, properly. Currently it has implemented LRU cache, but some data of past few days is missing upon clicking the new window again

RCA -> below point is causing issue
https://github.com/isaacs/node-lru-cache#ttl

Max time to live for items before they are considered stale. Note that stale items are NOT preemptively removed by default, and MAY live in the cache, contributing to its LRU max, long after they have expired.


Readme improvments

Hey there! I've taken a shot at making the README a bit clearer, friendlier, and more encouraging. My goal was to foster a stronger sense of community and make the instructions and guidelines as easy to follow as possible.

Our primary objective here is to make sure people don't blame us when they install a CSS theme that messes things up.

Understood  👍


Ignore errors if dirty data exists in code_item

When I try to parse and remove the hardened APK, running into DalvikCode will result in an error where unpack requires a buffer of 2 bytes. This is because AndroGuard resolves all code_items according to map_list, whereas the hardened APK internals are dynamically repaired and indexed by address. May result in dirty code_item data, causing AndroGuard parsing to fail and stop. Therefore, the wrong code_item needs to be ignored
Ignore errors if dirty data exists in code_item

Ping :)

Given that this PR is old and the project is substantially different nowadays, I will be closing this.
Please feel free to reopen it in case it still stands.


update test case and user guide

update test case and user guide

Coverage remained the same at 55.228% when pulling f035136fa71a762c4502b8925ddba7578c13d420 on sigsbee:dev into ad02cab82a1b96151c276c82f5f9a77a591635d2 on vmware:master.

close for separate  pr


Error in plotting

I am trying to simulate the example "two_parametrons.ipynb" in the [https://github.com/NonlinearOscillations/HarmonicBalance-notebooks]. I am facing the issue with the subplots function.
f,ax = HarmonicBalance.subplots(1,5,figsize=(4*5,4))
HarmonicBalance.plot_1D_jacobian_eigenvalues(soln_1d, x="ω/2", physical=true, stable=false,filename="plot_1D_jacobian",ax=ax);

I am getting the following error UndefVarError:subplotsnot defined Stacktrace:  [1] getproperty(x::Module, f::Symbol)    @ Base .\Base.jl:31  [2] top-level scope    @ In[61]:1
################################
Also for coupled parametrons, its not generating the plot against the following commad.
HarmonicBalance.plot_2D_phase_diagram(result_coupled, stable=false);

Hey @hk06319, the notebooks you are referring to are quite old and outdated. We are in the progress (see #221) of updating these notebooks. That said could you try
plot_phase_diagram(result_coupled)

Hey, we know have a new website where we better examples and tutorials. Feel free to check it out :)


[storage][dashboard] add estimated tree size with prune window counter

Motivation
compare the theoretic size of jellyfish merkle and the data from rocksdb.

@bors-libra land


fix(test): Remove failing compatibility tests

Description
3.74 and 4.1 are both out of support and are both affected by the issue where network entities are too large so remove them from the version compatibility matrix.
Checklist

[ ] Investigated and inspected CI test results
[ ] Unit test and regression tests added
[ ] Evaluated and added CHANGELOG entry if required
[ ] Determined and documented upgrade steps
[ ] Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.
Testing Performed
Here I tell how I validated my change
Version compatibility test
Reminder for reviewers
In addition to reviewing code here, reviewers must also review testing and request further testing in case the
performed one does not seem sufficient. As a reviewer, you must not approve the change until you understand the
performed testing and you are satisfied with it.

/test

/test gke-version-compatibility-tests

/test gke-version-compatibility-tests

/test gke-version-compatibility-tests

/test gke-version-compatibility-tests


Molecule fails installing ansible roles from requirements.yml using git+ssh

Issue Type

Bug report

Molecule and Ansible details
ansible --version && molecule --version

ansible 2.7.1
  config file = None
  configured module search path = [u'/home/molecule/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.15 (default, Aug 16 2018, 14:17:09) [GCC 6.4.0]

molecule, version 2.19.0

Molecule installation method (one of):

docker image

Ansible installation method (one of):

docker image

Detail any linters or test runners used:
None
Desired Behavior
The Docker image quay.io/ansible/molecule is missing the openssh-client package which prevents us from installing Ansible roles from private git repositories over ssh. This action is failing even though you have the ssh keys are present.

Validation completed successfully.
--> Test matrix

└── default
    └── dependency

--> Inventory /tmp/ansible-role-meetup/molecule/default/../inventory/group_vars/ linked to /tmp/molecule/ansible-role-meetup/default/group_vars
--> Scenario: 'default'
--> Action: 'dependency'
 [WARNING]: - ansible-role-common was NOT installed successfully: - command
/usr/bin/git clone git clone git@bitbucket.org:<teamname>/<rolename> <rolename> failed in directory /home/molecule/.ansible/tmp/ansible-local-
9ViBtYx/tmpD_FpFB (rc=128)

ERROR! - you can use --ignore-errors to skip failed roles and finish processing the list.

In more details here is the exact git error:
docker run --rm -it \
  -v /Users/florian/Workspace/gumgum-ansible-roles/ansible-role-meetup:/tmp/ansible-role-meetup \
  -v /var/run/docker.sock:/var/run/docker.sock -v /Users/florian/.ssh:/home/molecule/.ssh \
  -w /tmp/ansible-role-meetup \
  --volumes-from=ssh-agent \
  -e SSH_AUTH_SOCK=/.ssh-agent/socket \
  quay.io/ansible/molecule:latest bash

bash-4.4$ git clone git@bitbucket.org:<teamname>/<rolename>
Cloning into 'ansible-role-common'...
fatal: cannot run ssh: No such file or directory
fatal: unable to fork

https://github.com/ansible/molecule/pull/1604 is merged.


Models dict

adds braindecode.models.util.models_dict

Can you explain a bit what this does? And maybe add such explanation in the comments. Looks quite magic to me at the moment haha :)

I added a small comment explaining the filling logic @robintibor. It is a for that is executed when loading the library that will allow you to access the list of models and is updated dynamically.

@robintibor Haha it’s the same as for the dataset_list in MOABB
@bruAristimunha Thanks!

Thank you Pierre!


Support typeof this for cloning

Looking for the issue, I found multiple references to this, but all of them lead to this issue which is closed and marked as "fixed" while the problem remains (or at least the solution is not documented.
Search Terms

Clone
typeof this
cloning
Suggestion

It should be possible to write something like:
interface A {
  clone (): typeof this
  }
}

The expected result would be that X.clone() is inferred to typeof X when X implements A, and any implementation of A should define a function clone that returns an instance of this type.
Use Cases

This is very useful for cloning objects, but also for any object method that should return an instance of that same object.
Examples

For interface:
interface A {
  clone (): typeof this
  }
}
class Impl implements A {
  // Implementing A should force any implementation to define the method and return an implementation of this class
  clone (): typeof this {
     return new Impl()
  }
}

const a = new Impl()
console.log(typeof a.clone()) // Impl

For inheritance
class Container {
  content: number[]
  constructor(content: number[]) {
    this.content = content
  }
  extract(start: number, end: number): typeof this {
    return new Container(this.content.slice(start, end)
  }
}

class BetterContainer extends Container {
  superProp: string
  constructor(content: number[], superProp: string) {
    super(content)
    this.superProp = superProp
  }
  // Using `typeof this` as a return type in a class or interface should be considered as using `abstract` and force any inheritant to redefine the method.
  extract(start: number, end: number): typeof this {
    return new BetterContainer(this.contentslice(start, end), this.superProp)
  }
}

const container = new BetterContainer([1, 2, 3], 'test')
const extract = container.extract(0, 1).superProp // Should work

Checklist
My suggestion meets these guidelines:

[x] This wouldn't be a breaking change in existing TypeScript/JavaScript code
[x] This wouldn't change the runtime behavior of existing JavaScript code
[x] This could be implemented without emitting different JS based on the types of the expressions
[x] This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
[x] This feature would agree with the rest of TypeScript's Design Goals.

You can do this today. Given no discussion of how it doesn't fit your criteria I'm assuming you didn't know about it.
interface A {
  clone(): this
}

class C implements A {
    clone() {
        return this;
    }
}


Upgrade opentelemetry_proto to v0.19.0

Commit Message: Upgrade opentelemetry_proto to v0.19.0
Additional Description: This release had some breaking changes, notably deprecating instrumentation_library_logs and instrumentation_library_spans in favor of scope_logs and scope_spans, respectively. This PR bumps the version and updates references to those fields in the OTel access logger and the OTel tracer and their tests.
Risk Level: Low
Testing: Updated tests
Fixes #22559
+CC @moderation  and hat tip for the starter changes!

/lgtm deps


Audit profiles array deprecation support

Description
Patch to support both array of hashes & hash of hashes profiles format.

Added deprecations warning if profiles added by re-assign with array of hashes or push methods.
Extended default profiles object, so that it will support new format(hash of hashes).
Constructed final array of hashes while adding profiles so that no need to parse the profiles in audit report.

Issues Resolved
Fixes: https://github.com/chef-cookbooks/audit/issues/339
Check List

[ ] All tests pass. See https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/TESTING.MD
[ ] New functionality includes testing.
[ ] New functionality has been documented in the README if applicable
[ ] All commits have been signed for the Developer Certificate of Origin. See https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/CONTRIBUTING.MD

Hey @lamont-granquist,
I like this approach, this is going to be a much simple solution to implement such deprecations.
But about the default['audit']['profiles'] which are already set for a node on the server and are not provided by the user while new chef-client run, could you please let me know if I'm missing something while covering this crucial case:

(there should be no merging and both-set should throw a warning that the old array setting is not being used.

We should provide backward compatibility to these preloaded default['audit']['profiles'], and also load(merge) them along with default['audit']['profiles-ng'] followed by deletion of default['audit']['profiles']. later release will throw errors and fail if default['audit']['profiles'] is set.

I think @clintoncwolfe should make the call.  My impression is that this isn't highly used, so providing merging backcompat is not an additional requirement.

So I keep thinking this over, and still what we're trying to do here is construct an object which is both Array-like and Hash-like where we attempt to determine which one it should be by the pattern of access by the user which is, in a strong logical sense, non-sensical in ruby.
I don't think we can do this.
It is made actually worse by the fact that the blast radius is everyone that uses the cookbook.
What needs to happen is this needs to follow the pattern of using another attribute (good old filesystem-vs-filesystem2).
We do not need to drop support for Hashes coming from policyfiles.  Also users can go ahead and just blow away the default Array themselves manually and continue to use it.
But for users that need to 'convert' from Arrays to Hashes I think they need to use a profiles2 object, then at some point in the future a major version drops which drops Array support from profiles and then eventually profiles2 can be dropped.  But that's the only way to run Arrays and Hashes side-by-side coherently.
The other alternative is to just drop a new major version and as part of upgrading to the new major version that attribute must be fixed by the user or they're blocked on the upgrade.  The N-1 is the Array and N+1 is the Hash and there's no dancing around profiles-to-profiles2-and-back.
It also may just be better to document that users should take the upgrade path of manually blowing away the Array with a Hash in their wrapper cookbooks and just do a docs upgrade.
Actually I kinda like that last one.  Fix the docs.
Don't do this.

Okay so the best way I can think of to actually make this work is to more or less leave the HashProfile mixin there, so that object hash special behavior so that if I user writes default['audit']['profiles'] = [] or default[:audit][:profiles] = [] then we need to give a warning.
But then we need to initialize default[:audit][:profiles] earlier with a decorator object.  And really that object needs to make an early decision about if it is really an Array or a Hash -- then it needs to use the node.__path information in order to actually rewrite itself out of existence in the node object and replace itself with an actual ImmutableArray and/or ImmutableMash object.
This can still be attacked any number of ways though since before you have an access that can clearly delineate if the object is an Array or a Hash what do you do if someone calls .is_a?(Hash) on the object?  It could be, we don't know yet...

Hi @lamont-granquist  @btm,
Thanks for the valuable feedback and will try to cover all edge cases and let you know.
Actually, I was under the impression to provide support only to those methods which are given in the README docs ie. .push.

Hi @lamont-granquist & @btm
sorry for the late reply!
Tried to catch some of the edges cases and added a warning for the all Array mutator methods.
Could you please have a look?
Thanks

Rather than the complexity here of trying to support both arrays and hashes, I believe what we want to do is the clean break of making a new major release of the audit cookbook that requires only using hashes. Users will have to switch to hashes when they upgrade.
We'd like to have a less abrupt migration path but I don't think anyone is comfortable with this complexity.

Thank you for the contributions. v9 of the cookbook uses profile hash by default based on this change:
https://github.com/chef-cookbooks/audit/pull/385
Ok to close this PR?

Yup @alexpop go ahead and close it. thanks

Appreciate the quick reply Vivek. Closing. Thank you!


[Feature] Enhancing Efficient Filters(in Faiss Engine) for Restrictive Filtering use case

Introduction
This document proposes the enhancements for Efficient Filters for Faiss Engine for Restrictive Filtering use case. Restrictive Filtering is the case where you have lets say 100K documents in graph but filter reduce the size of the search space to like 1000 documents or less than that.
META Issue for filtering Feature: https://github.com/opensearch-project/k-NN/issues/903
Background
While doing the Efficient Filtering for K-NN query we do follow the below step for each segment of a shard in OpenSearch.

Do a constant score query in via Lucene and get the DocIds.
If Length of Filtered DocIds <= K then we do the exact Search, else we do the ANN Search. The reason for doing this because to get K nearest neighbor you need to do at-least K distance calculation. Hence switching to exact search make sense in this case.
While doing the exact search, we read the doc values(which is byte representation of Vectors) convert them to array of floats and then use the Lucene distance calculation functions to calculate the distance between the query vector and indexed vector. After that we return the Top K docIds for that segment.

Problem
The problem with the logic defined above in #2 is, that if K is small and filtered Ids is large but total docs present in the segment is even larger than the Recall drops. Example: Let’s say we ingested 1M documents with 24 shards in an index and then we do a query with K=100, and FilteredIds is 2K but segment contains like ~41K document then the recall drops. We saw this in our experiments for 2.9 OpenSearch release.
{
    "metadata": {
      "test_name": "Faiss HNSW Restrictive Filter Test",
      "test_id": "Faiss HNSW Restrictive Filter Test",
      "date": "07/20/2023 20:52:31",
      "python_version": "3.8.17 (default, Jul  5 2023, 21:04:15) \n[GCC 11.2.0]",
      "os_version": "Linux-6.1.34-59.116.amzn2023.x86_64-x86_64-with-glibc2.17",
      "processor": "x86_64, 36 cores",
      "memory": "920506368 (used) /<PHONE_NUMBER>4 (available) /<PHONE_NUMBER>8 (total)"
    },
    "results": {
      "test_took":<PHONE_NUMBER>99801,
      "delete_index_took_total":<PHONE_NUMBER>9959996,
      "create_index_took_total":<PHONE_NUMBER>0200885,
      "ingest_multi_field_took_total": 60584.180823600036,
      "refresh_index_store_kb_total": 2993302.6438476564,
      "refresh_index_took_total": 37546.422699699906,
      "force_merge_took_total":<PHONE_NUMBER>6769945,
      "query_with_filter_took_total": 58514.0,
      "query_with_filter_took_p50": 5.0,
      "query_with_filter_took_p90": 6.0,
      "query_with_filter_took_p99": 6.7,
      "query_with_filter_took_p99.9": 9.4,
      "query_with_filter_took_p100": 1128.0,
      "query_with_filter_client_time_total": 94834.7,
      "query_with_filter_client_time_p50": 8.7,
      "query_with_filter_client_time_p90": 9.5,
      "query_with_filter_client_time_p99": 10.5,
      "query_with_filter_client_time_p99.9": 199.4,
      "query_with_filter_client_time_p100": 1133.0,
      "query_with_filter_memory_kb_total": 1297436.0,
      "query_with_filter_recall@K_total": 0.18980632427320202,
      "query_with_filter_recall@1_total": 0.4012699802934092
    }
}

In vector search space a recall of 18% at K is really bad. We want to improve the recall to be > 0.95.
The same case happened for one of the customer: https://github.com/opensearch-project/k-NN/issues/1043
Proposed Solutions
Option 1
The proposal to solve the problem is to provide customers a capability via settings for an index where they can define a threshold(used at a segment level) below which if filtered ids are present we will always do Exact Search instead of ANN along with if filteredIds <= K. I did the experiments(Refer Experiment Section) and found out 2K is a good default number up-to 4096 dimensions.
PUT /hotels-index
{
"settings": {
    "index": {
      "knn": true,
      "knn.algo_param.ef_search": 100,
      "knn.advanced.filtered_exact_search_threshold_pct" : <Threshold_Value_in_pct>,
      "knn.advanced.filtered_exact_search_threshold" : <Threshold_Value>

    }
  }
  ......
}

The threshold about which we are talking about will be defined in 2 ways:

As percentage with default value as 10%. So if the filtered ids is <= 10% of the total live documents present in the segment than we do exact search. This 10% number is coming from the experiments which we did to get 5K number. So if you just use the above example provided what we can say is 5K is ~12% of 41K documents.
As absolute value with a default value as 2000. So if the filtered ids is <= 5000 than we do exact search.

Both of these will make sure that if customer has high number of documents in the his segment then we are caping the exact search to an absolute value. With 1 we get the advantage if a segment has 2.5K document only we are not falling in exact search case.
Overall the condition to do exact search will be :
FilteredIds <=K OR (
       (filterIds/totalLiveDocuments) * 100 <= filtered_exact_search_threshold_pct
       AND
       filterIds.length <= filtered_exact_search_threshold
    )

Pros:

Customers will have full control over this switch and can tune it based on their needs.

Cons:

This adds learning curve for customers as they now have to understand this new setting apart from M, EF_SEARCH and EF_CONSTRUCTION.
There are cases like a segment with 25K nodes and had like 3K filtered ids then we would end up doing ANN search which could have a bad recall if the higher level in HNSW graph doesn’t have those nodes.

Option 2
If we look closely what is happening during restrictive filtered search is we are not able to reach the nodes which are present in the filtered set. So the graph search at segment level is not returning K documents(even when FilteredIds Length > K). We can take advantage of this behavior where when ANN filtered search happened and it didn’t give K documents (even when FilteredIds Length > K) then discard the result of the ANN search and perform the exact search.
Pros:

The benefit of this approach is customers don’t have to configure anything in the index settings. Ease of use.

Cons:

As we are doing ANN search and then exact search, we are doing double work. But given that ANN search is pretty fast at a segment level, the added latency will not be that big. (Latency is factor of many things so not adding numbers here)

Option 3(Recommended)
This approach is nothing but a combination of 1 & 2. So we will have index settings and also the logic to do exact search if K ids are not returned by ANN Search.
Note: Even by combining both Option 1 & 2 there can still be cases where HNSW graph is built in a fashion which leads to bad recall like with 20K document in a segment and 5K filtered Ids, those filtered Ids could be far apart such that we get K results but not the best K results.
Pros:

It has Pros of both the above approach. :P

Cons:

As both the above approaches are little complementary to one another it overall we should have some good coverage on the different use cases of Filtered Search.

Experiment Setup
Philosophy
The basic philosophy behind the experiment setup is:

We should be able to do the testing on different dimensions and different number of FilterIds in Quick and efficient fashion.
The distance calculation setup should be as close as to the actual production. For this we will use the same classes provided by Lucene that we use in K-NN plugin for doing distance calculation. The reason to use lucene classes is because we use them in production and Lucene with JDK 20 has added the support for SIMD. Once that is enabled in the OpenSearch we will automatically get the benefits.

Setup
I create a small gradle java(Java 17) project with Lucene(9.7 version) as a dependency(Code: https://gist.github.com/navneet1v/87c693f015ae1267984173df7036ff32). I generated randomized dataset with different dimensions and then run it on mac(16GB RAM with 6 cores). For every dimensions there were 10 iterations and best latency was taken.
Experiment table template 1000_latency(ms): Represents the latency observed when filtered Ids count was 1000.

dimensions
1000_latency(ms)
2000_latency(ms)
5000_latency(ms)
8000_latency(ms)
10000_latency(ms)
15000_latency(ms)

64

128

256

512

768

968

1024

2048

4096

Experiment
Below are the steps that are performed to run this experiment:

For every dimension and every run, generate a dataset with required number of vectors in it. We choose 1k, 2k, 5k, 8k, 10k, 15k vectors count. The vectors in this dataset are serialized using the same code we use in K-NN(Code ref).
After query vector is generated. We don’t need to serialize the query as this is not what happens in the exact search.
Now to do the distance calculation we first de-serialized the vectors in the dataset using same code used by k-NN(Code ref) and then performed the distance calculation using Lucene provided functions. This is done for different dimensions and dataset. The time taken in this full step de-serialization and distance calculation is what being provided in below table. Lucene distance calculation functions are used to do the distance calculation. code ref
The experiment then collects all the latencies and format them in a table.

So, the query latency include:

Time taken to de-serialize the vectors to floats.
Distance calculation between the query vector and indexed vector.

dimensions
1000_latency_serialized(ms)
2000_latency_serialized(ms)
5000_latency_serialized(ms)
8000_latency_serialized(ms)
10000_latency_serialized(ms)
15000_latency_serialized(ms)

64
0.5361
0.30428
0.77805
1.2804
1.8098
2.36014

128
0.43123
0.54735
1.24873
1.9306
2.48106
3.5845

256
0.58271
1.06349
2.07025
3.69847
4.90092
8.71385

512
0.79718
1.65931
4.18262
6.67985
8.53127
15.2144

768
1.15486
2.20225
5.68341
9.35401
12.03496
23.44284

968
1.4213
2.85394
7.74272
14.27054
14.97346
33.13775

1024
1.46418
3.15568
7.50078
14.56883
17.00156
29.74787

2048
2.97711
6.85896
16.52853
24.79757
30.17373
78.9689

4096
5.90317
11.74842
31.05969
64.13292
83.25422
122.25665

Experiment Conclusion

Flipping to exact search when there are 2K Filtered Documents seems to be a good default. Reason for choosing 2K is because the latency diff between 2K and 5K is pretty high for all dimensions almost double. So if there are more than 1 segment then we will have high latencies. Example: for 1024 dimensions with 5K document latency was 7.5ms so for 5 segments it will be like > 35ms which substantially high compared to ANN search is pretty fast.
With Lucene coming up with SIMD instruction we will be able to reduce the distance calculation time.

FilteredIds <=K OR (
       (filterIds/totalLiveDocuments) * 100 <= filtered_exact_search_threshold_pct
       AND
       filterIds.length <= filtered_exact_search_threshold
  )

Referring to the formula you proposed, how about we consider dimensions also to account for better optimizations to switch to exact search. As per the experiments, its evident that for smaller dimensions say upto 768, it takes ~2ms to do exact search for 2k vectors.  But in the above formula, we would be doing approximate search even for smaller dimensions when there are fewer filterIDs. For example, lets say segment has 10k vectors of 128D and lets assumer number of filterIds is 1100 and k=100. Applying above formula will lead to Approximate as filterId percentage would be 11% ( 1100/10000 *100) while Exact search would do for 0.5ms and give 100% recall.
My proposal would be
FilteredIds <=K OR (
       **(filterIds.length <= filtered_exact_search_threshold and dimensions <= 768) OR (**
       (filterIds/totalLiveDocuments) * 100 <= filtered_exact_search_threshold_pct
       AND
       filterIds.length <= filtered_exact_search_threshold
  )

Applying above formula will lead to Approximate as filterId percentage would be 11% ( 1100/10000 *100) while Exact search would do for < 0.5ms and give 100% recall.

@vamshin to avoid this case only I added the option 3. It will cover those cases if K documents are not present from ANN Search. Moreover as these exposed as a setting to the user they can tune it based on their needs. I am planning to put this in the documentation as a reference so that customers can take a better call.
The problem with adding dimensions is:

I don't understand the rationale behind why ~2ms is good but not ~3ms which for 1024 dimensions.
To expose this dimension, we need to add another setting which is not a great customer experience, plus if till 768 dimensions and 2K ids exact search is taking 2ms, the ANN will be more faster as ANN Search is logarithmic. Hence, it's a tradeoff between providing a user experience where user need to tune multiple settings or adding may be less than 1ms latency. My inclination is towards user experience here.

Nevertheless I am open to suggestions here around why we should add the dimension.

@navneet1v Option3 does not make much sense for lower dimensions(<=768) and when the filterIDs are fewer say few hundreds. It further adds overhead by additional call for approximate search, when we already know the Exact search is better with 100% recall. I dont think we need to expose "dimension" to customers, it can be inbuilt optimization similar to "filterIds <= k".

I don't understand the rationale behind why ~2ms is good but not ~3ms which for 1024 dimensions.
We can take a judgement call on what can be acceptable latencies. Given we have possibility of getting 100% recall with marginal difference in latencies when compared to approximate, i feel this can be good optimization.

@navneet1v Option3 does not make much sense for lower dimensions(<=768) and when the filterIDs are fewer say few hundreds.

Actually it make more sense at lower dimensions, because if exact search is taking < 2ms ANN search will be way faster.

I don't think we need to expose "dimension" to customers, it can be inbuilt optimization similar to "filterIds <= k".

The reason for "filterIds <= k" check is because you have to do same amount of distance calculation in ANN and exact search both hence that is added explicitly. But for dimensions check we need to consider many things, one of that is does 2ms is good or 3ms is good. So its much more user dependent, hence want to expose that as a setting is required.

My bias is trading 1ms for 100% recall is acceptable. Might be wrong.

What I am not able to understand is why this bias is for 2ms and not 3ms? or are you suggesting now we should make the check from 768 dimensions to 1024 dimensions? Help me understand this.

It further adds overhead by additional call for approximate search

Let's talk about this overhead. If Exact Search is taking around 2ms latency and given ANN is logarithmic, are we talking about < 1ms here?

What I am not able to understand is why this bias is for 2ms and not 3ms? o

As mentioned 2ms will be latencies if we cutoff at 768D. We can make judgement call if we want to do it till 256D to keep latencies <1ms. As mentioned, my bias is more towards having 100% recall over 1ms additional latency and hence took 2ms as the cutoff and limited to 768D(as per the above table for 2k dimensions) to switch to Exact search.

Let's talk about this overhead. If Exact Search is taking around 2ms latency and given ANN is logarithmic, are we talking about < 1ms here?

I think you are missing the case when ANN could not find k results which is exactly Option 3 you referred . In that case as per option 3, you would do ANN(say 0.5ms) and then do Exact (2ms) which adds additional overhead

We can make judgement call if we want to do it till 256D to keep latencies <1ms.

Yes, this judgement call is what I am saying is more user dependent. I don't think we should take this call. We should leave this upto user to decide hence, we should expose this via an index setting.

I think you are missing the case when ANN could not find k results which is exactly Option 3 you referred

No, I am not missing that case.

In that case as per option 3, you would do ANN(say 0.5ms) and then do Exact (2ms) which adds additional overhead of 0.5ms especially for lower dimensions.

So, for a 100% recall we should take 2ms(if we take 768D as cutoff) but not 2.5ms (ANN + Exact Search) that seems bit odd to me.

I think we can go round and round on this.
Let's take it like this, we expose a setting for the dimension with 256 dimensions as default value for that setting. We will add an advance prefix with the setting as like other settings proposed in this issue. Also, if ANN search is done over filtered documents and K results are not returned we will do the exact search too.
Can we have a consensus on this?

Just to be clear, I am not opposing doing Exact Search when ANN cannot find k results(Option3). What I am opposing is, for smaller dimensions when we are clear that we could get 100% recall with marginal overhead with Exact Search, i would avoid doing ANN and then fall back to Exact. Just doing Exact would be fine.

Let's take it like this, we expose a setting for the dimension with 256 dimensions as default value for that setting(I know it will be 1 additional setting for user but I cannot see if we can avoid that).

Sounds like a plan. But am worried about too many settings that can cause confusion. One option is to have as a constant in this release similar to max dimensions constant and if we see demand for customers to have this tweaked, we can open as a setting.  Please take a call. I dont have strong reservation on this.

Just to be clear, I am not opposing doing Exact Search when ANN cannot find k results(Option3).

Thanks for clearing this out.

Sounds like a plan. But am worried about too many settings that can cause confusion. One option is to have as a constant in this release similar to max dimensions constant and if we see demand for customers to have this tweaked, we can open as a setting. Please take a call. I dont have strong reservation on this.

Valid point. but if we are going with adding this as a constant then lets go with 256(being more conservative here). Plus as per k-NN recommendation we recommend users to force merge to large segments this case should happen less frequent.

but if we are going with adding this as a constant then lets go with 256(being more conservative here).

512 should be good. Take a call.

Plus as per k-NN recommendation we recommend users to force merge to large segments this case should happen less frequent.

Segment could be large but filterIDs could be small(restrictive filter), so it could be frequent.

Im in alignment with option 3, but have alternative proposal on settings.
I think we could provide 2 expert settings and not provide dimension setting:
"knn.advanced.filtered_exact_search_threshold_pct" : <Threshold_Value_in_pct>,
"knn.advanced.filtered_exact_search_threshold" : <Threshold_Value>

However, instead of using a simple default value for "filtered_exact_search_threshold", we can create more custom default behavior. By default, we can take into account the dimension and filtered set size for the field being queried and set a dynamic limit. For 128 dimensions, cutoff may be 10,000, but for 1536, it may be 1,000, etc.
This would allow us to provide better tailored default behavior, without exposing too many settings. I think this is okay because a user trying to fine tune probably is only managing an index with 1 vector field. So they dont need to account for too many vector fields.

However, instead of using a simple default value for "filtered_exact_search_threshold", we can create more custom default behavior. By default, we can take into account the dimension and filtered set size for the field being queried and set a dynamic limit. For 128 dimensions, cutoff may be 10,000, but for 1536, it may be 1,000, etc.

So what you are proposing is have dynamic value in the code which changes based on dimensions. Correct?
I see 1 small problem with that, the dimension like 384 what we should do should I consider the value used for 256 or 512? I am thinking more logic in order
query vector dimension <= 128 use 10K
query vector dimension <= 256 use 5K
query vector dimension <= 512 use 2K
else use 1K
@vamshin can we have alignment on this?

I think we could provide 2 expert settings and not provide dimension setting:
"knn.advanced.filtered_exact_search_threshold_pct" : <Threshold_Value_in_pct>,
"knn.advanced.filtered_exact_search_threshold" : <Threshold_Value>

Yeah this was the proposal.

I think you could make it a function of total distance computations estimated.
From your experiments, this is time with additional columns around distance computations:

dimensions
num vecs
distance computations
time with serialization (ms)

64
1000
64000
0.5361

64
2000
128000
0.30428

64
5000
320000
0.77805

64
8000
512000
1.2804

64
10000
640000
1.8098

64
15000
960000
2.36014

128
1000
128000
0.43123

128
2000
256000
0.54735

128
5000
640000
1.24873

128
8000
1024000
1.9306

128
10000
1280000
2.48106

128
15000
1920000
3.5845

256
1000
256000
0.58271

256
2000
512000
1.06349

256
5000
1280000
2.07025

256
8000
2048000
3.69847

256
10000
2560000
4.90092

256
15000
3840000
8.71385

512
1000
512000
0.79718

512
2000
1024000
1.65931

512
5000
2560000
4.18262

512
8000
4096000
6.67985

512
10000
5120000
8.53127

512
15000
7680000
15.2144

768
1000
768000
1.15486

768
2000
1536000
2.20225

768
5000
3840000
5.68341

768
8000
6144000
9.35401

768
10000
7680000
12.03496

768
15000
11520000
23.44284

968
1000
968000
1.4213

968
2000
1936000
2.85394

968
5000
4840000
7.74272

968
8000
7744000
14.27054

968
10000
9680000
14.97346

968
15000
14520000
33.13775

1024
1000
1024000
1.46418

1024
2000
2048000
3.15568

1024
5000
5120000
7.50078

1024
8000
8192000
14.56883

1024
10000
10240000
17.00156

1024
15000
15360000
29.74787

2048
1000
2048000
2.97711

2048
2000
4096000
6.85896

2048
5000
10240000
16.52853

2048
8000
16384000
24.79757

2048
10000
20480000
30.17373

2048
15000
30720000
78.9689

4096
1000
4096000
5.90317

4096
2000
8192000
11.74842

4096
5000
20480000
31.05969

4096
8000
32768000
64.13292

4096
10000
40960000
83.25422

4096
15000
61440000
122.25665

Plotting distance comps vs latency, it looks pretty linear

Zooming in. Say we want to add a max latency of 3ms, we see that 2,000,000 distance computations might be a good limit:

@jmazanec15  I like the idea. This takes care of smaller dimensions. Thanks
Should we also change the setting name to something like knn.filtered_exact_search_threshold.operations = 2,000,000 ?

I think you could make it a function of total distance computations estimated.
From your experiments, this is time with additional columns around distance computations:
dimensions	num vecs	distance computations	time with serialization (ms)
64	1000	64000	0.5361
64	2000	128000	0.30428
64	5000	320000	0.77805
64	8000	512000	1.2804
64	10000	640000	1.8098
64	15000	960000	2.36014
128	1000	128000	0.43123
128	2000	256000	0.54735
128	5000	640000	1.24873
128	8000	1024000	1.9306
128	10000	1280000	2.48106
128	15000	1920000	3.5845
256	1000	256000	0.58271
256	2000	512000	1.06349
256	5000	1280000	2.07025
256	8000	2048000	3.69847
256	10000	2560000	4.90092
256	15000	3840000	8.71385
512	1000	512000	0.79718
512	2000	1024000	1.65931
512	5000	2560000	4.18262
512	8000	4096000	6.67985
512	10000	5120000	8.53127
512	15000	7680000	15.2144
768	1000	768000	1.15486
768	2000	1536000	2.20225
768	5000	3840000	5.68341
768	8000	6144000	9.35401
768	10000	7680000	12.03496
768	15000	11520000	23.44284
968	1000	968000	1.4213
968	2000	1936000	2.85394
968	5000	4840000	7.74272
968	8000	7744000	14.27054
968	10000	9680000	14.97346
968	15000	14520000	33.13775
1024	1000	1024000	1.46418
1024	2000	2048000	3.15568
1024	5000	5120000	7.50078
1024	8000	8192000	14.56883
1024	10000	10240000	17.00156
1024	15000	15360000	29.74787
2048	1000	2048000	2.97711
2048	2000	4096000	6.85896
2048	5000	10240000	16.52853
2048	8000	16384000	24.79757
2048	10000	20480000	30.17373
2048	15000	30720000	78.9689
4096	1000	4096000	5.90317
4096	2000	8192000	11.74842
4096	5000	20480000	31.05969
4096	8000	32768000	64.13292
4096	10000	40960000	83.25422
4096	15000	61440000	122.25665

The problem here is with outliers. Let's say we want to make sure that latency is never above 3ms. Then applying filters on then the view is this.
Table with latency < 3ms filter

dimensions
num vecs
distance computations
time with serialization (ms)

64
1000
64000
0.5361

64
2000
128000
0.30428

64
5000
320000
0.77805

64
8000
512000
1.2804

64
10000
640000
1.8098

64
15000
960000
2.36014

128
1000
128000
0.43123

128
2000
256000
0.54735

128
5000
640000
1.24873

128
8000
1024000
1.9306

128
10000
1280000
2.48106

256
1000
256000
0.58271

256
2000
512000
1.06349

256
5000
1280000
2.07025

512
1000
512000
0.79718

512
2000
1024000
1.65931

768
1000
768000
1.15486

768
2000
1536000
2.20225

968
1000
968000
1.4213

968
2000
1936000
2.85394

1024
1000
1024000
1.46418

2048
1000
2048000
2.97711

But now if you take the highest value(2048000) of distance computation done the data says that latency can go above 3 ms.

dimensions
num vecs
distance computations
time with serialization (ms)

64
1000
64000
0.5361

64
2000
128000
0.30428

64
5000
320000
0.77805

64
8000
512000
1.2804

64
10000
640000
1.8098

64
15000
960000
2.36014

128
1000
128000
0.43123

128
2000
256000
0.54735

128
5000
640000
1.24873

128
8000
1024000
1.9306

128
10000
1280000
2.48106

128
15000
1920000
3.5845

256
1000
256000
0.58271

256
2000
512000
1.06349

256
5000
1280000
2.07025

256
8000
2048000
3.69847

512
1000
512000
0.79718

512
2000
1024000
1.65931

768
1000
768000
1.15486

768
2000
1536000
2.20225

968
1000
968000
1.4213

968
2000
1936000
2.85394

1024
1000
1024000
1.46418

1024
2000
2048000
3.15568

2048
1000
2048000
2.97711

There are outliers here like latency above 3ms like 3.5 or 3.6ms.
But if we go with 2ms as cutoff then these outliers go away.
Table with latency < 2ms filter

dimensions
num vecs
distance computations
time with serialization (ms)

64
1000
64000
0.5361

64
2000
128000
0.30428

64
5000
320000
0.77805

64
8000
512000
1.2804

64
10000
640000
1.8098

128
1000
128000
0.43123

128
2000
256000
0.54735

128
5000
640000
1.24873

128
8000
1024000
1.9306

256
1000
256000
0.58271

256
2000
512000
1.06349

512
1000
512000
0.79718

512
2000
1024000
1.65931

768
1000
768000
1.15486

968
1000
968000
1.4213

1024
1000
1024000
1.46418

Table with max distance calculation value 1024000 from top.

dimensions
num vecs
distance computations
time with serialization (ms)

64
1000
64000
0.5361

64
2000
128000
0.30428

64
5000
320000
0.77805

64
8000
512000
1.2804

64
10000
640000
1.8098

64
15000
960000
2.36014

128
1000
128000
0.43123

128
2000
256000
0.54735

128
5000
640000
1.24873

128
8000
1024000
1.9306

256
1000
256000
0.58271

256
2000
512000
1.06349

512
1000
512000
0.79718

512
2000
1024000
1.65931

768
1000
768000
1.15486

968
1000
968000
1.4213

1024
1000
1024000
1.46418

So if we look at this, it the exact same thing which I added but as part of dimensions only.

Should we also change the setting name to something like knn.filtered_exact_search_threshold.operations = 2,000,000 ?

@vamshin This is not the proposal. The proposal is to use the distance calculation as hard coded value than a map of dimensions which was proposed here. Please check this line I think you could make it a function of total distance computations estimated.
I see 1 small problem with that, the dimension like 384 what we should do should I consider the value used for 256 or 512? I am thinking more logic in order

query vector dimension <= 128 use 10K
query vector dimension <= 256 use 5K
query vector dimension <= 512 use 2K
else use 1K

So rather than using a map of values we can use 1 single hardcoded value. 1 single hardcoded value is more like a magic number. So I was trying to avoid this number but rather make even default values more configurable let's say if we need fine tune these defaults for a specific dimension.

I see, I think we could do either the map or the universal value. With the map of values, though, we might end up with more magic numbers (10k, 5k, 2k, 1k, in above example). But my main point is that we can just expose the setting for number of vectors at which to cutoff as well as the percentage setting (no setting for dimensions). Then, implement more complex default behavior, instead of just having a default number of vectors we follow.

But my main point is that we can just expose the setting for number of vectors at which to cutoff as well as the percentage setting (no setting for dimensions). Then, implement more complex default behavior, instead of just having a default number of vectors we follow.

Yes we will do that. That was the proposal. We are not going to provide dimension setting.
But I want to make sure that apart from that we should have a map of dimension to number of vectors map also rather than the number of distance calculation value as a single value. Just to make sure that we can tune some dimensions in future rather than 1 single value.

My suggestion is to have setting like this knn.filtered_exact_search_threshold.computations = 2,000,000
Threshold could be defined in the code as knn.filtered_exact_search_threshold.computations/dimensions instead of number of vectors.
For example for 128D, this would be 2,000,000/128 = 15625
512D, this would be 2,000,000/128 = 3900
this solution does not need any map of values between vectors and dimensions

My suggestion is to have setting like this knn.filtered_exact_search_threshold.computations
I think we went away from settings and want to hard code the value just like we have for MAX_DIMENSIONS. This is what you were suggesting earlier

@vamshin what above solution lacks is if K-NN needs to do tuning for a set of dimensions like for 128 what if we need to increase the computation threshold from 2,000,000 to 3,000,000? Thats why I was opting towards a map of some set of dimensions which can be tuned in future rather than a hard coded value.

@vamshin what above solution lacks is if K-NN needs to do tuning for a set of dimensions like for 128

Understood but I am not sure if this is a valid use case. From the graphs shared by Jack, I see that latencies vs Computation graph is linear.  So special tuning for a particular dimension is not required IMO. It boils down to number of vector computations. If someone needs higher threshold they need to update knn.filtered_exact_search_threshold.computations setting to higher value.
Maintaining a Map infact seems more hardcoded than one setting. Also I feel its more maintenance as new dimensions keep adding.

Can we use knn.filtered_exact_search_threshold.computations in place of knn.advanced.filtered_exact_search_threshold_pct and knn.advanced.filtered_exact_search_threshold?
The concept of computations and getting right value might have high learning curve to users but it seems more flexible to handle the balance between latency and recall.

@heemin32 how do we define the computations for an index? like what we are even looking at when we say computations? and what will be a default value for that?

On a second thought, I think knn.filtered_exact_search_threshold.computations can replace knn.advanced.filtered_exact_search_threshold but not knn.advanced.filtered_exact_search_threshold_pct.
For the number, computation = dimension * document number. If this case, we choose default doc number as 2000 and dimension 512 to have less than 2ms latency overhead, the computation value will be 1024000. With this default value, the threshold can get calculated dynamically based on dimension. threshold = 1024000/dimensions.

On a second thought, I think knn.filtered_exact_search_threshold.computations can replace knn.advanced.filtered_exact_search_threshold but not knn.advanced.filtered_exact_search_threshold_pct.
For the number, computation = dimension * document number. It seems, we choose default doc number as 2000 and the dimension for not having more than 2ms latency overhead is 512. Then, the computation value will be 1024000. With this default value, the threshold can get calculated dynamically based on dimension. threshold = 1024000/dimensions.

Let's think about this 1 more time. I have 4096 dimension vector with 20K records in the segment. I did the filter and got like 1K documents. Now if we choose the number 1024000, then customer will get bad recall, because we moved to ANN search.
See this is the reason I am saying recall in Filtering is not a function of computation, its a function of number of docs. So both the setting threshold and threshold_pct needs to be added.
what we need to deicide, for smaller dimensions do we need 1 more setting/hard coded value of distance computation which can be used in OR with above 2 settings, just for better optimization. But we keep on forgetting if exact search is taking like 2ms then ANN will be very very fast (in micro seconds).

Okay. That make sense. If we are focusing more on recall than latency degradation, I agree that relying on the number of document is correct direction. I was more focusing on a latency degradation than recall.

@navneet1v  same argument goes for smaller dimensions as well. When you could go up to  8k vectors for 128D under 2ms with exact search and get 100% recall we are switching to Ann because of 2k vector threshold and giving bad recall. And majority of the usecases would be smaller dimensions from my understanding.

@heemin32 , I am trying strike a balance between latency and recall. I don’t think so you understand the example correctly. If I am getting a 20% recall very fast then it is of no use. This is why I am saying we should rely on number of documents then number of distance calculations. Because number of documents in filtered search impacts how the recall will be(simplification of the actual logic).
@vamshin this is where you are missing the point, if we don’t get K results from ANN(very very minimal latency) we are switching to exact search. Your distance calculation is not impacting recall in filtered search.

And majority of the usecases would be smaller dimensions from my understanding.

We don’t have a metrics for this to claim anything here. Hence I don’t agree here.

@navneet1v  let's not get to case of missing with Ann and switching to exact search. Just think about 8k 128D vectors and switching to Ann and getting results from Ann. In this case, it's bad recall when we know exact search can give recall of 1.0 with ~2ms latencies.

@navneet1v By using number of distance calculation, we can set a threshold on latency(of course it could be vary depends on hardwares). By using number of document, we can set a threshold on recall(this also could be vary depends on data set).  Whether we want to use one of them of combine them is another story.

Discussed with thread owners(@heemin32 , @jmazanec15 , @vamshin) on the Slack to get an alignment on what should be the final logic.
Below is the logic on how we will switch to exact search for Filtered Search:
maxDistanceComputation = 2048000 // based on the tables above which makes latency < 3.15 ms for all dimensions

if(set(ThresholdSettingValue) AND filterdIds.length <= ThresholdSettingValue) {
	// do exact search

} else if(maxDistanceComputation <= dim * filterdIds.length) {
	// do exact search
} else {
       // do ANN search with filteredIds
}

The maxDistanceComputation value will be hard coded in the code. We will expose this setting in future based on user demand. If a user is putting a setting then we are honoring that setting and not using the internal logic.
With the above setting if a user has multiple fields in the index then internal logic is dynamic in nature for switching to exact search.

The changes are merged. I will post the benchmark results once the 2.10 snapshot is present and will close the issue.

Posting the results of the benchmarks done on a 16 core machine with snapshot version of OpenSearch and k-NN built locally. This benchmarks is a preliminary benchmarks as part of release testing more benchmarks will be added on the release issue.
Restrictive Filters
Faiss (HNSW) 2.9
{
    "metadata": {
      "test_name": "Faiss HNSW Restrictive Filter Test",
      "test_id": "Faiss HNSW Restrictive Filter Test",
      "date": "07/20/2023 20:52:31",
      "python_version": "3.8.17 (default, Jul  5 2023, 21:04:15) \n[GCC 11.2.0]",
      "os_version": "Linux-6.1.34-59.116.amzn2023.x86_64-x86_64-with-glibc2.17",
      "processor": "x86_64, 36 cores",
      "memory": "920506368 (used) /<PHONE_NUMBER>4 (available) /<PHONE_NUMBER>8 (total)"
    },
    "results": {
      "test_took":<PHONE_NUMBER>99801,
      "delete_index_took_total":<PHONE_NUMBER>9959996,
      "create_index_took_total":<PHONE_NUMBER>0200885,
      "ingest_multi_field_took_total": 60584.180823600036,
      "refresh_index_store_kb_total": 2993302.6438476564,
      "refresh_index_took_total": 37546.422699699906,
      "force_merge_took_total":<PHONE_NUMBER>6769945,
      "query_with_filter_took_total": 58514.0,
      "query_with_filter_took_p50": 5.0,
      "query_with_filter_took_p90": 6.0,
      "query_with_filter_took_p99": 6.7,
      "query_with_filter_took_p99.9": 9.4,
      "query_with_filter_took_p100": 1128.0,
      "query_with_filter_client_time_total": 94834.7,
      "query_with_filter_client_time_p50": 8.7,
      "query_with_filter_client_time_p90": 9.5,
      "query_with_filter_client_time_p99": 10.5,
      "query_with_filter_client_time_p99.9": 199.4,
      "query_with_filter_client_time_p100": 1133.0,
      "query_with_filter_memory_kb_total": 1297436.0,
      "query_with_filter_recall@K_total": 0.18980632427320202,
      "query_with_filter_recall@1_total": 0.4012699802934092
    }
}

Faiss (HNSW) 2.10
{
  "metadata": {
    "test_name": "Faiss HNSW Restrictive Filter Test",
    "test_id": "Faiss HNSW Restrictive Filter Test",
    "date": "08/31/2023 22:59:24",
    "python_version": "3.8.16 (default, Mar  2 2023, 03:21:46) \n[GCC 11.2.0]",
    "os_version": "Linux-5.4.250-173.369.amzn2int.x86_64-x86_64-with-glibc2.17",
    "processor": "x86_64, 16 cores",
    "memory": "24262311936 (used) /<PHONE_NUMBER>2 (available) /<PHONE_NUMBER>4 (total)"
  },
  "results": {
    "test_took": 51304.0,
    "query_with_filter_took_total": 51304.0,
    "query_with_filter_took_p50": 5.0,
    "query_with_filter_took_p90": 6.0,
    "query_with_filter_took_p99": 6.0,
    "query_with_filter_took_p99.9": 10.0,
    "query_with_filter_took_p100": 17.0,
    "query_with_filter_client_time_total": 78088.0,
    "query_with_filter_client_time_p50": 8.0,
    "query_with_filter_client_time_p90": 8.0,
    "query_with_filter_client_time_p99": 9.0,
    "query_with_filter_client_time_p99.9": 37.0,
    "query_with_filter_client_time_p100": 369.0,
    "query_with_filter_memory_kb_total": 648718.0,
    "query_with_filter_recall@K_total": 1.0,
    "query_with_filter_recall@1_total": 1.0
  }
}

Lucene
{
  "metadata": {
    "test_name": "Lucene HNSW Restrictive Filter Test",
    "test_id": "Lucene HNSW Restrictive Filter Test",
    "date": "09/01/2023 05:15:05",
    "python_version": "3.8.16 (default, Mar  2 2023, 03:21:46) \n[GCC 11.2.0]",
    "os_version": "Linux-5.4.250-173.369.amzn2int.x86_64-x86_64-with-glibc2.17",
    "processor": "x86_64, 16 cores",
    "memory": "20951351296 (used) /<PHONE_NUMBER>8 (available) /<PHONE_NUMBER>4 (total)"
  },
  "results": {
    "test_took": 93793.0,
    "query_with_filter_took_total": 93793.0,
    "query_with_filter_took_p50": 9.0,
    "query_with_filter_took_p90": 10.0,
    "query_with_filter_took_p99": 11.0,
    "query_with_filter_took_p99.9": 13.0,
    "query_with_filter_took_p100": 19.0,
    "query_with_filter_client_time_total": 122488.0,
    "query_with_filter_client_time_p50": 12.0,
    "query_with_filter_client_time_p90": 13.0,
    "query_with_filter_client_time_p99": 14.0,
    "query_with_filter_client_time_p99.9": 41.0,
    "query_with_filter_client_time_p100": 378.0,
    "query_with_filter_memory_kb_total": 10660.0,
    "query_with_filter_recall@K_total": 1.0,
    "query_with_filter_recall@1_total": 1.0
  }
}

Conculsion
So with the changes in the logic we can see that recall of Faiss Restrictive filters have become 100% from 40 for recall@k.
I am going ahead and closing this issue now, as the changes are merged in 2.10 and ready for release.


test improvement

This is a list of what kinds of tests we still need. In case of the regression:
Smoke test

[x] Each example should work

[x] https://github.com/dai-shi/waku/issues/206

E2E

[x] Basic usage (click button, async server component)

SSR

[x] Should ssr render correctly on the first screen

RSC

[x] Should RSC work well
[x] Should server action work well
[ ] Should server action interactive work well with local files
[x] Dev mode should be the same as Prod mode.

Ecosystem

[ ] RSC with CSS
[ ] RSC with some vite plugins like (alias, vanilla extract CSS...)

Unit Test

[ ] Each single plugin should work well
[ ] RSC render worker should be predictable for each input/output

How do we test the serverless function like vercel, cloudflare, deno?

I think we can test deno and cloudflare, but not vercel. Vercel requires us to build the code in production so we can test it?


Error when creating Qualifications

Steps to Reproduce:
In Workspace:
Select a Facility, Open, and Checkout a Configuration for the Facility
Select 'Qualifications' from the Sections dropdown and click on it
Click on 'Create Qualifications'
System throws an error. See below:

Bug Fixed


Upload-OIDF-Presentation-at20240206

What type of PR is this?

subproject management

What this PR does / why we need it:

Upload the OIDF presentation material at 20240206 meeting to share

Which issue(s) this PR fixes:
NA
Special notes for reviewers:
Changelog input
Additional documentation

Hi @GillesInnov35 , @fernandopradocabrillo,  Thanks very much.


Create helper script for udev rules installation

It's currently done manually in the install instructions: http://turtlebot3.robotis.com/en/latest/sbc_software.html#usb-settings-burger-and-waffle
For the TurtleBot2 we have a simple script that makes it a one liner to setup: https://github.com/yujinrobot/kobuki_core/blob/devel/kobuki_ftdi/scripts/create_udev_rules
Wiki instructions for using the script are here: http://wiki.ros.org/turtlebot/Tutorials/indigo/Kobuki Base
This will decrease the likelyhood of user error at this stage of the installation instructions.

@allenh1 can you look at doing this?

@allenh1 can you look at doing this?

Absolutely!

There appears to be tools to automatically install them using dh_installudev http://manpages.ubuntu.com/manpages/trusty/man1/dh_installudev.1.html
Though there's potentially an issue resolving the ROS_DISTRO:
https://answers.ros.org/question/277124/buildfarm-doesnt-define-the-ros_disto-environment-variable/
A script would still be valuable for from source installations.

Hello :)
I commit devel branch including script for usb connection.
It will be merged master branch.
Thanks @tfoote and @allenh1 to contribute TurtleBot3 package
Darby


[folding] Optionally fold chained method calls

For frameworks that expose a fluent API (ex: d3.js), it is common to chain method calls on separate lines. Example here:
d3.select(".chart")
  .selectAll("div")
  .data(data)
  .enter()
  .append("div")
  .style("width", function(d) { return d * 10 + "px"; })
  .text(function(d) { return d; });

Currently, the vscode editor.foldAll command folds these chained method calls and hides everything except the first line d3.select(".chart").... It would be nice to optionally enable/disable this functionality based on your use case. (In my case, I would disable it by default for all languages.)
Likely related to folding not being language aware: https://github.com/Microsoft/vscode/issues/3422

Yes, this would need language awareness.


Improve search box

Typed words in the search box is not visible. @SauravMukherjee44 please assign me this, I can work on it.

Go for it! For support join our discord community: https://discord.com/servers/codein-community-831388874118660156

Go for it! For support join our discord community: https://discord.com/servers/codein-community-831388874118660156

Thanks @SauravMukherjee44, I'm already a member of code-in community

Can you please assist me this issue ? @SauravMukherjee44

@SauravMukherjee44 no one has closed this issue yet. Is it fine if you assign it to me?

@SauravMukherjee44
Please assign this issue to me.

I would like to work on this issue. Can you please assign it to me?


Add some tests to app-permissions package

Well, that was fun. Down to one test file.

Use PR#22 instead


fix: some location LRO return 201 rather than 200

/azp run java - core - ci

/azp run java - core - ci

/azp run java - core - ci

@yaohaizh CI has passed.

@anuchandy please help review this one


Viper config implementation

Implement viper for configuration mgmt
This change allows three types of input (in order of load priority), in accordance with the 12 factor app design.
* CLI flags
* Environment Variables
* Configuration file

Environment variables are declared using the GUAC prefix, e.g..
export GUAC_GDBUSER=neo4j
It requires the former creds value be split over two flags:
--gdbuser neo4j --gdbpass password
Closes: #273
Signed-off-by: Luke Hinds<EMAIL_ADDRESS>

Note; this is based upon #272, so that will need landing first.
Goddam DCO bot, I am positive I signed everything.

6eb8a9b seems to be missing signature

give me a minute, i just started a git dumpster fire.

should be good now


Add integration test for Catalog V2 cluster repos

Issue: https://github.com/rancher/qa-tasks/issues/397, https://github.com/rancher/qa-tasks/issues/396, https://github.com/rancher/rancher/issues/38793
Add test to validate create/update/delete of HTTP and Git cluster repos

I forgot to ask, but do we need a backport for v2.6 as well? @PennyScissors

Yes, tests should be backported to v2.6 unless they are specific to 2.7x+ functionality.

Alright, I'll backport this


proposal: log/slog: change slog.Group signature to ...any

Currently there is now way to access the standard conversion of ...any to ...Attr without building a record.
It would be nice to be able to build a group Attr this way, for patterns where you want to be able to pass around a set of attrs (like queries that return a standard set for logging, or building a set of labels before an if that adds more).
All existing calls to slog.Group would keep working as the ...any forms happily accept Attr directly anyway, they would just be a fraction slower, in cases where the performance is needed you could use GroupValue directly.
This also exposes the standard conversion to helpers that are not performance critical so they can have a signature that looks like the normal logging functions.
Performance critical helpers need to build a Record directly to avoid allocations, and could not use this approach, but exposing an API for that is a much larger surface area.

I don't have a strong opinion on this but I agree there is some value in having the standard ("suggested", "conventional"?) conversion defined and easily available. Another situation where a common conversion might be useful is when implementing alternatives to Logger. Using something other than Logger to wrap a Handler seems entirely anticipated by slog, and many of these might want to accept variadic any - my impression from experiments along these lines is that the utility of different conversions is not very high, but the confusion resulting from different conversions is surprisingly high.
Rather than changing slog.Group, what about a func Attrs(...any) []slog.Attr, with logic invoking slog's argsToAttr?
For groups, something like this would result in more tokens than changing the signature to Group(string, ...any), but far fewer tokens than requiring them per-attribute.
slog.Group("label", slog.Attrs("a", 1, "b", 2)...)

CC @jba

I'm generally in favor of this; I agree there should be some way to expose the logic that converts k-v pairs to Attrs. The Attrs function is a bit more general, but I think Group(string, ...any) slog.Attr is a slightly better choice for efficiency reasons, though I'd have to implement both and see. The other nice thing about Group is that we wouldn't be adding something.
Linking to the main proposal: #56345.

It's worth noting that you can easily do Attrs in terms of Group anyway, slog.Group("", args...).Group(), so I would lean in the direction of no new API, especially as I think the group case will be the most common use of attr building outside of a record.
I think most new Logger "replacements" will be building records and using Record.Add rather than a custom Attr builder

It sounds like people are in favor of slog.Group taking ...any over adding slog.Attrs. Do I have that right?

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Suppose someone wants to write an alternative to slog.Logger that always requires ...slog.Attr signatures and does not allow ...any in its API. Would accepting this proposal create too many holes in the damn to plug?

Not sure if this is what you are asking, but you can still build a group using Attr{"label", GroupValue(attrs...)} if you want to build one directly from a []Attr.

I don't think that's what I'm asking.
At the moment the none of the Attr constructors accept ...any. So it is possible to write a package that provides an alternate Logger API restricted to ...Attr parameters but reusing the slog.Attr constructors and Handler unmodified. The only "hole" left is the Record.Add method, but client code doesn't normally use that so it's OK. If the slog.Group signature takes ...any it backdoors this hypothetical package's attempts to hide ...any signatures.
I understand the desire to provide a tool for people who want access to the the ...any -> []Attr logic, so I prefer adding the func Attrs(...any) []slog.Attr function as both a more orthogonal and opt-in approach.

Suppose someone wants to write an alternative to slog.Logger that always requires ...slog.Attr signatures and does not allow ...any in its API. Would accepting this proposal create too many holes in the damn to plug?

That is an interesting point. But wouldn't they just go the whole way?
package kvless_slog

type Logger ...
type Handler = slog.Handler
type Attr = slog.Attr
type Value = slog.Value
func Int(key string, val int) Attr { return slog.Int(key, val) }
...
func Group(key string, attrs ...Attr) { return slog.Group(attrs...) }

Yes, there are a lot of those shims, but it would probably take no more than an hour and since slog is in the standard library, your maintenance burden would be small—a few tweaks every six months. And then you'd be completely locked down: no chance of calling Record.Add, or slog.Logger.Info for that matter (since client packages would no longer import log/slog at all).

But wouldn't they just go the whole way?

Now that you've show us how, I suppose they would. That approach had not occurred to me yet, though, so it might not occur to others. ;). But it's good to know it's possible and enterprising individuals can find the path. Thanks.

Is there a use case for func Attrs(...any) []slog.Attr, other than using it to create a Group?

Is there a use case for func Attrs(...any) []slog.Attr, other than using it to create a Group?

I am not sure. If there is a use case for code to create variables of type []slog.Attr and we want to populate them conveniently with the same rules that slog uses to handle ...any parameters then that seems like a use case.
But when I review code that has variables like that with our internal logging package it usually seems like a code smell and should be replaced by a use of logger.With call. But there is debate about that, for example: https://github.com/golang/go/issues/59126#issuecomment-1481436882.

Is there a use case for func Attrs(...any) []slog.Attr, other than using it to create a Group?

I would characterize the utility as syntactic, in a language that on principle doesn't embrace syntax metaprogramming ... there's some tension in this and it seems like a sensitive question.
A dumb playground example of what I mean: it's possible to prepare any error with file:line annotation and arbitrary additional logging attributes while still passing it around as a valid error. There would be ways to write this without a func Attrs, but I feel like the utility is tangible here.

I think Group(string, ...any) slog.Attr is a slightly better choice for efficiency reasons

I did some quick benchmarking of a slog.Group(string, ...Attr), groupAny, and AttrsFunc, using test lists based on the ones from slog/benchmark, and didn't spot a difference between the latter two. Both had worse numbers than the current Group(string, ...Attr) version (slower, and sometimes allocating more).
type Attr = slog.Attr

func groupAny(name string, args ...any) Attr {
	as := make([]Attr, 0, len(args))
	var a Attr
	for len(args) > 0 {
		a, args = argsToAttr(args)
		as = append(as, a)
	}
	return Attr{name, slog.GroupValue(as...)}
}

func AttrsFunc(args ...any) []Attr {
	as := make([]Attr, 0, len(args))
	var a Attr
	for len(args) > 0 {
		a, args = argsToAttr(args)
		as = append(as, a)
	}
	return as
}

Here is another consideration, briefly mentioned in the top post:

This also exposes the standard conversion to helpers that are not performance critical so they can have a signature that looks like the normal logging functions.

To elaborate Ian's argument: we've decided that ...any is the main way to use the log output functions, with ...Attr relegated to a performance-sensitive corner. Given that decision, and the ability write groups inline, it seems natural to allow
logger.Info("msg",
    "a", 1,
    "b", 2,
    slog.Group("g",
          "c", 3,
          "d", 4,
    ),
    "e", 5)

and jarring to require that arguments to Group be Attrs.
Indeed, a developer who only needed to produce log output, and didn't care about how Handlers or LogValuers worked, wouldn't even have to learn about Attrs and Values except for Group. (There is also the fact that the package doc begins with Attrs and Values; I now think that material should be moved later.)
To me, this is a strong reason to prefer changing the signature of Group.

Have all remaining concerns about this proposal been addressed?

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group


registration: Delete PreregistrationUsers created before 1.7.1.

The main benefit here is not having to deal with the possibility (in the code structure, code comments, etc) of prereg users with (realm=None and realm_creation=False), since it is no longer possible to create such prereg users.
The reason we couldn't do this earlier is because it gives the wrong error message for registration and invitation links that were created before 1.7.1. But I think it's been long enough since 1.7.1 that it's probably fine now.

Merged, thanks!


[BEAM-9887] Throw IllegalArgumentException when building Row with logical types with Invalid input

schema.logicaltypes.FixedBytes logical type expects an argument - the length of the byte[].
When an invalid input value (with length < expectedLength) is provided while building the Row with FixedBytes logical type, IllegalArgumentException is expected. But, the Exception is not thrown. The below code illustrates the behaviour:
 Schema schema = Schema.builder().addLogicalTypeField("char", FixedBytes.of(10)).build();
 byte[] byteArray = {1, 2, 3, 4, 5};
 Row row = Row.withSchema(schema).withFieldValue("char", byteArray).build();
 System.out.println(Arrays.toString(row.getLogicalTypeValue("char", byte[].class)));

The above code prints "[1, 2, 3, 4, 5]" with length 5 to the console, whereas the expected length of FixedBytes, is 10.
This PR fixes the issue.
Negative and Positive Test for FixedBytes Logical type are added.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

[ ] Choose reviewer(s) and mention them in a comment (R: @username).
[ ] Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
[ ] Update CHANGES.md with noteworthy changes.
[ ] If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

R: @reuvenlax @TheNeuralBit

I have a couple of questions regarding the behaviour of logical types:

Will this line ever get hit? If not, can we modify FixedBytes as a PassThroughLogicalType?: https://github.com/apache/beam/blob/34c58c42f14d2534a2f72f9194ddf9cc69138eea/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java#L78

Can we consider that the input value provided is of BaseType, which we can convert to InputType and store in memory?

retest this please

Hm so there are several ways of manually building a Row instance that provide different levels of runtime type-checking. Row#addValues explicitly validates everything, and Row#attachValues explicitly does not, for performance reasons. In SQL we have an option to switch between the two:
https://github.com/apache/beam/blob/34c58c42f14d2534a2f72f9194ddf9cc69138eea/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamAggregationRel.java#L354-L357
So we can have runtime type-checking for debugging, but then turn it off for performance.
I'm not sure how withFieldValue is intended to work. I'm not sure if the missing toInputType(toBaseType(value)) for that code path is intentional or an oversight. Can you clarify @reuvenlax?

@TheNeuralBit
schemas.logicaltypes.LogicalTypesTest.testFixedBytesIllegalArgument Test will fail even if addValue() method is used instead of withFieldValue() method.

@TheNeuralBit withFieldValue should replace addValues for must users. addValues is difficult and error prone and withFieldValues allows building a row based on named fields instead of positional fields.

LGTM

@reuvenlax @TheNeuralBit
Thanks for the review.
Could you please clarify the questions that I have asked above?
I wanted to understand more about logical types for my PR #11581

Will this line ever get hit?

We always convert logical types to their base type when serializing with SchemaCoder, and convert back to the input type when deserializing. Other than that I think the only time it should get called is when constructing a Row instance (unless you use attachValues).

Can we consider that the input value provided is of BaseType, which we can convert to InputType and store in memory?

Would this just be so that we're guaranteed to call toInputType whenever setting a value on Row? This PR accomplishes the same thing right?

We always convert logical types to their base type when serializing with SchemaCoder, and convert back to the input type when deserializing. Other than that I think the only time it should get called is when constructing a Row instance (unless you use attachValues).

In that case, there is no need to handle this else case right? as we are making sure that the input has expected length while building the Row.
https://github.com/apache/beam/blob/5e1571760b61b8ce247d5375b71c8df4d69d6409/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java#L77
Even if attachValues is used while building the Row and the provided input value is invalid(invalid length), during serialization in SchemaCoder, the input value cannot be converted to base type as it doesn't have expected length and an IllegalArgumentException will be thrown.

Would this just be so that we're guaranteed to call toInputType whenever setting a value on Row? This PR accomplishes the same thing right?

Can we support this feature: depending on the type of the input value provided while building the Row, we can call toInputType(toBaseType(inputValue)) or toInputType(inputValue) i.e. support for providing base value while building the Row. If both the InputType and BaseType are one and the same, we can directly call toInputType(inputValue). I am thinking that this might be helpful for logical types like FixedBytes or FixedLengthString.

In that case, there is no need to handle this else case right? as we are making sure that the input has expected length while building the Row.

checkArgument ensures that the condition is true, so we are rejecting byte arrays that are too short, and accepting anything longer than the fixed size. I don't think there's any capability that pads zeroes for short arrays, this logic just allows longer byte arrays, and copies out the appropriate portion.

Whoops sorry I completely misread that. We accept short byte arrays, and zero-pad them. We do not accept long byte arrays so that we don't have to truncate them.

Ahh ok. I'm sorry for being so dense, I see what you're saying now. In toBaseType we validate that the array's length == the fixed length:
https://github.com/apache/beam/blob/5e1571760b61b8ce247d5375b71c8df4d69d6409/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/FixedBytes.java#L67-L70
This is where the exception checked in your new tests is thrown.
So we never actually get into toInputType where we could accept a shorter byte array and zero-pad. And there doesn't seem to be any other way to set a value by base type with a shorter byte array and pad it.
This seems to be a holdover. Previously Row stored logical type values as their base type, so we probably called toBaseType(toInputType(x)).

I'd be +1 for just dropping the padding logic. I don't think it should be the responsibility of the LogicalType to coerce values like this. What do you think @reuvenlax?

This seems to be a holdover. Previously Row stored logical type values as their base type, so we probably called toBaseType(toInputType(x)).

Even before the code which changed Row to store logical type values instead of base values in memory, while building the row, the input value is expected to be of correct length.
https://github.com/apache/beam/blob/9f0cb649d39ee6236ea27f111acb4b66591a80ec/sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java#L658-L660
I think this issue is present since the introduction of FixedBytes.

If there are no comments, can we close this PR?

I'd be +1 for just dropping the padding logic. I don't think it should be the responsibility of the LogicalType to coerce values like this. What do you think @reuvenlax?

@TheNeuralBit @reuvenlax I have taken care of this for FixedBytes and other standard logical types in #11581

cc: @ibzib
@ibzib can this fix be cherry-picked for 2.21.0 release?


Python Django project being misclassified as HTML

https://github.com/bensnyde/django-csa

Hey @bensnyde.
You can stop these files being included in the repository statistics by using the Linguist Overrides.


feat: generate different scripts for different projects if they are a part of the same workspace

@joeldenning ping.

@joeldenning what do you think if we let the user to specify a port when running ng add?
There will be a question right after routing and browserAnimationsModule, the default port will be 4200.

👍 sure


Allow sync to persistent storage

Using the same merge functionality in overlayfs-tools already used to
write changes to persistent storage it is possible to write changes to
disk without completely reseting the zram device. This allows for a
quick sync to persistent storage to be run whenever desired.
Without the ability to sync, any changes made to the underlying
filesystem will be lost if there was a sudden power loss or system
shutdown. This can be mitigated by periodically running the sync command
which will write the changes to persistent storage ensuring that the
data will not be lost if anything goes wrong.
A default service that runs the sync every night is included in this
change, however, it is not installed by default.
Fixes #72
Signed-off-by: Ethan Dye<EMAIL_ADDRESS>

Just wanted to ask you about syncing now seeing you have already implemented it ? Hadn't seen that earlier.
Great ! How much testing did you do ? Is this ready for deployment in openHABian ?

Just wanted to ask you about syncing now seeing you have already implemented it ? Hadn't seen that earlier.

Yeah, I just got that implemented about a week ago.

How much testing did you do ?

Not a ton, however it is using the exact same procedure as when we stop zram-config. The only difference is instead of cleaning up after the overlay merge we remount the overlay filesystem and keep going.

Is this ready for deployment in openHABian or is (lazy) unmounting impossible with OH running ?

Still impossible, as it would cause broken writes the sync still would require stopping openHAB or for that matter any service or program accessing the directory in zram before the merge and the restarting it after. It is merely a simpler way to merge changes than completely stopping zram and restarting it. It could be deployed but I have left it as an optional install here and would probably recommend doing the same there as well. As a side not because of the way I have implemented the sync, there would be no need to manually stop openHAB's services as I use the same custom service identification code that I do in the stop functionality to achieve the service stopping.


[VCQ][DevDiv_VSO][VSCode][Source error]

MS PM
Daniel Ye

MOR PM
Jan Baltus (Moravia IT)

String Resource ID
0; "src/vs/workbench/contrib/terminal/browser/remoteTerminalService.terminal.integrated.starting"

Source String
Starting2...

Subtenant
VSCode

Source Control Link

Filename
vscode-workbench\vs_workbench_contrib_terminal.xlf

Question
It seems starting2 is a source error of starting.
We just followed the source form to add 2 for now.
Please check and confirm to keep or remove 2 in the target.

Additional details

Core Team Response

Fixed already.


Example for disabling nkey rollover?

Do y'all happen to have an example to disable nkey rollover?
Thanks!

https://github.com/KinesisCorporation/Adv360-Pro-ZMK/blob/0c975742d105747d551b41dc4056e5ae1c74982d/config/boards/arm/adv360/adv360_left_defconfig#L64
Change =y to =n


Extend the simulation to take the configuration into account

Because of the corona crisis it would be nice to offer some more possibilities to work with the simulation. As a first step the ultrasonic and color/light sensors (only NXT and EV3) should be supported multiple times, like on a real robot e.g.

This will make it much easier to create line follower robots. Multiple ultrasonic sensors could be used to determine locations of objects more precisely.
Only the robot's configuration will be extended, there will be no way to change the robot's geometry. Sensors will still have a predefined position on the robot, e.g.

Simulation is now mostly configurable, specific amounts of sensors lead to different robot layouts.


/editions/{year}/questiontags/showInList doesn't work

The /editions/{year}/questiontags/showInList endpoint doesn't seem to work. I tested it locally and postgres can't handle uppercase characters in column names without quoting them, which I suspect sqlalchemy doesn't do. It's also camelcase which is a different style from the usual snake case so I propose we change the name of the endpoint and column name to show_in_list.

After closer inspection, the actual issue was that the routers weren't ordered properly. Refactoring to snake case still seems beneficial to make names more consistent.


Feature/issue50 2

Added the three new detector geometries as described in Issue 50.
Also implemented Erin's suggestions regarding the definitions of WCCapPMTSpacing and WCCapEdgeLimit.
Ran WCSim with the new geometries. Occasionally get the warning "Potential overlap in geometry! *** This is just a warning message.". Ran WCSim with the (already implemented) SuperK geometry and notice a similar number of warnings. Assume this is OK?

Great, the commit history looks a lot cleaner in this pull request. I also just checked out and ran your pull request branch and things seem to be working properly. This looks good to me! I will merge this today if there are no other comments.

@samshort25 , do you mind adding a fourth detector configuration with this pull request? An SK detector with 20" PMTs and 20% photocoverage (this is just like the SK detector in SetSuperKGeometry() but with a 20% photocoverage). I will update issue #50 to include this detector.

No problem at all. I will do it tomorrow (as I'm in the process of writing a talk for tomorrow at the moment). Do you want me to change the WCCapPMTSpacing and WCCapEdgeLimit parameters too? Or would you prefer to have them left at the SK values?

Change WCCapPMTSpacing and WCCapEdgeLimit  as you did for the other detector configurations so that everything gets properly updated when you change the photocoverage.

Should I implement all of the proposed designs here: https://wiki.hyperk.org/DesignReport/DetectorOptimization before you merge my branch (I'm not sure if that's the correct terminology?) All just cylinders, yes?

I don't want to implement every possible tank that we might simulate in the future.  Things get left around that way and everyone is too afraid to get rid of things that area already there.
So, lets start with the proper SK options + the 1 100m tank you already put in.
@eosullivan will compare agains the table and confirm if we have the correct ones.
In the next step if we need to do more we can add them then.

Let's let the dust settle a bit today so we can make sure we have agreement among the working groups about which configurations they would like to see. I suspect in the end we will implement the four detectors that you already have in there, but the photocoverage might need to be updated slightly. We will try and match this pull request to the "Detector Configurations in WCSim" table on the detector optimization wiki page at: https://wiki.hyperk.org/DesignReport/DetectorOptimization#preview

Thanks for adding the table Erin.
Maybe it should be:
Detector Configurations in WCSim ->
Detector Configurations Currently implemented in WCSim
And then to the table also add the WCSim detector configuration name.

Good idea to add the configuration name. I've just put it on the wiki.

Ok, I've updated the description of Issue 50 to reflect the new photocoverages given the information provided by the photosensor group. @samshort25 can you update the photocoverage values in this pull request? It seems like these are now finalized.

@eosullivan I think that's all done now. Let me know if there's anything else that needs adding.

I think you should add some comments to the detector configurations in the macros (novis.mac and vis.mac) about what the actual photocoverage is, just so it is really obvious to those running the configurations that they are slightly different than the label suggests. Other than that, I think the changes look good. Let's review them one more time in the software meeting tonight and then hopefully we can get these changes merged in.

Ok, as per the meeting tonight, we will add a fifth detector option (SK with 20" HPD and 20.27% photocoverage). @samshort25 , sorry about the many changes to the issue! There was consensus in the group that after the fifth detector is added in, the pull request can be merged in. I updated issue #50 with the fifth detector option.

Ok, everything looks good. Thanks for implementing these, Sam!


Failing CRRao Tests

The CRRao Tests are failing due to a ADGradient error.
A similar error occurs in this program:
using Turing, StableRNGs

@model function example(x)
    m ~ Normal(0, 1)
    x ~ Normal(m, sqrt(1))
end

res = sample(StableRNG(123), example(1), NUTS(), 10)

This issue might be related to this issue.

Seems to be working on Turing v0.26.0, which version are you using?

I was using v0.23.3. That might have been the issue. CRRao also currently uses v0.23.3 which might be causing it's test to fail.

Okay, I've removed the old version of Turing in this PR.
https://github.com/xKDR/CRRao.jl/pull/121
Let's wait for the CI to complete.

These two logistic regression tests are failing
(prior, link) = (Prior_HorseShoe(), Logit(CRRao.Logit_Link))
(prior, link) = (Prior_HorseShoe(), Probit(CRRao.Probit_Link))

For Negative binomial, we get an error outside the test:
zero(p) < p <= one(p)
These are the only two errors.

It is being fixed and new release  0.1.1 is being done


question

Hello, I find your work has been accepted by ECCV. Congratuations, but I want to ask when can you release your training code?

Thanks! Considering the partner company, we have no plans to release the complete open-source code at present.


boards/arm64/imx9/imx93-evk: add trch header

Example trdc header for bootloader build
Summary
Impact
Testing

Commit title "add trch header" -> "add trdc config header" ?

@joukkone I fixed the style issue and re-based against current master; feel free to merge if you are happy with this.