feat: add cosine similarity crowding alerts - #1497
Conversation
stranske
added
agent:codex
autofix
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
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:
📝 WalkthroughWalkthroughThe change adds cosine similarity storage and ranking, extends the similar-managers API, and adds similarity-based crowded-trade alert metadata and condition evaluation. ChangesSimilarity and crowding alert flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SimilarManagersAPI
participant manager_similarity
Client->>SimilarManagersAPI: Request similar managers with basis
SimilarManagersAPI->>manager_similarity: Query selected similarity score
manager_similarity-->>SimilarManagersAPI: Return ranked similarity rows
SimilarManagersAPI-->>Client: Return basis, score, metrics, and counts
sequenceDiagram
participant conviction_flow
participant manager_similarity
participant alert_engine
conviction_flow->>manager_similarity: Find managers above similarity threshold
manager_similarity-->>conviction_flow: Return connected manager IDs
conviction_flow->>alert_engine: Submit crowded-trade payload
alert_engine-->>conviction_flow: Evaluate similar_manager_count_gte
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
93c953f to
466a893
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_alert_engine.py (1)
134-150: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a below-threshold assertion.
Keep the equality case, then evaluate the same rule with
similar_manager_count: 2and assert that it does not fire. This verifies the lower boundary forsimilar_manager_count_gte.As per path instructions, flag new behavior without sufficient test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_alert_engine.py` around lines 134 - 150, Add a second evaluation alongside the existing crowded-trade equality case, reusing the same rule and event context but setting payload similar_manager_count to 2; assert that this below-threshold event does not fire, while preserving the current equality assertion for value 3.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@alerts/engine.py`:
- Around line 86-90: Update the similar_manager_count_gte handling in the alert
evaluation flow to convert expected to a finite float instead of int(expected),
then compare the integer count against that fractional threshold so a count of 3
does not satisfy 3.9. Preserve the existing false result for missing or
below-threshold counts and the continue behavior for valid matches.
In `@api/managers.py`:
- Around line 1355-1357: Add an HTTP-level test for the endpoint exposing the
basis query parameter, alongside the existing valid jaccard and cosine cases.
Send a request with an invalid basis value and assert that the response status
is 422, preserving coverage of the new validation contract.
- Around line 1373-1401: The SQL query in the manager similarity fetch does not
filter out non-finite values (NaN, Infinity) that PostgreSQL can store, causing
these values to rank above finite results and eventually fail during JSON
serialization. Update the WHERE clause condition in the query to exclude
non-finite values for both the score_column (cosine or jaccard) and the cosine
column using PostgreSQL's IS FINITE function, ensuring only valid numeric values
are returned before the serialization step in the return statement.
In `@etl/manager_similarity_flow.py`:
- Around line 14-23: Update cosine_similarity to reject non-finite values in
both input vectors before calculating the denominator, and validate the
denominator and computed score with math.isfinite before returning. Preserve the
existing None behavior for mismatched, empty, zero-magnitude, or non-finite
inputs and results.
In `@tests/test_conviction_flow.py`:
- Around line 59-60: Add a test in the existing _similar_manager_ids coverage
using a pair whose Jaccard similarity is exactly the supplied minimum_score, and
assert that the pair is included. Preserve the current below-threshold assertion
while explicitly verifying the inclusive jaccard >= minimum_score boundary.
In `@tests/test_crowded_contrarian.py`:
- Around line 320-323: Extend the post-dispatch assertions in the crowded-trade
test to validate the event payload’s similar_manager_ids, similar_manager_count,
and similarity_floor fields against the seeded similarity data. Use the existing
dispatched crowded-trade event assertion path and preserve the current
assertions.
---
Outside diff comments:
In `@tests/test_alert_engine.py`:
- Around line 134-150: Add a second evaluation alongside the existing
crowded-trade equality case, reusing the same rule and event context but setting
payload similar_manager_count to 2; assert that this below-threshold event does
not fire, while preserving the current equality assertion for value 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5b44145-a223-493e-86b1-574c7f49f745
📒 Files selected for processing (12)
.env.examplealembic/versions/016_manager_similarity_cosine.pyalerts/engine.pyalerts/models.pyapi/managers.pyetl/conviction_flow.pyetl/manager_similarity_flow.pyschema.sqltests/test_alert_engine.pytests/test_conviction_flow.pytests/test_crowded_contrarian.pytests/test_manager_similarity_flow.py
The activism campaign migration merged to main as revision 016, so this branch's cosine migration collided on the same identifier and Alembic reported two heads named 016.
|
Closer lane: fixed the Gate failure introduced by the rebase onto merged Failure: Fix ( Validation
|
|
Runner dispatch state for codex on PR #1497. Do not edit. |
🤖 Keepalive Loop StatusPR #1497 | Agent: Codex | Iteration 0/12 Current State
Last Codex Run
To retry immediately:
Or wait for the next successful Gate run to automatically retry. 🔍 Failure Classification| Error type | infrastructure |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@langsmith-fleet-worker-attempt.json`:
- Line 4: Remove the langsmith-fleet-worker-attempt.json file from the
repository, as it declares the langsmith-fleet/v1 schema but lacks the required
contract fields (recorded_at, schema_version, run_id, and status). If any tests
depend on this artifact with its current shape, relocate it to a test fixtures
directory instead of keeping it at the repository root.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80c4ea28-f773-4b57-88d3-a122bb9f30f8
📒 Files selected for processing (1)
langsmith-fleet-worker-attempt.json
|
Addressed the seven CodeRabbit findings in f6a0b97. Similar-manager thresholds now preserve fractional values; cosine generation and API responses reject non-finite scores; the endpoint, inclusive-boundary, and alert-payload contracts are covered; and the malformed generated worker-attempt artifact was removed. The invalid-basis regression asserts this APIs established validation mapping (400). Validation: 37 focused tests passed; Ruff, Black, and git diff --check passed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
etl/manager_similarity_flow.py (1)
89-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a flow-level test for persisted cosine scores.
The current cosine test calls
cosine_similaritydirectly. The API test seeds cosine values manually. Neither test verifies thatcompute_manager_similaritystores the computed cosine value inmanager_similarity.Add a deterministic flow test that runs the rebuild and asserts the stored cosine score for a known manager pair.
As per path instructions, flag “new or changed behavior with no accompanying test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etl/manager_similarity_flow.py` around lines 89 - 99, Update the similarity flow tests around compute_manager_similarity to add a deterministic rebuild test using known manager vectors, then query manager_similarity for the expected manager pair and assert the persisted cosine score matches the computed value. Keep existing direct cosine and API tests intact while covering the end-to-end database persistence path.Source: Path instructions
api/managers.py (1)
1375-1410: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftDo not load every peer row before applying
limit.This query loads all similarity rows for the manager, then filters and limits them in Python. A manager with many peers can cause large database reads and allocations for a request that returns at most 100 items.
Filter finite scores in dialect-specific SQL, or retrieve bounded pages until the endpoint collects
limitfinite rows. Add coverage where invalid rows precede valid rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/managers.py` around lines 1375 - 1410, Update the peer-similarity retrieval around the query and finite_rows construction so the database does not load every matching row before enforcing limit. Apply finite-score filtering in dialect-specific SQL, or fetch bounded pages until limit valid rows are collected, while preserving score and overlap ordering; add coverage for invalid rows preceding valid results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/managers.py`:
- Around line 1375-1410: Update the peer-similarity retrieval around the query
and finite_rows construction so the database does not load every matching row
before enforcing limit. Apply finite-score filtering in dialect-specific SQL, or
fetch bounded pages until limit valid rows are collected, while preserving score
and overlap ordering; add coverage for invalid rows preceding valid results.
In `@etl/manager_similarity_flow.py`:
- Around line 89-99: Update the similarity flow tests around
compute_manager_similarity to add a deterministic rebuild test using known
manager vectors, then query manager_similarity for the expected manager pair and
assert the persisted cosine score matches the computed value. Keep existing
direct cosine and API tests intact while covering the end-to-end database
persistence path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56707971-4725-42c2-aa48-8262bbff9b02
📒 Files selected for processing (7)
alerts/engine.pyapi/managers.pyetl/manager_similarity_flow.pytests/test_alert_engine.pytests/test_conviction_flow.pytests/test_crowded_contrarian.pytests/test_manager_similarity_flow.py
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
|
Workflow state fingerprint for Agents Verifier. Do not edit. |
The runner writes langsmith-fleet-worker-attempt.json to the repo root, so keepalive committed it into this PR (the same artifact was removed from #1497). .gitignore only covered artifacts/langsmith/.
* feat: add short-interest conviction context * fix: document short-interest endpoint override * chore(codex-keepalive): apply updates (PR #1498) * fix: address short-interest review findings Rejects rows without a real calendar report_date, preserves zero short-interest values, treats a blank FINRA_SHORT_INTEREST_URL as unset, gives metric_id a SQLite INTEGER variant, keeps DB write errors out of the fetch-failure path, removes DDL from the annotation read path, and prefers CUSIP over a reassigned ticker. * chore: keep generated langsmith fleet telemetry out of the PR The runner writes langsmith-fleet-worker-attempt.json to the repo root, so keepalive committed it into this PR (the same artifact was removed from #1497). .gitignore only covered artifacts/langsmith/. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: stranske <stranske@users.noreply.github.com>
…1500) Issue #1469 required the cosine basis to be selectable via config with Jaccard remaining the default; merged PR #1497 only added a per-request basis query parameter. MANAGER_SIMILARITY_DEFAULT_BASIS now supplies the default, an explicit query parameter still wins, and an unrecognized value warns and falls back to jaccard. Co-authored-by: stranske <stranske@users.noreply.github.com>
Closer verifier disposition — openai
|
1 participant
Closes #1469
Summary
basis=jaccard|cosinepeer rankingValidation
.venv/bin/python -m pytest -q tests/test_manager_similarity_flow.py tests/test_alert_engine.py tests/test_conviction_flow.py tests/test_schema.py.venv/bin/python -m ruff check ....venv/bin/python -m black --target-version py313 --fast --check ...git diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests