Skip to content

bindings/python: align max_connections default with engine #5352 (16 -> 32) + document Vamana semantics - #5355

Closed
tae898 wants to merge 1 commit into
ArcadeData:mainfrom
humemai:python-bindings
Closed

bindings/python: align max_connections default with engine #5352 (16 -> 32) + document Vamana semantics#5355
tae898 wants to merge 1 commit into
ArcadeData:mainfrom
humemai:python-bindings

Conversation

@tae898

@tae898 tae898 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Follows up #5352: the Python wrapper (create_vector_index) carried its own max_connections=16 default, now inconsistent with the engine's new default of 32. This aligns the wrapper and updates all bindings docs/examples with the documented semantics (per-layer Vamana degree, not hnswlib M; use 2*M to reproduce an hnswlib configuration).

  • wrapper default 16 -> 32, docstring documents the 2*M mapping
  • docs (api/vector, api/schema, api/database, guide/vectors, testing) and examples 11/13 updated
  • full suite green (353 passed)

🤖 Generated with Claude Code

https://claude.ai/code/session_01KPPtpXXDs5EZFmx9DAPnRB

…-20)

Regenerated by make-upstream-pr-branch.sh: bindings/python brought to the fork's main state. Scope: the minimum needed to reproduce the wheel build and run the tests and examples.
@mergify

mergify Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Python bindings and examples. It replaces references to the "real_ladybug" package with "ladybug" in the StackOverflow OLTP and OLAP examples. Additionally, it increases the default value for "max_connections" (or "maxConnections") from 16 to 32 across vector index building, hybrid queries, and the core embedded API, updating the corresponding documentation and help messages to reflect this change and explain its relation to JVector/Vamana per-layer degree. There are no review comments, so no feedback is provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@tae898

tae898 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Checking in on this one, and asking a question rather than just nudging.

State as I read it: mergeable, one commit, bindings/python only, no engine files touched, and the two comments on it are both Codacy reporting zero new issues. The unstable check state looks like it is inherited from main rather than from anything here. ha-integration-tests and integration-tests have been failing on main itself for the last several Java CI - test runs, independently of any PR (that is the thing I flagged separately on #5518, and it is worth its own issue since a real regression would currently be indistinguishable from the noise).

The question. We have a second bindings/python contribution ready and I would rather ask how you want it than guess. It is the Python half of #5474, which does not currently exist upstream: append_samples(primitive=True) plus a small TimeSeriesBatcher Java helper so each column crosses the FFI boundary once instead of per element.

Measured on TSBS, released 26.8.1.dev21, mini, N=3:

arm pts/s vs previous
Python lists + Object[] 417,393 baseline
numpy arrays + Object[] 1,289,054 3.09x, no engine change
numpy arrays + TimeSeriesBatch 1,730,039 1.34x

The 3.09x was our own adapter's fault and is already fixed on our side: it was passing Python lists, taking the per-element conversion path instead of the bulk array path the binding has always had. The 1.34x on top is your new primitive TimeSeriesBatch API actually being reached from Python, which is the part that would be useful upstream. Together they take DuckDB's lead on that lane from 4.7x to 1.12x.

So: separate PR against main, or stacked on this one? Stacking keeps the two bindings/python changes from conflicting, but it also means this one has to land first, and I do not want to make that your problem if you would rather take them independently. Happy either way, just say which.

@robfrank

Copy link
Copy Markdown
Collaborator

Thanks for the follow-up, @tae898, and thanks especially for catching that the wrapper was shadowing the engine default. That one was easy to miss: core.py calls builder.withMaxConnections(...) unconditionally, so the wrapper's 16 was silently overriding the engine's 32 for every Python user who didn't pass the argument explicitly. Good find, and the docstring you wrote tracks the engine Javadoc nicely.

A few things I'd like to work through with you before we merge.

Example 11 and the shared --max-connections flag

This is the one I'd most like your thoughts on. In 11_vector_index_build.py, the single --max-connections flag feeds six backends that don't agree on what the number means:

  • arcadedb_sql (line 588): "maxConnections", a Vamana per-layer degree, applied as-is
  • faiss (617), lancedb (699), pgvector (843), qdrant (867), milvus (1299): all hnswlib-style M, which doubles to 2*M at the base layer

So at the new default of 32, ArcadeDB builds at degree 32 while the other five build at an effective 64. That's the same 2:1 mismatch #5352 identified, just shifted up a notch, and it now costs the other engines extra build time too.

Would it make sense to keep --max-connections as the ArcadeDB degree and pass max_connections // 2 to the five HNSW backends? That would make the comparison degree-matched, which I think is the outcome the issue was really after. A separate --hnsw-m flag would work too if you'd rather keep them independent. Whichever way you go, the help string probably wants to say which backend it's describing, since right now it reads as Vamana semantics but is consumed as hnswlib M in five of six places.

The description mentions docs that aren't in the diff

The body lists updates to api/vector, api/schema, api/database, guide/vectors, and testing, but there's no docs/ directory under bindings/python in this repo, so I think those live only in your fork. No problem at all, just worth trimming the description so the next reviewer isn't looking for files that can't land here.

The real_ladybug to ladybug rename

I went and checked PyPI: real-ladybug 0.15.3 (lbugdb/lbug) and ladybug 0.19.0 (LadybugDB/ladybug) are indeed the same project under its new name, so the rename itself is right. Two small things. It's an unpinned 0.15 to 0.19 jump on a 0.x package, where the API can move, and CI runs examples 09 and 10 with --db arcadedb_cypher (test-python-examples.yml:357,363), so that path never actually gets exercised here. Could you pin the version in the packages.append(...) list and add a line about the swap to the description? It's currently invisible to anyone skimming the PR.

A test to keep the two defaults tied together

bindings/python/tests has no reference to max_connections today, which is probably why the drift happened in the first place. Rather than asserting a literal 32, which would just reset the same clock, would you be up for a small test that reads LSMVectorIndexMetadata().maxConnections through JPype and asserts the wrapper's signature default matches it? Then the two can't separate again.

Relatedly, CI still passes --max-connections 16 for example 11 (test-python-examples.yml:369), so the new default isn't exercised end to end. Dropping that flag would be a nice touch if you're already in there.

Small, entirely optional

While you're near it: examples 03 and 06 both print beam_width: 256 but never set it, so they actually build at the engine default of 100. Your change makes their max_connections: 32 line true, and it'd be satisfying to make the beam_width line honest in the same pass. Completely fine to leave for another day.

The core.py and 13_*.py changes look good to me as they stand. It's really just example 11 and the description I'd want sorted before merge. Thanks again for staying on top of this one.

@robfrank

Copy link
Copy Markdown
Collaborator

@tae898 rather than leave this sitting, I went ahead and did the follow-ups myself. They are in #5617, which keeps your original commit and authorship intact and adds five commits on top.

I tried to push them straight to this branch first, since maintainer edits are enabled. That does not work here: humemai is an organization, and GitHub does not honor maintainer edits on org-owned forks, so a separate PR was the only route. Nothing of yours was changed or reverted.

What I added:

  • example 11 now converts the ArcadeDB degree to hnswlib M for faiss, lancedb, pgvector, qdrant and milvus, so all six backends build at the same base-layer density. Without it the new default compared ArcadeDB at degree 32 against an effective 64 elsewhere.
  • a regression test that reads LSMVectorIndexMetadata.maxConnections through JPype and asserts the wrapper default equals it, so this drift cannot come back.
  • CI no longer pins --max-connections 16 for the example 11 smoke test.
  • ladybug pinned to 0.19.0, since the rename crosses four minors on a 0.x package and no CI job exercises that path.
  • examples 03 and 06 print the beam width they actually build with.

I also dropped the docs paragraph from the description, since those files live only in your fork.

If you would rather land this yourself, say so and I will close #5617 and hand the patch back, or you can grant push access on the fork and I will move the commits over. Happy to take either. Thanks again for catching the shadowed default, which was a genuine bug that would have quietly halved base-layer density for every Python user who did not pass the argument.

On your TSBS question from the 29th, that deserves its own thread and I will reply there separately.

robfrank added a commit that referenced this pull request Jul 31, 2026
…ersedes #5355) (#5617)

Co-authored-by: Taewoon Kim <taewoon@humem.ai>
@tae898

tae898 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #5617, which is merged. It carries this commit with its authorship intact, plus the degree-matching fix for example 11, a regression test that reads maxConnections off the index metadata rather than asserting a literal, and CI coverage for the aligned default.

Closing in favour of that. The docs updates named in the description above live in humemai/arcadedb-embedded-python, which has no counterpart here, so nothing is lost by closing this.

@tae898 tae898 closed this Jul 31, 2026
robfrank added a commit that referenced this pull request Aug 14, 2026
…ersedes #5355) (#5617)

Co-authored-by: Taewoon Kim <taewoon@humem.ai>
(cherry picked from commit 924c12f)
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.

2 participants