Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync #1

Merged
merged 234 commits into from
Oct 17, 2024
Merged

Sync #1

merged 234 commits into from
Oct 17, 2024

Conversation

aalekhpatel07
Copy link
Owner

No description provided.

Warmchay and others added 30 commits April 2, 2024 06:11
Summary:
**Bugs:**
When following rocksdb_ivf demo to build executable file, its output as:
```bash
faiss/demos/rocksdb_ivf/RocksDBInvertedLists.h:52:35: error: 'faiss::InvertedListsIterator* faiss_rocksdb::RocksDBInvertedLists::get_iterator(size_t) const' marked 'override', but does not override
   52 |     faiss::InvertedListsIterator* get_iterator(size_t list_no) const override;
      |                                   ^~~~~~~~~~~~
make[2]: *** [CMakeFiles/demo_rocksdb_ivf.dir/build.make:90: CMakeFiles/demo_rocksdb_ivf.dir/RocksDBInvertedLists.cpp.o] Error 1
```

**Solution:**
Add relevant variable `void* inverted_list_contex` corresponding `get_iterator`'s base virtual function.

Pull Request resolved: #3326

Reviewed By: mlomeli1, mdouze

Differential Revision: D55629580

Pulled By: algoriddle

fbshipit-source-id: a12fcacb483e0dd576411ad91a3dd1e0de94abec
…Lists (#3327)

Summary:
Pull Request resolved: #3327

**Context**
1. [Issue 2621](#2621) discuss inconsistency between OnDiskInvertedList and InvertedList. OnDiskInvertedList is supposed to handle disk based multiple Index Shards. Thus, we should name it differently when merging invls from index shard.
2. [Issue 2876](#2876) provides usecase of shifting ids when merging invls from different shards.

**In this diff**,
1. To address #1 above, I renamed the merge_from function to merge_from_multiple without touching merge_from base class.
why so? To continue to allow merge invl from one index to ondiskinvl from other index.

2. To address #2 above, I have added support of shift_ids in merge_from_multiple to shift ids from different shards. This can be used when each shard has same set of ids but different data. This is not recommended if id is already unique across shards.

Reviewed By: mdouze

Differential Revision: D55482518

fbshipit-source-id: 95470c7449160488d2b45b024d134cbc037a2083
Summary:
Pull Request resolved: #3338

add reconstruct_n for GPU IVFFlat

Reviewed By: mdouze

Differential Revision: D55577561

fbshipit-source-id: 47f8b939611e2df7dbcd087129538145f627293c
Summary:
Pull Request resolved: #3349

**Context**
[Issue 3128](#3128) is an enhancement request to support remove_ids for IVFPQFastScan.

Existing mechanism use direct map and iterate over items in selector and use scopecodes and scopeIds to replace item to be removed. Given that codes are packed, it is hard to return single code how it is packed in CodePackerPQ4. Thus, we need a custom implementation to removed_ids.

**In this diff**,
1. We have added custom implementation of remove_ids from BlockInvertedLists which unpack code as it iterate and repack in new position. DirectMap use this remove_id function in BlockInvertedLists for type NoMap in DirectMap.

2. Also, we are throwing exception for other map type in DirectMap i.e. HashTable

Reviewed By: mdouze

Differential Revision: D55723390

fbshipit-source-id: 0017b556bd790765251e778ac48ed37ff3a29a45
…3336)

Summary:
Pull Request resolved: #3336

Issues:
#3269
#3024

List of implemented GPU indices: https://github.com/facebookresearch/faiss/wiki/Faiss-on-the-GPU#implemented-indexes

Reviewed By: mdouze

Differential Revision: D55577576

fbshipit-source-id: 49f490cfba6784661e378acf4de3cce4195bb43b
Differential Revision:
D55723390

Original commit changeset: 0017b556bd79

Original Phabricator Diff: D55723390

fbshipit-source-id: 58d61467b30dd11d27398f9f825162f598896845
Summary:
Pull Request resolved: #3354

**Change was previously reverted because of build failure as change D55577576 removed the definition of FAISS_THROW_IF_MSG**

**Context**
[Issue 3128](#3128) is an enhancement request to support remove_ids for IVFPQFastScan.

Existing mechanism use direct map and iterate over items in selector and use scopecodes and scopeIds to replace item to be removed. Given that codes are packed, it is hard to return single code how it is packed in CodePackerPQ4. Thus, we need a custom implementation to removed_ids.

**In this diff**,
1. We have added custom implementation of remove_ids from BlockInvertedLists which unpack code as it iterate and repack in new position. DirectMap use this remove_id function in BlockInvertedLists for type NoMap in DirectMap.

2. Also, we are throwing exception for other map type in DirectMap i.e. HashTable

Reviewed By: ramilbakhshyiev

Differential Revision: D55858959

fbshipit-source-id: c8a0631495380b7dead36720e4507f4d1900d39f
Summary: Pull Request resolved: #3304

Reviewed By: junjieqi

Differential Revision: D55823369

Pulled By: mdouze

fbshipit-source-id: c0e9f4b85d979758f02e9953f3706b63a846bf22
Summary:
Pull Request resolved: #3362

Add test to Alex' PR

Reviewed By: junjieqi

Differential Revision: D56003946

fbshipit-source-id: 5a8a881d450bc97ae0777d73ce0ce8607ec6b686
Summary:
Pull Request resolved: #3363

'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead

{F1484071654}

Reviewed By: kuarora

Differential Revision: D56009251

fbshipit-source-id: ec222cf589ff98b016979058d59fc20cccec8f43
Summary:
LLVM-15 has a warning `-Wunused-but-set-variable` which we treat as an error because it's so often diagnostic of a code issue. Unused variables can compromise readability or, worse, performance.

This diff either (a) removes an unused variable and, possibly, it's associated code, or (b) qualifies the variable with `[[maybe_unused]]`, mostly in cases where the variable _is_ used, but, eg, in an `assert` statement that isn't present in production code.

 - If you approve of this diff, please use the "Accept & Ship" button :-)

Reviewed By: dmm-fb

Differential Revision: D56065763

fbshipit-source-id: b0541b8a759c4b6ca0e8753fc24b8c227047bd3d
Summary: Previously this code conformed from clang-format 12.

Reviewed By: igorsugak

Differential Revision: D56065247

fbshipit-source-id: f5a985dd8f8b84f2f9e1818b3719b43c5a1b05b3
Summary:
The CMakeLists.txt in faiss/gpu uses the $<LINK_LIBRARY:WHOLE_ARCHIVE expression which requires at least cmake 3.24.

Pull Request resolved: #3305

Reviewed By: mlomeli1

Differential Revision: D56234500

Pulled By: algoriddle

fbshipit-source-id: dfe7df3379c5250dedec7d1988cffa889fc1c393
Summary:
In this commit ab2b7f5, they changed format based on clang-format-18. However, we still use clang-format-11 in our circle ci job which caused the failure. In this PR, we are going to switch to clang-format-18

Pull Request resolved: #3372

Reviewed By: kuarora

Differential Revision: D56280363

Pulled By: junjieqi

fbshipit-source-id: f832ab2112f762e6000b55a155e3e43fe99071d7
Summary:
Pull Request resolved: #3371

This will never happen because N is fixed at compile time and the buffer is large enough. It is misleading to add error handling code for a case that will never happen.

Reviewed By: kuarora

Differential Revision: D56274458

fbshipit-source-id: ca706f1223dbc97e69d5ac9750b277afa4df80a7
Summary:
The current loop goes from 0 to 31.  It has an if statement to do an assignment for j < 16 and a different assignment for j >= 16.  By unrolling the loop to do the j < 16 and the j >= 16 iterations in parallel the if j < 16 is eliminated and the number of loop iterations is reduced in half.

Then unroll the loop for the j < 16 and the j >=16 to a depth of 2.

This change results in approximately a 55% reduction in the execution time for the bench_ivf_fastscan.py workload on Power 10 when compiled with CMAKE_INSTALL_CONFIG_NAME=Release.

The removal of the if (j < 16) statement and the unrolling of the loop removes branch cycle stall and register dependencies on instruction issue. The result is the unrolled code is able issue instructions earlier thus reducing the total number of cycles required to execute the function.

Pull Request resolved: #3364

Reviewed By: kuarora

Differential Revision: D56455690

Pulled By: mdouze

fbshipit-source-id: 490a17a40d9d4439b1a8ea22e991e706d68fb2fa
Summary:
This pull request is for issue #3330. This patch makes sure that packed code arrays are in big endian format. Kindly let us know if we need any changes or if we can have a better approach.

Pull Request resolved: #3345

Reviewed By: junjieqi

Differential Revision: D55957630

Pulled By: mdouze

fbshipit-source-id: f728f9563f6b942af9d8899b54662d7ceb811206
Summary:
Pull Request resolved: #3361

Fix a few issues in the PR.
Normally all tests should pass on a litlle-endian machine

Reviewed By: junjieqi

Differential Revision: D56003181

fbshipit-source-id: 405dec8c71898494f5ddcd2718c35708a1abf9cb
Summary:
Pull Request resolved: #3383

In this diff, I am fixing minor issues in bench_fw where either certain fields are not accessible when index is build from codec. It also requires index to be discovered using codec alias as index factory is not always available.

In subsequent diff internal to meta will have testcase that execute this path.

Reviewed By: algoriddle

Differential Revision: D56444641

fbshipit-source-id: b7af7e7bb47b20bbb5515a66f41dd24f42459d52
Summary: Fixes #3343

Reviewed By: kuarora, junjieqi

Differential Revision: D56526842

fbshipit-source-id: b7c4377495db4e68283cf4ce2b7c8fae008cd404
Summary:
The osx failed

https://app.circleci.com/pipelines/github/facebookresearch/faiss/5698/workflows/4e029c32-8d8b-4db7-99e2-8e802aad6653/jobs/32701

Pull Request resolved: #3357

Reviewed By: kuarora

Differential Revision: D56039739

Pulled By: junjieqi

fbshipit-source-id: dd434a8817148364797eae39c09e0e1e9edbe858
Summary: Remove debugging log lines

Reviewed By: mlomeli1

Differential Revision: D56626636

fbshipit-source-id: 2721b84e4e1359d1372df2b2c95cc668c6a75c3f
Summary:
This demonstrates how to query several independent IVF indexes with a trained index in common. This avoids to duplicate the coarse quantizer and metadata in memory.

On the Faiss side, it also implements a InvertedListIterator on top of the flat inverted lists, which can prove useful.

Reviewed By: junjieqi

Differential Revision: D56575887

fbshipit-source-id: cc3b26e952ee21f24b10169b5b614066600cf4b8
Summary:
`nullptr` is typesafe. `0` and `NULL` are not. In the future, only `nullptr` will be allowed.

This diff helps us embrace the future _now_ in service of enabling `-Wzero-as-null-pointer-constant`.

Reviewed By: palmje

Differential Revision: D56650318

fbshipit-source-id: 803ae62114c39143b65946f6f0387715eaf7f534
Summary:
This commit is the first in a series in an attempt to incrementally enable all jobs currenlty performed by CircleCI. It includes the main configuration files provided by GitHub team + 1 build.

Original PR: #3325

Reviewed By: junjieqi

Differential Revision: D56671582

fbshipit-source-id: c8a21cd69aabaf86134eb86753e90b1facf51bc3
Summary: GitHub checks

Reviewed By: junjieqi

Differential Revision: D56733297

fbshipit-source-id: fe5a2ca7c67f36a4fe986af78fb6dc8f4f843150
Summary:
Fixes #3379

Pull Request resolved: #3381

Reviewed By: junjieqi

Differential Revision: D56570120

Pulled By: kuarora

fbshipit-source-id: 758ea4ab866609d6dd5621e6e6ffda583ba52503
Summary: Migration to GitHub actions

Reviewed By: junjieqi

Differential Revision: D56745520

fbshipit-source-id: 5311a549842f19672ae574edaa8be3ea5a580dbc
Summary:
Pull Request resolved: #3405

Migration to GitHub Actions

Reviewed By: junjieqi

Differential Revision: D56843276

fbshipit-source-id: 3d5c7ee9a36a783407dfdcc3574c377da5f9db78
Summary:
Pull Request resolved: #3406

Migration to GitHub Actions

Reviewed By: junjieqi

Differential Revision: D56848895

fbshipit-source-id: 5a351534d9151369a9104314fee203657ac40043
mengdilin and others added 27 commits September 30, 2024 19:35
Summary:
Pull Request resolved: #3901

1) remove system time from benchmark as this metric has extremely high jitter (50-100%) and is not useful for us

2) clean up command-line arguments and define a main function the external world can call

3) tweak default so microbenchmark runs fast by default (this does not the parameters we pass to microbenchmarks for servicelab)

Reviewed By: mnorris11

Differential Revision: D63650110

fbshipit-source-id: efc81563291f00701a0d1df1d27172adeb3ef231
Summary: Pull Request resolved: #3887

Reviewed By: kuarora

Differential Revision: D63355030

Pulled By: asadoughi

fbshipit-source-id: 38792e49fe678c2811896faca7a3ddcab19f8bd0
Summary:
Pull Request resolved: #3907

same as title.
Fix checking right desc

Reviewed By: satymish

Differential Revision: D63854967

fbshipit-source-id: b8bc48662bc38ac96cf9241bdbe2be2b23f1a37e
Summary: Pull Request resolved: #3921

Reviewed By: pankajsingh88

Differential Revision: D64005877

Pulled By: ramilbakhshyiev

fbshipit-source-id: 663c7ab752db04751c7675095d2545adec4be173
Summary:
Similar to .github/workflows/nightly.yml

Pull Request resolved: #3910

Reviewed By: kuarora, pankajsingh88

Differential Revision: D63923478

Pulled By: asadoughi

fbshipit-source-id: df92a86ba48aa0d19aae40d7ca11aeedf4dfac51
Summary:
Pull Request resolved: #3919

These tests are passing successfully in `dev` mode during my local development when I added them but I recently noticed they are failing on contbuild which is running them in opt/mode: https://www.internalfb.com/intern/test/281475152762853/

Upon further inspection, 2 of these were from floating point comparisons which we can fix with `EXPECT_NEAR`. The another one stems from indeterminism of the results in opt mode, so we will relax the test until we figure out a way to deal with the indeterminism

Reviewed By: junjieqi

Differential Revision: D63942329

fbshipit-source-id: 60f1c0b8a0db93015cd32bf991ab983ff2d1af13
Summary:
Pull Request resolved: #3916

Adding missing wrapper to the torch wrappers in Faiss + test it.

Also factorized a  bit of code between search functions.

Reviewed By: algoriddle

Differential Revision: D63974821

fbshipit-source-id: a0415a57a763e2d1896956c503e503615c167860
Summary:
Sometimes between Sept 25 to Oct 2, downloading and linking against `openblas=*=*openmp*` package to run tests have caused a 4-7x slow down. Link it with the regular openblas package which is not compiled with `USE_OPENMP=1`. We will set the openblas omp threads via the environment variable `OPENBLAS_NUM_THREADS` according to https://github.com/OpenMathLib/OpenBLAS/wiki/Faq#multi-threaded

Pull Request resolved: #3918

Test Plan: SVE CI should finish within 40 minutes

Reviewed By: ramilbakhshyiev

Differential Revision: D64059860

Pulled By: mengdilin

fbshipit-source-id: 3ba2bda5fce5122f051421f459692f15ad5360a4
Summary:
Pull Request resolved: #3928

Fix issue in T203425107

Reviewed By: asadoughi

Differential Revision: D64068971

fbshipit-source-id: 56db439793539570a102773ff2c7158d48feb7a9
Summary:
* Replaced 1.8.0 to 1.9.0.
* Fixed x86-64 architecture reference: https://en.wikipedia.org/wiki/X86-64

Tested with: `conda install -c pytorch/label/staging faiss-cpu`

Pull Request resolved: #3929

Reviewed By: ramilbakhshyiev

Differential Revision: D64082430

Pulled By: asadoughi

fbshipit-source-id: 8a1427a7c14b8c3de4a341533b138d9d8f8490f2
Summary:
Pull Request resolved: #3934

Initial thought was to be able to call individual operations on execution operator but it make sense to keep single interface 'execute' and move all these implementations to respective operators.

Reviewed By: satymish

Differential Revision: D63290104

fbshipit-source-id: d1f0b1391c38552c5cdb0a8ea935e23d0d0cb75b
Summary:
Pull Request resolved: #3935

same as title

Reviewed By: satymish

Differential Revision: D64144800

fbshipit-source-id: 4a298aa83315d82f44ee424bf0a30737d5bf48a4
Summary:
Pull Request resolved: #3917

The norm computation ST_norm_from_LUT was not implemented in Faiss. See issue

#3882

This diff adds an implementation for it. It is probably not very quick. A few precomputed tables for AdditiveQuantizer were moved form ResidualQuantizer.

Reviewed By: asadoughi

Differential Revision: D63975689

fbshipit-source-id: 1bbe497a66bb3891ae727a1cd2b719479f80a836
…3922)

Summary:
Pull Request resolved: #3922

We need to be able to build external modules into FAISS, but don't have an example yet. This diff shows what CMakeLists.txt changes need to happen to incorporate an external module.

Reference: #3699

Reviewed By: mdouze

Differential Revision: D63991471

fbshipit-source-id: 0c1cd25eabbffb01d2a7170d6725a0c4a13c5bf0
Summary:
Pull Request resolved: #3941

Support PQFS with index trainer

Reviewed By: kuarora

Differential Revision: D64259953

fbshipit-source-id: fd7ed90aed2ff6b6351460dcf7b61058c59cd25b
Summary:
Pull Request resolved: #3942

Reverted splitting of partition string to align with previous behavior of finding partition intersection rather than the union.
Also changed util dependency from laser to vector_search, which makes more sense.

Reviewed By: kuarora

Differential Revision: D64274531

fbshipit-source-id: 4614ae42d0fb534c9c9ce3314fd3c26a0c74d049
#3939)

Summary:
Add `cmake/link_to_faiss_lib.cmake`, which exposes a useful and reusable CMake `link_to_faiss_lib()` function

Pull Request resolved: #3939

Reviewed By: mnorris11

Differential Revision: D64250261

Pulled By: mengdilin

fbshipit-source-id: bab5b7fab8effb33cb73024eb7eefd2319998e5b
Summary:
#2943 had removed about SVE information (added on #2886 ) on the installation document. This PR fixes it.

This PR changes only the document, so it doesn't affect software behavior.

Pull Request resolved: #3915

Reviewed By: asadoughi

Differential Revision: D63967842

Pulled By: ramilbakhshyiev

fbshipit-source-id: ce0a0bfe591cb75b504cdf6362b5e8ed156928d5
Summary:
Pull Request resolved: #3948

I noticed this file I added was violating the header check. https://www.internalfb.com/intern/opensource/github/repo/1812028399049977/checkup/

Reviewed By: asadoughi

Differential Revision: D64341054

fbshipit-source-id: 4564191661acbff193c8ffe970582cef8fb3a490
Summary:
related: #2884

I added some SVE implementations of:

- `code_distance`
    - `distance_single_code`
    - `distance_four_codes`
- `exhaustive_L2sqr_blas_cmax_sve`
- `fvec_inner_products_ny`
- `fvec_madd`

## Evaluation result

I evaluated the search for SIFT1M dataset on AWS EC2 c7g.large and r8g.large instances.
`main` is the current (2e6551f) implementation.

### c7g.large (Graviton 3)

![g3_sift1m](https://github.com/user-attachments/assets/9c03cffa-72d1-4c77-9ae8-0ec0a5f5a6a5)

![g3_ivfpq](https://github.com/user-attachments/assets/4a8dfcc8-823c-4c31-ae79-3f4af9be28c8)

On Graviton 3, `IndexIVFPQ` has been improved particularly. In the best case (IndexIVFPQ + IndexFlatL2, M: 32), this PR is approx. 2.38-~~2.50~~**2.44**x faster than `main` .

- nprobe: 1, 0.069ms/query → 0.029ms/query
- nprobe: 4, 0.181ms/query → ~~0.074~~**0.075**ms/query
- nprobe: 16, 0.613ms/query → ~~0.245~~**0.251**ms/query

### r8g.large (Graviton 4)

![g4_sift1m](https://github.com/user-attachments/assets/e8510163-49d2-4143-babe-d406e2e40398)

![g4_ivfpq](https://github.com/user-attachments/assets/dc9a3ae0-a6b5-4a07-9898-c6aff372025c)

On Graviton 4, especially `IndexIVFPQ` for tiny `nprobe` has been improved. In the best case (IndexIVFPQ + IndexFlatL2, M: 8, nprobe: 1), this PR is approx. 1.33x faster than `main` (0.016ms/query → 0.012ms/query).

Pull Request resolved: #3933

Reviewed By: mengdilin

Differential Revision: D64249808

Pulled By: asadoughi

fbshipit-source-id: 8a625f0ab37732d330192599c851f864350885c4
…G module" (#3954)

Summary:
Pull Request resolved: #3954

Original commit changeset: 0c1cd25eabbf

Original Phabricator Diff: D63991471

Reviewed By: mengdilin, asadoughi

Differential Revision: D64439886

fbshipit-source-id: cc90958f6d90a429a4eece8e1cd1322b20d9aceb
Summary:
Pull Request resolved: #3959

Our upcoming compiler upgrade will require us not to have shadowed variables. Such variables have a _high_ bug rate and reduce readability, so we would like to avoid them even if the compiler was not forcing us to do so.

This codemod attempts to fix an instance of a shadowed variable. Please review with care: if it's failed the result will be a silent bug.

**What's a shadowed variable?**

Shadowed variables are variables in an inner scope with the same name as another variable in an outer scope. Having the same name for both variables might be semantically correct, but it can make the code confusing to read! It can also hide subtle bugs.

This diff fixes such an issue by renaming the variable.

 - If you approve of this diff, please use the "Accept & Ship" button :-)

Reviewed By: meyering

Differential Revision: D64398686

fbshipit-source-id: 44c60ea6e99d9542acf5af15adba6cdccda95577
…viceProperties (#3950)

Summary:
This diff enables to cache the device major version value so getCudaDeviceProperties() doesn't need to be called multiple times. Currently, the profiler of the code looks as so:
{F1933796291}

Pull Request resolved: #3950

Test Plan:
N5114369 -- provides a toy example (2) which exhibits the following timings:
Average timings before change: 3.35s
Average tmings after change:  1.99s

Reviewed By: algoriddle

Differential Revision: D64047778

Pulled By: mlomeli1

fbshipit-source-id: 2f09373944237e80b96d40f35c6714c06f5741a9
Summary:
Pull Request resolved: #3958

Our upcoming compiler upgrade will require us not to have shadowed variables. Such variables have a _high_ bug rate and reduce readability, so we would like to avoid them even if the compiler was not forcing us to do so.

This codemod attempts to fix an instance of a shadowed variable. Please review with care: if it's failed the result will be a silent bug.

**What's a shadowed variable?**

Shadowed variables are variables in an inner scope with the same name as another variable in an outer scope. Having the same name for both variables might be semantically correct, but it can make the code confusing to read! It can also hide subtle bugs.

This diff fixes such an issue by renaming the variable.

 - If you approve of this diff, please use the "Accept & Ship" button :-)

Reviewed By: meyering

Differential Revision: D64398701

fbshipit-source-id: 9f7b417bf6e8da6758f9cac4167a8b581bfed8b7
Summary:
Pull Request resolved: #3961

Our upcoming compiler upgrade will require us not to have shadowed variables. Such variables have a _high_ bug rate and reduce readability, so we would like to avoid them even if the compiler was not forcing us to do so.

This codemod attempts to fix an instance of a shadowed variable. Please review with care: if it's failed the result will be a silent bug.

**What's a shadowed variable?**

Shadowed variables are variables in an inner scope with the same name as another variable in an outer scope. Having the same name for both variables might be semantically correct, but it can make the code confusing to read! It can also hide subtle bugs.

This diff fixes such an issue by renaming the variable.

 - If you approve of this diff, please use the "Accept & Ship" button :-)

Reviewed By: meyering

Differential Revision: D64398743

fbshipit-source-id: 3ec24a1655133ee0d3b94a55e38857ffa8853268
Summary:
Pull Request resolved: #3960

Our upcoming compiler upgrade will require us not to have shadowed variables. Such variables have a _high_ bug rate and reduce readability, so we would like to avoid them even if the compiler was not forcing us to do so.

This codemod attempts to fix an instance of a shadowed variable. Please review with care: if it's failed the result will be a silent bug.

**What's a shadowed variable?**

Shadowed variables are variables in an inner scope with the same name as another variable in an outer scope. Having the same name for both variables might be semantically correct, but it can make the code confusing to read! It can also hide subtle bugs.

This diff fixes such an issue by renaming the variable.

 - If you approve of this diff, please use the "Accept & Ship" button :-)

Reviewed By: meyering

Differential Revision: D64398709

fbshipit-source-id: b10e44b40aa1d3e21aeb5112eb93fb63d64d4118
Summary:
Pull Request resolved: #3952

Our upcoming compiler upgrade will require us not to have shadowed variables. Such variables have a _high_ bug rate and reduce readability, so we would like to avoid them even if the compiler was not forcing us to do so.

This codemod attempts to fix an instance of a shadowed variable. Please review with care: if it's failed the result will be a silent bug.

**What's a shadowed variable?**

Shadowed variables are variables in an inner scope with the same name as another variable in an outer scope. Having the same name for both variables might be semantically correct, but it can make the code confusing to read! It can also hide subtle bugs.

This diff fixes such an issue by renaming the variable.

 - If you approve of this diff, please use the "Accept & Ship" button :-)

Reviewed By: asadoughi

Differential Revision: D64398749

fbshipit-source-id: 0e8fd4ab8f6dbf780d4412083fa88fc0df3b89c2
@aalekhpatel07 aalekhpatel07 merged commit cbcf8da into aalekhpatel07:main Oct 17, 2024
aalekhpatel07 pushed a commit that referenced this pull request Oct 17, 2024
…Lists (facebookresearch#3327)

Summary:
Pull Request resolved: facebookresearch#3327

**Context**
1. [Issue 2621](facebookresearch#2621) discuss inconsistency between OnDiskInvertedList and InvertedList. OnDiskInvertedList is supposed to handle disk based multiple Index Shards. Thus, we should name it differently when merging invls from index shard.
2. [Issue 2876](facebookresearch#2876) provides usecase of shifting ids when merging invls from different shards.

**In this diff**,
1. To address #1 above, I renamed the merge_from function to merge_from_multiple without touching merge_from base class.
why so? To continue to allow merge invl from one index to ondiskinvl from other index.

2. To address Enet4#2 above, I have added support of shift_ids in merge_from_multiple to shift ids from different shards. This can be used when each shard has same set of ids but different data. This is not recommended if id is already unique across shards.

Reviewed By: mdouze

Differential Revision: D55482518

fbshipit-source-id: 95470c7449160488d2b45b024d134cbc037a2083
aalekhpatel07 pushed a commit that referenced this pull request Oct 17, 2024
…ookresearch#3527)

Summary:
Pull Request resolved: facebookresearch#3527

**Context**
Design Doc: [Faiss Benchmarking](https://docs.google.com/document/d/1c7zziITa4RD6jZsbG9_yOgyRjWdyueldSPH6QdZzL98/edit)

**In this diff**
1. Be able to reference codec and index from blobstore (bucket & path) outside the experiment
2. To support #1, naming is moved to descriptors.
3. Build index can be written as well.
4. You can run benchmark with train and then refer it in index built and then refer index built in knn search. Index serialization is optional. Although not yet exposed through index descriptor.
5. Benchmark can support index with different datasets sizes
6. Working with varying dataset now support multiple ground truth. There may be small fixes before we could use this.
7. Added targets for bench_fw_range, ivf, codecs and optimize.

**Analysis of ivf result**: D58823037

Reviewed By: algoriddle

Differential Revision: D57236543

fbshipit-source-id: ad03b28bae937a35f8c20f12e0a5b0a27c34ff3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.