No global vars by Ngalstyan4 · Pull Request #51 · lanterndata/lantern

Ngalstyan4 · 2023-08-14T00:00:31Z

Update usearch API to take a postgres context and pass it to all external retrievers(the context is initialized and passed to usearch iv usearch_init, and it is passed back to postgress in all retriever calls).
Move extra_dirtied page tracking to per-index-operation context
Move dirtied header to per-index-operation context and get rid of that last global variable
~~Rethink variables names and, generally, cleanup external_index.c~~ (cleaned up a little, needs more work but let's do that later)

Incorporates the commit from #67 and closes #67

codecov · 2023-08-14T04:16:43Z

Codecov Report

Merging #51 (3655eb4) into main (969cc91) will increase coverage by 0.24%.
The diff coverage is 89.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   82.86%   83.10%   +0.24%     
==========================================
  Files           9       11       +2     
  Lines         887      876      -11     
  Branches      179      165      -14     
==========================================
- Hits          735      728       -7     
+ Misses         71       67       -4     
  Partials       81       81

Files Changed	Coverage Δ
src/hnsw/build.c	`86.73% <75.00%> (-1.15%)`	⬇️
src/hnsw/retriever.c	`83.33% <83.33%> (ø)`
src/hnsw/extra_dirtied.c	`86.11% <86.11%> (ø)`
src/hnsw/external_index.c	`86.22% <89.15%> (+1.19%)`	⬆️
src/hnsw.c	`84.11% <100.00%> (+0.14%)`	⬆️
src/hnsw/insert.c	`84.41% <100.00%> (-0.40%)`	⬇️
src/hnsw/scan.c	`76.47% <100.00%> (+0.28%)`	⬆️

src/hnsw/external_index.h

davkhech · 2023-08-15T18:51:14Z

src/hnsw/external_index.h

+    uint32      vector_dim;
+    uint32      num_vectors;
+    BlockNumber last_data_block;
+    uint32      blockno_index_start;


I think we can get rid of this one as well. Maybe in separate PR though

davkhech · 2023-08-15T18:52:32Z

src/hnsw/extra_dirtied.c

+    return ed;
+}
+
+void extra_dirtied_add(ExtraDirtiedBufs *ed, BlockNumber blockno, Buffer buf, Page page)


maybe not good suggestion, but I'd rather pass the context and then get the buffers from the context here

davkhech

LGTM

var77 · 2023-08-20T07:06:05Z

scripts/run_all_tests.sh

 TMP_OUTDIR=$TMP_ROOT/tmp_output
 FILTER="${FILTER:-}"
-DB_USER="${DB_USER:-}"
+DB_USER="${DB_USER:-postgres}"


I think we can set default to os user

Suggested change

DB_USER="${DB_USER:-postgres}"

DB_USER="${DB_USER:-$USER}"

Good catch! fixed, and also made sure to use "postgres" in place of "root" when root is the user.
This is useful for when we are in docker containers

davkhech · 2023-08-20T12:14:57Z

test/sql/results/..out.diff

remove empty file

davkhech · 2023-08-20T12:15:07Z

test/sql/results/debug_helpers.sql.out.diff

remove empty file

* if the user is root, this is typically in docker and the database is usually under postgres user * if the user is something else, typically a psql user exists for the user so we attempt to use that. * we set the user once in run_all_tests instead of running the same logic again in test_runner

Now when usearch external retriever is initialized, an opeque is passed in which then is passed back in retrievers. This way, we no longer need to have global variables to manage memory on which external index nodes live.

Get rid of num_blocks in index header Cleanup and format

…51) * Cancel running embedding job with tokio tasks * Gracefully exit ongoing embedding job if the job is canceled from db * Delete collected insert rows on job cancel

davkhech reviewed Aug 15, 2023

View reviewed changes

src/hnsw/external_index.h Show resolved Hide resolved

davkhech reviewed Aug 15, 2023

View reviewed changes

Ngalstyan4 force-pushed the feature/narek-no-global-vars branch from 7116a05 to f92da5c Compare August 18, 2023 04:36

davkhech approved these changes Aug 18, 2023

View reviewed changes

Ngalstyan4 force-pushed the feature/narek-no-global-vars branch 3 times, most recently from a5922de to 2836a03 Compare August 20, 2023 04:29

Ngalstyan4 changed the title ~~[WIP] no global vars~~ No global vars Aug 20, 2023

Ngalstyan4 requested a review from davkhech August 20, 2023 04:35

Ngalstyan4 marked this pull request as ready for review August 20, 2023 04:35

Ngalstyan4 requested review from var77 and removed request for davkhech August 20, 2023 04:36

Ngalstyan4 force-pushed the feature/narek-no-global-vars branch 2 times, most recently from 4fa5175 to 65ead0b Compare August 20, 2023 05:25

var77 reviewed Aug 20, 2023

View reviewed changes

var77 approved these changes Aug 20, 2023

View reviewed changes

davkhech reviewed Aug 20, 2023

View reviewed changes

test/sql/results/..out.diff

Copy link
Copy Markdown

Contributor

davkhech Aug 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty file

Ngalstyan4 reacted with thumbs up emoji

davkhech reviewed Aug 20, 2023

View reviewed changes

test/sql/results/debug_helpers.sql.out.diff

Copy link
Copy Markdown

Contributor

davkhech Aug 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty file

Ngalstyan4 reacted with thumbs up emoji

Ngalstyan4 force-pushed the feature/narek-no-global-vars branch from eb7db07 to 28ea3a7 Compare August 20, 2023 22:03

Ngalstyan4 added 9 commits August 20, 2023 22:07

Address nits + update usearch to latest

b7b38c4

Run clang-format on headers too

58f6d37

Transition to contexted external retriever

c48a51a

Now when usearch external retriever is initialized, an opeque is passed in which then is passed back in retrievers. This way, we no longer need to have global variables to manage memory on which external index nodes live.

Get rid of most global vars (INDEX_HEADER remaining)

826870b

Get rid of num_blocks in index header Cleanup and format

Remove redundant variable from CMakeLists

9fdfd00

Remove the last remaining global variable from hnsw

f6808a6

Use assert in place of elog for an invariant violation

9311817

Enforce minimum clang-format version to avoid inconsistent reformatting

81fff45

Ngalstyan4 force-pushed the feature/narek-no-global-vars branch from 28ea3a7 to 81fff45 Compare August 20, 2023 22:11

Remove unused function arguments

3655eb4

Ngalstyan4 merged commit c2ea66d into main Aug 20, 2023

Ngalstyan4 deleted the feature/narek-no-global-vars branch August 20, 2023 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No global vars#51

No global vars#51
Ngalstyan4 merged 10 commits intomainfrom
feature/narek-no-global-vars

Ngalstyan4 commented Aug 14, 2023 •

edited

Loading

Uh oh!

codecov bot commented Aug 14, 2023 •

edited

Loading

Uh oh!

Uh oh!

davkhech Aug 15, 2023

Uh oh!

davkhech Aug 15, 2023

Uh oh!

davkhech left a comment

Uh oh!

var77 Aug 20, 2023 •

edited

Loading

Uh oh!

Ngalstyan4 Aug 20, 2023

Uh oh!

davkhech Aug 20, 2023

Uh oh!

davkhech Aug 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Ngalstyan4 commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

davkhech Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

davkhech Aug 15, 2023

Choose a reason for hiding this comment

Uh oh!

davkhech left a comment

Choose a reason for hiding this comment

Uh oh!

var77 Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngalstyan4 Aug 20, 2023

Choose a reason for hiding this comment

Uh oh!

davkhech Aug 20, 2023

Choose a reason for hiding this comment

Uh oh!

davkhech Aug 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ngalstyan4 commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 14, 2023 •

edited

Loading

var77 Aug 20, 2023 •

edited

Loading