bindings/python: align max_connections default with engine #5352 (supersedes #5355)

[robfrank]

Supersedes #5355, preserving @tae898's original commit and authorship. Opened as a separate PR only because humemai is an organization-owned fork, and GitHub does not honor maintainer edits on org-owned forks, so the fixes could not be pushed to the original branch.

Follows up #5352. The Python wrapper (create_vector_index) carried its own max_connections=16 default, which shadowed the engine's new default of 32 because the wrapper always calls withMaxConnections. This aligns the wrapper and documents the semantics: maxConnections is a Vamana per-layer degree, not hnswlib M, so reproducing an hnswlib configuration takes 2*M.

Original contribution by @tae898

• wrapper default 16 -> 32, docstring documents the 2*M mapping
• example 13 index metadata 16 -> 32
• example 11 CLI default 16 -> 32
• examples 09 and 10 follow the upstream rename of real_ladybug to ladybug

Maintainer follow-ups

• example 11 degree-matching. One --max-connections flag fed six backends with two different meanings. ArcadeDB applies the value verbatim to every layer; faiss, lancedb, pgvector, qdrant and milvus are hnswlib-derived and double it at the base layer. At the new default that compared ArcadeDB at degree 32 against an effective 64 elsewhere. A hnsw_m_from_max_connections() helper now converts at each call site, and the four hnsw_m entries in the results metadata report the converted value.
• a regression test that cannot drift. bindings/python/tests had no reference to max_connections, which is how the wrapper and engine separated in the first place. The new test reads LSMVectorIndexMetadata.maxConnections through JPype and asserts the wrapper default equals it, rather than asserting a literal 32.
• CI exercises the default. The example 11 smoke test pinned --max-connections 16, so the aligned default never ran end to end.
• ladybug pinned to 0.19.0. 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 (100, the engine default), instead of an unset 256.

Verification

Full Python suite against a wheel built from this branch: 351 passed, 11 skipped. Six of the skips are test_docs_examples.py, which skips because bindings/python/docs does not exist in this repository; those run in the contributor's fork, which accounts for the difference from the 353 reported on #5355.

The drift test was proven to fail before being trusted: with the installed default flipped to 16, both new tests fail assert 16 == 32; restored to 32, all 13 tests in that file pass.

Note: the docs updates mentioned in #5355's description live in humemai/arcadedb-embedded-python. This repository has no bindings/python/docs directory, so nothing here covers them.

[mergify]

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

• Queue this pull request

[codacy-production]

Not up to standards ⛔

🔴 Issues 7 high · 4 minor

Alerts:
⚠ 11 issues (≤ 0 issues of at least minor severity)

Results:
11 new issues

Category	Results
Documentation	4 minor
Security	7 high

View in Codacy

🟢 Metrics 8 complexity

Metric	Results
Complexity	8

View in Codacy

🟢 Coverage ∅ diff coverage · -5.72% coverage variation

Metric	Results
Coverage variation	✅ -5.72% coverage variation
Diff coverage	✅ ∅ diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (2f985b8)	140939	105044	74.53%
Head commit (18c00b8)	174076 (+33137)	119782 (+14738)	68.81% (-5.72%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5617)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{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.}

[claude]

PR Review: align max_connections default with engine (#5352, supersedes #5355)

Reviewed the full diff and cross-checked against the engine. Clean, well-reasoned change with good tests.

What is correct and verified

• Wrapper/engine alignment is real. LSMVectorIndexMetadata.maxConnections is = 32 at declaration (engine/.../LSMVectorIndexMetadata.java:52) and the constructor does not override it, so the drift test constructing LSMVectorIndexMetadata("DefaultProbe", property_names, 0) genuinely reads the engine default. The (String, String[], int) constructor signature matches. The test asserts against the live jar rather than a literal - the right way to keep this from re-drifting.
• The 2*M semantics are documented consistently across the core.py docstring, the engine Javadoc, and the example help text.
• hnsw_m_from_max_connections is applied at every faiss/lancedb/pgvector/qdrant/milvus call site and in the results metadata, so the reported hnsw_m matches what is actually built. The base-layer-density reasoning is sound and the max(1, ...) clamp avoids M=0.
• CI now exercises the default (dropping --max-connections 16 from the smoke test) - this is the gap that let the wrapper and engine separate originally, so closing it is the most valuable part.
• Examples 03/06 prints now match the SQL: both create the index with only dimensions/similarity, so engine defaults (32 / 100) do apply. Accurate now.

Minor observations (non-blocking)

1. Examples 03/06 still hardcode max_connections: 32 / beam_width: 100 as print strings. Same drift class this PR fixes: if the engine default changes again these prints go stale silently. Low priority since they are illustrative prints, but they could read the values back from the created index metadata (as the tests do) to be drift-proof.
2. hnsw_m_from_max_connections accepts str by design (tested); a non-numeric string would raise ValueError from int(). Fine given argparse type=int guarantees an int at the real call sites - the string test is just defensive. No change needed.
3. Pinning ladybug==0.19.0 is reasonable for reproducibility on a 0.x package with no CI coverage; worth a periodic bump reminder so it does not rot.

Coverage
Test coverage is the strong point: the drift test, the omitted-arg-reaches-the-index end-to-end assertion, and the hnsw_m conversion unit tests target the exact failure modes described. The stated verification (351 passed / 11 skipped, plus the deliberate red-then-green proof of the drift test) is convincing.

Nice work preserving @tae898 authorship and layering the maintainer follow-ups as separate, well-described commits.

[tae898]

Thanks for picking this up, and for keeping the authorship on the original commit.

The example 11 catch is the one that matters. My change would have shipped a comparison of ArcadeDB at degree 32 against an effective 64 on the hnswlib-derived backends, which is exactly what #5352 was about, one level down. The test reading maxConnections off the index metadata rather than asserting a literal 32 is also the right shape, since a literal is how the wrapper and engine drifted apart in the first place.

One scope note, not a blocker: the beam width prints in examples 03 and 06 are unrelated to max_connections. Correct as far as I can tell, just flagging in case you would rather keep this PR to the one change and take those separately.

Happy for this to supersede #5355.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.00%. Comparing base (2f985b8) to head (18c00b8).
⚠️ Report is 301 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5617      +/-   ##
============================================
+ Coverage     65.63%   67.00%   +1.36%     
============================================
  Files          1717     1718       +1     
  Lines        140939   142084    +1145     
  Branches      30174    30521     +347     
============================================
+ Hits          92506    95202    +2696     
+ Misses        35936    34211    -1725     
- Partials      12497    12671     +174     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[robfrank]

Addressed observation 1, took observation 3 as a code comment, and left observation 2 alone. Pushed as 18c00b8.

1. Hardcoded prints in examples 03/06 — fixed rather than left.

You are right that this is the same drift class the PR exists to fix, and leaving a known instance of it in the same PR would be inconsistent. Both examples now read the values off the created index via the public db.schema.get_vector_index(type, prop).get_metadata() and print dimensions, similarity_function, max_connections and beam_width from there, so nothing is restated.

Example 03 needed a reorder: the parameter block printed before the CREATE INDEX, so there was nothing to read back. Moving it after the create also reads better, since it now reports what was built rather than what was intended.

Verification, since these are the two examples with the weakest coverage:

• example 03 is not in the CI example matrix, so I ran it end to end locally. Exit 0, and it prints dimensions: 384 / distance_function: COSINE / max_connections: 32 / beam_width: 100, read from the index.
• example 06 needs sentence-transformers and a MovieLens database, so I could not run it here. It is in CI (test-python-examples.yml). Its only material difference from the proven path is a suffixed property name, so I probed exactly that in isolation: create Movie.embedding_v1 via the same SQL, then get_vector_index("Movie", "embedding_v1").get_metadata() returns COSINE 32 100. CI covers the rest.

Full suite still 351 passed / 11 skipped.

2. hnsw_m_from_max_connections accepting str. Agreed, no change. argparse type=int guarantees an int at every real call site; the string case is only there so the helper is not silently dependent on that guarantee.

3. Ladybug pin rot. Added a comment at both pin sites recording why the pin exists (pre-1.0, no CI coverage) and to re-check it when refreshing published results. A comment beats a tracking issue here since it is visible exactly where someone would bump it.

One thing worth separating out, unrelated to this diff: while building a wheel to verify the drift test I hit bindings/python/scripts/build.sh returning exit 0 on a failed build twice, once for a missing build module and once for six javac errors, leaving a stale wheel in dist/ that looks like success. That is a real footgun in our own tooling and I would rather file it on its own than fold it in here.

[claude]

Review

Reviewed the full diff against the codebase. This is a tidy, well-reasoned PR: it's Python-bindings/examples/CI only, no engine changes, and each commit is narrowly scoped with a clear rationale. Nice work, and the "prove the test fails before trusting it" note in the description is exactly the right discipline.

I verified the load-bearing details against the repo:

• test_vector_params_verification.py probes com.arcadedb.schema.LSMVectorIndexMetadata("DefaultProbe", propertyNames, 0) — the 3-arg (String, String[], int) constructor exists (LSMVectorIndexMetadata.java:72) and maxConnections defaults to 32 at the field initializer (:52), so the "read the default off a fresh metadata object rather than a literal" approach is sound and genuinely drift-proof.
• Examples 03/06 read back via db.schema.get_vector_index(...).get_metadata(), and the metadata dict does expose the keys used (dimensions, similarity_function, max_connections, beam_width in vector.py:508-512). Type names (Article, Movie/embedding{suffix}) line up with the CREATE INDEX statements above each call.
• hnsw_m_from_max_connections — the max(1, int(x) // 2) conversion and its three tests (half-of-degree, floor-at-1, string input) are correct and the docstring accurately captures the Vamana-vs-hnswlib base-layer distinction.

Minor observations (non-blocking)

1. get_vector_index(...).get_metadata() assumes a non-None return. Schema.get_vector_index is documented to return Optional (None when not found). In examples 03/06 the index is created immediately above, so this is safe in practice — but a lookup miss would surface as a bare AttributeError: 'NoneType' object has no attribute 'get_metadata' rather than a clear message. Since these are teaching examples, a one-line guard (or a brief comment noting the index was just created) would make a failure less cryptic. Purely cosmetic.

2. Docstring vs. behavior wording for max_connections in core.py says "matching the engine default since LSM_VECTOR maxConnections is Vamana per-layer degree, not hnswlib M: document the 2x mapping (degree-matched 10M curves: recall 0.875 -> 0.948 at same efSearch, no build-time cost) #5352" — good — but the value 32 is still a literal in the wrapper signature. That's unavoidable for a default argument, and the new test_wrapper_default_matches_engine_default is precisely what keeps it honest, so this is fine as-is; just flagging that the literal and the engine constant remain two sources tied only by the test.

3. CI smoke test now drops --max-connections 16 and relies on the aligned default — this correctly exercises the new default end-to-end. Worth a mental note that the smoke index now builds at double the previous density, so the timeout_duration=1200 budget should have headroom (looks fine for stackoverflow-tiny).

Test coverage

Good. The two new test files cover both the conversion helper and the wrapper/engine default tie, and both are structured to fail loudly if the two layers drift again — which is exactly the class of bug this PR fixes. The ladybug==0.19.0 pin is appropriately justified for a pre-1.0 package on an unexercised path.

No correctness, performance, or security concerns. LGTM.

[robfrank]

Correcting myself on the build tooling note at the end of my previous comment. I said scripts/build.sh "returns exit 0 on a failed build twice, once for a missing build module and once for six javac errors." That is wrong. It returned exit 1 on both runs.

I had run it as a compound command:

./scripts/build.sh ... > log 2>&1; echo "EXIT=$?"; tail -15 log

so the status I read back was the trailing tail's, not the script's. The one place I wrote a real EXIT= marker into the log was the run that succeeded, so the two failing runs carried no marker and I never cross-checked. On top of that I saw a wheel sitting in dist/ and took it as evidence the build had worked, which made the wrong story fit.

@tae898 is right in 5a8445e that the javac case fails properly: injecting a javac error does fail the build. Apologies for sending you after a defect that was not there.

What I actually hit was the narrower thing that commit fixes: a failed build leaves the previous wheel in dist/, so the presence of a wheel is not evidence this run produced one. In my case that was a 25.10.1 wheel surviving two failed 26.8.1 builds, which is exactly the stale-artifact trap. Checking version and mtime rather than existence is the lesson, and the marker-file comparison in that commit is a better fix than clearing the directory since it preserves wheels built for other platforms.

Two related things I ran into on the same path, in case they are useful and separate from that commit:

• src/arcadedb_embedded/jars/ is reused when it already exists, so a leftover 25.10.1 jar set made the bridge compile fail with cannot find symbol: class GraphBatch. The directory is untracked and not gitignored.
• The wheel I eventually produced contained both arcadedb-engine-25.10.1 and 26.8.1. The older sorts first on the classpath, so Schema resolved to the old class and every LSM_VECTOR test failed with INDEX_TYPE has no attribute LSM_VECTOR, which reads like a missing engine feature rather than shadowing.

None of this affects the diff under review here.

[tae898]

Merged, thanks. Reading the metadata back in examples 03/06 is a better fix than the one I had in mind, and reordering 03 so the block reports what was built rather than what was intended is the part I would not have thought to do.

Before you spend time filing the build.sh one: I went looking for it, and one half reproduces and one half does not.

The stale wheel is real, and fixed. The Docker path's success gate was ls dist/*.whl, which asks whether a wheel exists rather than whether this run produced one. Nothing clears dist/, so a wheel from any earlier build satisfied it, and an export stage that yields nothing still prints the success banner and exits 0. build-native.sh already avoided this with rm -f "$DIST_DIR"/*.whl before building; the Docker branch had no equivalent, which is why the two behave differently.

Fixed in humemai/arcadedb-embedded-python@5a8445e, which I will carry into the next bindings PR here rather than opening a separate one. It compares wheel identities across the extraction instead of clearing the directory, so wheels deliberately kept for other platforms survive a build, and a same-named wheel is accepted only when the extraction actually rewrote it (marker file plus test -nt, since find -newermt is GNU-only and this script runs on macOS hosts too). Four states, verified in isolation and then end to end:

dist/ before	this build produced	rc
empty	nothing	1 (unchanged)
empty	a wheel	0
stale wheel	nothing	1 (was 0)
same-name wheel	rewrote it	0

A full Docker build with a stale wheel planted in dist/ still exits 0, reports the new wheel, and leaves the planted one alone.

The exit-0-on-javac-errors half does not reproduce for me. I put a deliberately broken .java into the bridge sources and ran build.sh linux/amd64 3.12: javac reports its errors, the RUN fails, and the script exits 1 with no success banner. Same story for the missing build module on the native path, where build-native.sh pre-flights it and exits 1. I diffed build.sh, build-native.sh and Dockerfile.build against main here before starting, and all three are byte-identical to our copies, so it is not fork drift.

So either those two runs failed somewhere I have not found, or the exit 0 you saw was the stale-wheel gate downstream of a build that had already gone wrong. If you still have the scrollback, the last 20 lines and the platform would settle it. If it was the stale wheel, the fix above covers it and there is nothing left to file.