Share execution configuration across run paths (#134) - #219
Open
leynos wants to merge 4 commits into
Conversation
Carry validated clustering and HNSW settings from the builder into both batch execution and session construction. Use those parameters for CPU pipeline construction and memory estimates so one-shot runs honour the same tuning policy as sessions. Remove the weaker public CPU pipeline entrypoint, leaving `Chutoro::run` as the validated public batch path, and cover the configured memory estimate with a regression test.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Limit details: You’ve used all 3 included reviews currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Summary
Related issue
WalkthroughChangesCentralise validated execution settings in Execution configuration flow
Sequence Diagram(s)sequenceDiagram
participant ChutoroBuilder
participant Chutoro
participant CPU_Pipeline
ChutoroBuilder->>Chutoro: construct ExecutionConfig
ChutoroBuilder->>Chutoro: pass ExecutionConfig
Chutoro->>Chutoro: estimate memory from HnswParams
Chutoro->>CPU_Pipeline: pass HnswParams and min_cluster_size
CPU_Pipeline->>CPU_Pipeline: build HNSW index with supplied parameters
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 7 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideIntroduce a shared ExecutionConfig to centralize validated clustering and HNSW CPU parameters, and ensure both batch runs and sessions use the same configuration for execution, memory estimation, and pipeline construction. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
August 23, 2026 21:27
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f7727c63a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+223
to
+224
| let estimated = | ||
| crate::memory::estimate_peak_bytes(items, self.hnsw_params().max_connections()); |
There was a problem hiding this comment.
When a caller supplies HnswParams::with_distance_cache_max_entries with a capacity above the default, these parameters now reach CpuHnsw, but this estimate still passes only max_connections and estimate_peak_bytes assumes the default 1,048,576-entry cache. The pre-flight guard can therefore approve a run whose configured cache alone exceeds max_bytes, defeating the OOM protection; derive the cache contribution from the configured HnswParams and cover a non-default capacity.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate modules =='
fd -t f -e rs . chutoro-core | rg '(memory|hnsw|cpu_pipeline|execution_config|builder)\.rs$' || true
printf '%s\n' '== Symbol map =='
ast-grep outline chutoro-core --items all --type function,struct,impl \
--match 'estimate_peak_bytes|HnswParams|CpuHnsw|run_cpu_pipeline_with_len' || true
printf '%s\n' '== Configuration, estimator, and CPU propagation references =='
rg -n -C 5 \
'estimate_peak_bytes_for_hnsw_params|estimate_peak_bytes\s*\(|distance_cache_max_entries|with_distance_cache_max_entries|CpuHnsw|run_cpu_pipeline_with_len' \
chutoro-core --glob '*.rs' || trueLength of output: 135218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Parameter-aware memory estimator =='
sed -n '84,145p' chutoro-core/src/memory.rs
printf '%s\n' '== HNSW parameter cache accessors =='
sed -n '1,125p' chutoro-core/src/hnsw/params.rs
printf '%s\n' '== Distance-cache configuration =='
sed -n '1,120p' chutoro-core/src/hnsw/distance_cache.rs
printf '%s\n' '== CPU HNSW allocation =='
sed -n '135,185p' chutoro-core/src/hnsw/cpu/construction.rs
printf '%s\n' '== Cache-capacity estimator tests =='
rg -n -C 6 \
'distance_cache|max_entries|parameter_estimate|MemoryLimitExceeded|estimate_peak_bytes_for_hnsw_params' \
chutoro-core/src/memory.rs chutoro-core/tests/chutoro.rsLength of output: 19096
@leynos, keep this review thread open.
The latest commit does not resolve the finding.
The code graph shows that HnswParams reaches CpuHnsw::with_capacity. That constructor creates DistanceCache from params.distance_cache_config(). Therefore, a caller-provided with_distance_cache_max_entries value changes the actual allocation.
However, estimate_peak_bytes_for_hnsw_params passes only max_connections and effective_ef_construction into the estimator. The estimator still charges DEFAULT_CACHE_MAX_ENTRIES at chutoro-core/src/memory.rs, Line 137. It does not read hnsw_params.distance_cache_config().max_entries().
Use this AI agent prompt:
Fix the `Chutoro::run` memory-limit estimate so it accounts for the configured HNSW distance-cache capacity.
Trace the existing configuration flow first:
- `Chutoro::check_memory_limit` calls `estimate_peak_bytes_for_hnsw_params`.
- `Chutoro::run_cpu` passes the same `HnswParams` into `run_cpu_pipeline_with_len`.
- `run_cpu_pipeline_with_len` passes those parameters to `CpuHnsw::build_with_edges`.
- `CpuHnsw::with_capacity` creates `DistanceCache` from `params.distance_cache_config()`.
Update `chutoro-core/src/memory.rs`:
1. Change the parameter-aware estimator or its internal helper so it derives the distance-cache contribution from `hnsw_params.distance_cache_config().max_entries().get()`.
2. Preserve `estimate_peak_bytes(point_count, max_connections)` as the default-configuration API, if it is public API. It must retain default cache-capacity behaviour.
3. Use saturating arithmetic for the cache entry calculation.
4. Update comments and documentation so they do not claim that every CPU HNSW index allocates `DEFAULT_CACHE_MAX_ENTRIES`.
Add regression coverage:
1. Add a unit test proving that `estimate_peak_bytes_for_hnsw_params` produces a larger estimate for a parameter set with `with_distance_cache_max_entries` above `DistanceCacheConfig::DEFAULT_MAX_ENTRIES`.
2. Add an integration test in `chutoro-core/tests/chutoro.rs` that:
- Configures `HnswParams` with a non-default cache capacity above the default.
- Calculates the parameter-aware estimate.
- Sets `max_bytes` below that estimate but above the equivalent default-cache estimate.
- Calls `Chutoro::run`.
- Asserts that it returns `ChutoroError::MemoryLimitExceeded` with the parameter-aware estimate.
3. Use `NonZeroUsize` for the custom cache capacity.
Run the existing focused Rust test suites after the change.
You are interacting with an AI system.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Symbol map =='
ast-grep outline chutoro-core/src/memory.rs --items all --type function,struct,impl \
--match 'estimate_peak_bytes|HnswParams' || true
ast-grep outline chutoro-core/src/hnsw --items all --type function,struct,impl \
--match 'HnswParams|DistanceCache|CpuHnsw|with_capacity|build_with_edges' || true
ast-grep outline chutoro-core/tests/chutoro.rs --items all --type function \
--match 'memory|cache|hnsw' || true
printf '%s\n' '== Configuration flow and regression coverage =='
rg -n -C 5 \
'estimate_peak_bytes_for_hnsw_params|estimate_peak_bytes\s*\(|distance_cache_config|distance_cache_max_entries|with_distance_cache_max_entries|DistanceCache::|DistanceCacheConfig|run_cpu_pipeline_with_len|build_with_edges|MemoryLimitExceeded' \
chutoro-core/src chutoro-core/tests --glob '*.rs' || trueLength of output: 71655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Parameter-aware estimator and cache arithmetic =='
sed -n '84,190p' chutoro-core/src/memory.rs
printf '%s\n' '== Memory-estimator tests =='
sed -n '250,335p' chutoro-core/src/memory.rs
printf '%s\n' '== One-shot memory-limit tests =='
sed -n '145,190p' chutoro-core/tests/chutoro.rs
printf '%s\n' '== Exact cache-capacity references in changed test areas =='
rg -n -C 8 \
'with_distance_cache_max_entries|DEFAULT_MAX_ENTRIES|cache.*estimate|estimate.*cache' \
chutoro-core/src/memory.rs chutoro-core/tests/chutoro.rs || trueLength of output: 11903
@leynos, keep this review thread open.
The latest commit does not resolve the finding.
Chutoro::check_memory_limit now calls estimate_peak_bytes_for_hnsw_params. The CPU pipeline also receives the configured HnswParams. CpuHnsw constructs DistanceCache with params.distance_cache_config().
However, estimate_peak_bytes_for_hnsw_params passes only max_connections and effective_ef_construction to its helper. The helper still calculates distance_cache from DEFAULT_CACHE_MAX_ENTRIES in chutoro-core/src/memory.rs. A custom HnswParams::with_distance_cache_max_entries value therefore changes the allocation but not the memory-limit estimate.
No regression test covers a non-default distance-cache capacity.
Use this AI agent prompt:
Fix the CPU memory-limit estimate so that it accounts for the configured HNSW distance-cache capacity.
Trace and preserve the current configuration flow:
- `Chutoro::check_memory_limit` calls `estimate_peak_bytes_for_hnsw_params`.
- `Chutoro::run_cpu` passes the same `HnswParams` to `run_cpu_pipeline_with_len`.
- `run_cpu_pipeline_with_len` passes the parameters to `CpuHnsw::build_with_edges`.
- `CpuHnsw` creates `DistanceCache` from `params.distance_cache_config()`.
Update `chutoro-core/src/memory.rs`:
1. Extend the internal estimator to accept a distance-cache entry capacity.
2. Make `estimate_peak_bytes_for_hnsw_params` pass `hnsw_params.distance_cache_config().max_entries().get()`.
3. Calculate the cache contribution from that capacity with saturating arithmetic.
4. Keep the public `estimate_peak_bytes(point_count, max_connections)` API unchanged. It must continue to use `DEFAULT_CACHE_MAX_ENTRIES`.
5. Update comments and documentation that state the cache always uses `DEFAULT_CACHE_MAX_ENTRIES`.
Add regression tests:
1. In `chutoro-core/src/memory.rs`, construct `HnswParams` with `with_distance_cache_max_entries` above `DistanceCacheConfig::DEFAULT_MAX_ENTRIES`. Assert that `estimate_peak_bytes_for_hnsw_params` returns a larger value than for equivalent default-cache parameters.
2. In `chutoro-core/tests/chutoro.rs`, configure the same non-default cache capacity. Set `max_bytes` below the parameter-aware estimate but above the default-cache estimate. Assert that `Chutoro::run` returns `ChutoroError::MemoryLimitExceeded` and reports the parameter-aware estimate.
3. Use `NonZeroUsize` for the custom cache capacity.
Run the focused memory and Chutoro integration tests with the `cpu` feature enabled.
You are interacting with an AI system.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
Post @coderabbitai resolve or @coderabbitai approve as a new top-level PR comment. Approve commands are disabled for review-thread replies.
Document the supported batch tuning and migration path, cover the removed pipeline export at compile time, and verify configured CPU HNSW parameters reach a successful one-shot run. Bound construction search state to the batch's useful width and include it in the memory guard. Record bounded resource decision fields in tracing.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Record bounded one-shot outcomes and CPU resource observations while keeping source-identifying data out of metrics and decision-point tracing. Prove shared execution configuration propagation across batch and session paths, and document the metric contract for operators.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Make the CPU memory guard reflect the distance-cache capacity supplied to the HNSW builder. Preserve the legacy public estimate's default-capacity behaviour, and cover both estimates and the one-shot guard with regressions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
echo '=== Changed files ==='
git diff --name-status "$base" HEAD
echo '=== Relevant diff ==='
git diff --unified=16 "$base" HEAD -- \
chutoro-core/src \
chutoro-core/tests \
docs/users-guide.md \
docs/chutoro-design.md \
docs/developers-guide.md
echo '=== Relevant symbols and instrumentation ==='
rg -n -C 5 --glob '*.rs' \
'ExecutionConfig|with_hnsw_params|run_cpu_pipeline|run_cpu_pipeline_with_len|bounded_for_point_count|effective_ef_construction|estimate_peak_bytes_for_hnsw_params|check_memory_limit|proptest!|trybuild|compile_fail|metrics::|counter!|histogram!|tracing::|error_code|MemoryLimitExceeded' \
chutoro-core/src chutoro-core/tests || true
echo '=== Documentation references ==='
rg -n -C 5 \
'with_hnsw_params|run_cpu_pipeline|ExecutionConfig|effective.*construction|memory|metric|tracing|error.code' \
docs/users-guide.md docs/chutoro-design.md docs/developers-guide.md 2>/dev/null || true
echo '=== Test and UI fixture inventory ==='
fd -HI -t f . chutoro-core/tests | sortLength of output: 1648 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Repository state ==='
git status --short --branch || true
git show -s --format='HEAD %H%n%s' HEAD || true
echo '=== Relevant file inventory ==='
fd -HI -t f . chutoro-core/src chutoro-core/tests docs \
| rg '(builder|chutoro|cpu_pipeline|execution_config|memory|params|session.*config|users-guide|chutoro-design|developers-guide|trybuild|api_surface)' \
| sort
echo '=== Source and test outlines ==='
for file in \
chutoro-core/src/builder.rs \
chutoro-core/src/chutoro.rs \
chutoro-core/src/cpu_pipeline.rs \
chutoro-core/src/execution_config.rs \
chutoro-core/src/memory.rs \
chutoro-core/src/hnsw/params.rs \
chutoro-core/src/session/config.rs \
chutoro-core/tests/chutoro.rs \
chutoro-core/tests/session_api_surface.rs
do
if [ -f "$file" ]; then
echo "--- $file"
ast-grep outline "$file" --items all 2>/dev/null || true
fi
done
echo '=== Targeted implementation, test, and telemetry references ==='
rg -n -C 8 --glob '*.rs' \
'ExecutionConfig|with_hnsw_params|run_cpu_pipeline|run_cpu_pipeline_with_len|bounded_for_point_count|effective_ef_construction|estimate_peak_bytes_for_hnsw_params|check_memory_limit|proptest!|prop_assert|trybuild|compile_fail|metrics::|counter!|histogram!|tracing::|error_code|MemoryLimitExceeded' \
chutoro-core/src chutoro-core/tests || true
echo '=== Targeted documentation references ==='
rg -n -C 8 \
'with_hnsw_params|run_cpu_pipeline|ExecutionConfig|effective.*construction|memory|metric|tracing|error.code' \
docs/users-guide.md docs/chutoro-design.md docs/developers-guide.md 2>/dev/null || true
echo '=== UI fixtures ==='
fd -HI -t f . chutoro-core/tests | rg '(trybuild|\.stderr$|api_surface)' | sortLength of output: 202191 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_matches() {
local file="$1" pattern="$2"
if [ -f "$file" ]; then
echo "--- $file"
rg -n -C 18 --pcre2 "$pattern" "$file" || true
fi
}
show_matches chutoro-core/src/chutoro.rs \
'fn run\b|fn run_cpu\b|fn check_memory_limit\b|fn estimate_peak_bytes\b|metrics::|tracing::|error_code|batch\.'
show_matches chutoro-core/src/cpu_pipeline.rs \
'fn run_cpu_pipeline_with_len\b|building CPU HNSW index|effective_ef_construction|bounded_for_point_count'
show_matches chutoro-core/src/memory.rs \
'estimate_peak_bytes_for_hnsw_params|effective_ef_construction|SEARCH_STATE_BYTES_PER_WIDTH'
show_matches chutoro-core/src/hnsw/params.rs \
'bounded_for_point_count|effective_ef_construction'
show_matches chutoro-core/tests/chutoro.rs \
'hnsw|Hnsw|memory|metrics|tracing|proptest|run\('
show_matches chutoro-core/tests/session_api_surface.rs \
'trybuild|compile_fail'
show_matches chutoro-core/tests/trybuild/run_cpu_pipeline_is_private.rs \
'.'
show_matches chutoro-core/tests/trybuild/run_cpu_pipeline_is_private.stderr \
'.'
echo '=== Batch test candidates ==='
rg -n -l --glob '*.rs' \
'chutoro\.batch|batch.*metrics|MemoryLimitExceeded|building CPU HNSW index|with_hnsw_params' \
chutoro-core/tests chutoro-core/src | sort
echo '=== Metrics feature and test configuration ==='
rg -n -C 5 \
'metrics.*=|metrics-util|tracing-subscriber|trybuild|proptest' \
chutoro-core/Cargo.toml Cargo.toml 2>/dev/null || trueLength of output: 37506 🏁 Script executed: #!/bin/bash
set -euo pipefail
for file in \
chutoro-core/src/chutoro/properties.rs \
chutoro-core/src/chutoro_tests.rs \
chutoro-core/src/batch_metrics.rs \
chutoro-core/tests/batch_metrics.rs \
chutoro-core/src/lib.rs \
chutoro-core/src/error.rs
do
if [ -f "$file" ]; then
echo "--- $file"
cat -n "$file"
fi
done
echo '=== Test module registration ==='
rg -n -C 6 --glob '*.rs' \
'mod properties|mod batch_metrics|batch_metrics|chutoro_tests|cfg\(test\)' \
chutoro-core/src chutoro-core/tests || trueLength of output: 97052
Do not create a follow-up issue. No remaining work is required for these checks. Runtime verification remains required. This inspection sandbox could not compile or run the repository tests. Run the PR validation commands in CI before merge. You are interacting with an AI system. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch makes builder-provided HNSW tuning govern both supported execution
paths. One-shot CPU runs now consume the same validated configuration as
sessions, use it in the memory guard, and retain the full run-precondition
policy behind
Chutoro::run.The parameter-aware memory guard now includes the configured HNSW distance-cache
capacity, while the legacy public estimate retains its default-capacity
behaviour. It also records bounded batch outcomes and CPU resource observations
when the
metricsfeature is enabled. Metrics and decision-point tracingexclude source names, paths, and payload data.
Closes #134.
Review walkthrough
Validation
Chutorointegration regressions: passed.make check-fmt,make typecheck,make lint,make markdownlint,make nixie, andmake test: passed (1,095 tests; 1 skipped).coderabbit review --agent --committed: passed with 0 findings.References
Summary by Sourcery
Share validated execution configuration across batch and session paths while aligning CPU resource usage, memory guards, observability, and run preconditions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: