chore: add query index compatibility checks - #101
Merged
Conversation
|
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:
📝 WalkthroughWalkthrough검색 인덱스 DataFrame 스키마 정규화 유틸리티 추가 및 query_index의 정규화 분리, 여러 회귀 테스트 스크립트 추가, ChangesDebug 디렉토리 무시 규칙 단순화
검색 인덱스 스키마 정규화 + query_index 리팩터링
회귀 테스트 — 챗봇 러너
DB 연결 및 설정 변경 (SSH 터널링 포함)
이미지 서비스 응답 로깅 개선
도커 빌드 컨텍스트 변경
Sequence Diagram(s)sequenceDiagram
participant Tester as 회귀 스크립트
participant FS as 파일시스템(Parquet/파일)
participant Normalizer as normalize_search_df_schema
participant QueryIndex as LLM.sub_model.query_index
participant Embed as sentence_transformers (stub)
participant BM25 as rank_bm25 (stub)
Tester->>FS: legacy DataFrame 생성 및 parquet 쓰기
Tester->>FS: 임시 인덱스 파일들 배치(embeddings, corpus, keywords)
Tester->>Normalizer: parquet에서 읽은 search_df 전달
Normalizer->>Normalizer: 필수 컬럼 보장·타입/텍스트 정규화 반환
Tester->>QueryIndex: 정규화된 DataFrame 로드 후 hybrid_search 호출
QueryIndex->>Embed: 임베딩 생성 요청
QueryIndex->>BM25: BM25 키워드 검색 요청
Embed-->>QueryIndex: 임베딩 결과
BM25-->>QueryIndex: 키워드 스코어 결과
QueryIndex-->>Tester: 검색/정렬된 결과 반환
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/regression/check_query_index_metadata_quality.py (2)
20-55: ⚖️ Poor tradeoff중복 코드:
check_query_index_compat.py와 공통 유틸리티 추출 고려.
install_fake_sentence_transformer,install_fake_rank_bm25,write_text함수들이 두 테스트 파일에서 동일하게 구현되어 있습니다.tests/regression/conftest.py또는 공통 모듈로 추출하면 유지보수성이 향상됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/check_query_index_metadata_quality.py` around lines 20 - 55, The duplicated test helpers install_fake_sentence_transformer, install_fake_rank_bm25, and write_text should be moved into a shared test utility (e.g., tests/regression/conftest.py or tests/regression/utils.py) and the two test files should import or reference them as fixtures/helpers instead of redefining; extract the functions into the shared module, convert any setup functions to pytest fixtures if appropriate (e.g., install_fake_sentence_transformer -> a module-scoped fixture), update the test files to import those symbols (install_fake_sentence_transformer, install_fake_rank_bm25, write_text) and remove the duplicate implementations from check_query_index_metadata_quality.py and check_query_index_compat.py.
375-377: 💤 Low value기본 출력 경로를
tempfile기반으로 개선 권장.
check_query_index_compat.py와 동일하게/tmp대신tempfile.gettempdir()를 사용하는 것이 좋습니다.♻️ 제안 수정
- out_path = Path(args.out) if args.out else Path("/tmp/query_index_metadata_quality_report.json") + out_path = Path(args.out) if args.out else Path(tempfile.gettempdir()) / "query_index_metadata_quality_report.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/check_query_index_metadata_quality.py` around lines 375 - 377, Change the default output path creation in the block using out_path and args.out to use the system temp directory via tempfile.gettempdir() instead of hardcoding "/tmp"; i.e., when args.out is falsy construct out_path from Path(tempfile.gettempdir()) (matching check_query_index_compat.py), then continue to call out_path.parent.mkdir(parents=True, exist_ok=True) and out_path.write_text(json.dumps(report, ensure_ascii=False, indent=2), encoding="utf-8").tests/regression/check_query_index_compat.py (1)
257-259: 💤 Low value기본 출력 경로를
tempfile기반으로 개선 권장.하드코딩된
/tmp경로 대신tempfile.gettempdir()를 사용하면 크로스 플랫폼 호환성이 향상됩니다.♻️ 제안 수정
- out_path = Path(args.out) if args.out else Path("/tmp/query_index_compat_report.json") + out_path = Path(args.out) if args.out else Path(tempfile.gettempdir()) / "query_index_compat_report.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/check_query_index_compat.py` around lines 257 - 259, Replace the hardcoded default "/tmp" with the platform temp dir: change the out_path assignment to use Path(args.out) if args.out else Path(tempfile.gettempdir()) / "query_index_compat_report.json", and ensure tempfile is imported at the top of the module; keep the existing mkdir and write_text logic unchanged (refer to the out_path variable in this block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/regression/run_chatbot_regression.py`:
- Around line 78-81: The loop using any_regex currently calls re.search(pat,
text) directly which lets a bad regex raise re.error and abort the whole run;
wrap each pattern check in a try/except re.error so a single invalid pattern
only fails that test case (append an explanatory message to result["reasons"]
like "invalid_regex:{pat}:{err}") or treat the bad pattern as non-matching, and
keep the surrounding logic that sets result["passed"]=False and appends
"missing_any_regex:{any_regex}" when none match; update the generator/any()
usage that iterates over any_regex to perform the try/except per pat and
optionally log the invalid pattern and exception.
- Around line 58-61: The current check uses `if engines and engine not in
engines:` which treats a string `engine_in` as an iterable (causing substring
matches); change it to explicitly handle types: retrieve `engines =
case.get("engine_in")`, then if `isinstance(engines, str)` compare with equality
(`if engine != engines:`) and mark failure, else if `isinstance(engines, (list,
tuple, set))` use membership (`if engine not in engines:`); ensure the same
`result["reasons"].append(...)` behavior and handle None/empty cases
consistently.
- Around line 26-36: When reading a successful HTTP response (the block using
request.urlopen and resp), JSON parsing errors currently fall through to the
broad "except Exception as e" and return status_code 0; instead catch JSON
decode errors there and return the actual resp.status with the raw body as an
error. Concretely, around the json.loads(raw) call (in the resp branch
referencing resp and raw) add a try/except for json.JSONDecodeError (or
ValueError) and return resp.status, {"error": raw} on parse failure so you don't
lose the HTTP status that the broad "except Exception as e" currently masks.
- Around line 24-26: Validate the URL scheme before calling request.urlopen:
check the `url` used to build `req` and only allow "http" and "https" (or your
approved set); if the scheme is not in the whitelist, raise an exception or
abort the request instead of calling `request.urlopen(req, ...)`. Implement this
validation where `req = request.Request(url, ...)` is created and ensure any
downstream callers (the block using `with request.urlopen(req, timeout=timeout)
as resp:`) only run for validated URLs.
---
Nitpick comments:
In `@tests/regression/check_query_index_compat.py`:
- Around line 257-259: Replace the hardcoded default "/tmp" with the platform
temp dir: change the out_path assignment to use Path(args.out) if args.out else
Path(tempfile.gettempdir()) / "query_index_compat_report.json", and ensure
tempfile is imported at the top of the module; keep the existing mkdir and
write_text logic unchanged (refer to the out_path variable in this block).
In `@tests/regression/check_query_index_metadata_quality.py`:
- Around line 20-55: The duplicated test helpers
install_fake_sentence_transformer, install_fake_rank_bm25, and write_text should
be moved into a shared test utility (e.g., tests/regression/conftest.py or
tests/regression/utils.py) and the two test files should import or reference
them as fixtures/helpers instead of redefining; extract the functions into the
shared module, convert any setup functions to pytest fixtures if appropriate
(e.g., install_fake_sentence_transformer -> a module-scoped fixture), update the
test files to import those symbols (install_fake_sentence_transformer,
install_fake_rank_bm25, write_text) and remove the duplicate implementations
from check_query_index_metadata_quality.py and check_query_index_compat.py.
- Around line 375-377: Change the default output path creation in the block
using out_path and args.out to use the system temp directory via
tempfile.gettempdir() instead of hardcoding "/tmp"; i.e., when args.out is falsy
construct out_path from Path(tempfile.gettempdir()) (matching
check_query_index_compat.py), then continue to call
out_path.parent.mkdir(parents=True, exist_ok=True) and
out_path.write_text(json.dumps(report, ensure_ascii=False, indent=2),
encoding="utf-8").
🪄 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: CHILL
Plan: Pro
Run ID: f10fe037-25e5-47bb-bf6c-c17a9ada53d0
📒 Files selected for processing (7)
.gitignoreLLM/sub_model/query_index.pyLLM/sub_model/query_index_schema.pytests/regression/chatbot_regression_cases.jsontests/regression/check_query_index_compat.pytests/regression/check_query_index_metadata_quality.pytests/regression/run_chatbot_regression.py
Comment on lines
+58
to
+61
| engines = case.get("engine_in") | ||
| if engines and engine not in engines: | ||
| result["passed"] = False | ||
| result["reasons"].append(f"engine_not_in:{engines}") |
There was a problem hiding this comment.
engine_in 타입 미검증으로 오판정될 수 있습니다.
Line 59는 engine_in이 문자열일 때 부분 문자열 비교로 동작합니다("a" in "fast"). 케이스 파일 오타가 있으면 결과가 왜곡됩니다.
🔧 제안 수정안
engines = case.get("engine_in")
- if engines and engine not in engines:
- result["passed"] = False
- result["reasons"].append(f"engine_not_in:{engines}")
+ if engines is not None and not isinstance(engines, (list, tuple, set)):
+ result["passed"] = False
+ result["reasons"].append("invalid_engine_in_type")
+ elif engines and engine not in engines:
+ result["passed"] = False
+ result["reasons"].append(f"engine_not_in:{engines}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| engines = case.get("engine_in") | |
| if engines and engine not in engines: | |
| result["passed"] = False | |
| result["reasons"].append(f"engine_not_in:{engines}") | |
| engines = case.get("engine_in") | |
| if engines is not None and not isinstance(engines, (list, tuple, set)): | |
| result["passed"] = False | |
| result["reasons"].append("invalid_engine_in_type") | |
| elif engines and engine not in engines: | |
| result["passed"] = False | |
| result["reasons"].append(f"engine_not_in:{engines}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/regression/run_chatbot_regression.py` around lines 58 - 61, The current
check uses `if engines and engine not in engines:` which treats a string
`engine_in` as an iterable (causing substring matches); change it to explicitly
handle types: retrieve `engines = case.get("engine_in")`, then if
`isinstance(engines, str)` compare with equality (`if engine != engines:`) and
mark failure, else if `isinstance(engines, (list, tuple, set))` use membership
(`if engine not in engines:`); ensure the same `result["reasons"].append(...)`
behavior and handle None/empty cases consistently.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/regression/run_chatbot_regression.py`:
- Around line 21-22: The scheme whitelist currently only allows "https" while
the default --url uses "http", causing every run without args to return
unsupported_url_scheme; update the check that uses the variable scheme (e.g.,
the line if scheme not in {"https"}:) to include "http" (change to {"http",
"https"}) or alternatively change the default URL to use https so the scheme and
default align; modify the condition that returns (0, {"error":
f"unsupported_url_scheme:{scheme or '<empty>'}"}) accordingly.
- Around line 35-42: The second except block redundantly re-catches
error.HTTPError and omits connection failures (error.URLError), so update the
handler to catch error.URLError instead of error.HTTPError: change the tuple
(error.HTTPError, TimeoutError, ValueError) to (error.URLError, TimeoutError,
ValueError) where the current except appears in run_chatbot_regression.py (the
blocks handling error.HTTPError / raw and the subsequent generic except); keep
the first HTTPError-specific handler as-is so HTTP bodies are parsed, and ensure
the module that provides error (used as error.HTTPError) already exposes
URLError so no extra imports are needed.
- Line 8: The import statement incorrectly tries to import urlparse from urllib
causing ImportError; update the imports so that request and error remain from
urllib while urlparse is imported from urllib.parse (e.g., keep request and
error from urllib and import urlparse from urllib.parse) so references to
urlparse in the script resolve correctly.
🪄 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: CHILL
Plan: Pro
Run ID: 4e47f245-5181-4dd5-ad19-f294439ae672
📒 Files selected for processing (2)
.dockerignoretests/regression/run_chatbot_regression.py
💤 Files with no reviewable changes (1)
- .dockerignore
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/regression/run_chatbot_regression.py (1)
65-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
engine_in이 문자열일 때 부분 문자열 비교로 오판정됩니다.
case.get("engine_in")이 문자열이면engine not in engines는 동등 비교가 아닌 부분 문자열 검사("ast" in "fast"→True)로 동작합니다. 케이스 JSON에"engine_in": "fast"형태로 작성했을 때 테스트 결과가 왜곡됩니다.🔧 수정 제안
engines = case.get("engine_in") - if engines and engine not in engines: + if engines is not None and not isinstance(engines, (list, tuple, set)): + result["passed"] = False + result["reasons"].append("invalid_engine_in_type") + elif engines and engine not in engines: result["passed"] = False result["reasons"].append(f"engine_not_in:{engines}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/run_chatbot_regression.py` around lines 65 - 68, The current check uses "engines = case.get('engine_in')" and then "engine not in engines", which misbehaves when engine_in is a string (causing substring matches); change it to normalize engine_in to a sequence or treat strings specially: if engines is a str, compare equality (engine != engines) or convert engines to a single-item list (engines = [engines]) before performing membership testing; update the logic around the symbols engines and engine so the result["passed"] and result["reasons"].append behavior remains the same but avoids substring matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@LLM/OSS/service.py`:
- Around line 72-126: 현재 로직은 settings.ssh_host 또는 settings.db_host가 명시돼 있어도 SSH
실패나 DB_HOST 실패 시 자동으로 "localhost"로 폴백해 의도치 않게 로컬 DB에 연결되므로, 명시된 설정을 권위 있게 처리하도록
고치세요: if settings.ssh_host is set, do not swallow exceptions in the
SSHTunnelForwarder block (_ssh_tunnel / SSHTunnelForwarder) — on failure
raise/propagate a ConfigurationError (or re-raise the caught exc) instead of
falling back to direct connect; likewise, when building the direct connection
loop (currently for host in (settings.db_host, "localhost")), only include
"localhost" in that list if settings.db_host is falsy (i.e., treat
settings.db_host as authoritative and avoid trying localhost when a DB host was
explicitly provided). Ensure logger and connection_errors handling is adjusted
accordingly.
---
Duplicate comments:
In `@tests/regression/run_chatbot_regression.py`:
- Around line 65-68: The current check uses "engines = case.get('engine_in')"
and then "engine not in engines", which misbehaves when engine_in is a string
(causing substring matches); change it to normalize engine_in to a sequence or
treat strings specially: if engines is a str, compare equality (engine !=
engines) or convert engines to a single-item list (engines = [engines]) before
performing membership testing; update the logic around the symbols engines and
engine so the result["passed"] and result["reasons"].append behavior remains the
same but avoids substring matches.
🪄 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: CHILL
Plan: Pro
Run ID: 3e4d2dbf-0c16-4f4f-8b5b-b04fb210dfa1
📒 Files selected for processing (4)
LLM/OSS/service.pycore/settings.pyimage_analysis/service.pytests/regression/run_chatbot_regression.py
✅ Files skipped from review due to trivial changes (1)
- image_analysis/service.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@LLM/OSS/service.py`:
- Line 108: The default DB host string is misspelled which breaks local
connections: update the fallback value used when settings.db_host is falsy (the
hosts assignment in LLM/OSS/service.py — the hosts variable referencing
settings.db_host) to use the correct "localhost" string as the single-element
tuple fallback; ensure the tuple format remains a one-item tuple (e.g.,
(fallback_value,)) so downstream connection code receives the same type.
🪄 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: CHILL
Plan: Pro
Run ID: 51fdd1b7-dcd2-4362-a321-a37e946c5a16
📒 Files selected for processing (1)
LLM/OSS/service.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
LLM/OSS/service.py (1)
122-129: 💤 Low value
psycopg2.Error를 명시적으로 catch하는 것을 고려하세요.정적 분석 도구(Ruff BLE001)가 broad
Exceptioncatch를 경고하고 있습니다. 데이터베이스 연결 실패 시 발생할 수 있는 예외를 더 명확히 하면 예상치 못한 예외가 묻히는 것을 방지할 수 있습니다.♻️ 제안된 수정
+from psycopg2 import Error as Psycopg2Error + ... - except Exception as exc: + except Psycopg2Error as exc: connection_errors.append(exc) logger.warning( "chatbot_direct_db_connect_failed db_host=%s db_port=%s error=%s", host, settings.db_port, exc, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LLM/OSS/service.py` around lines 122 - 129, Replace the broad except Exception handler with a specific psycopg2 error handler: catch psycopg2.Error (e.g., via `psycopg2.Error` or `from psycopg2 import Error`) when appending to connection_errors and logging the failure in the block that currently uses connection_errors.append(exc) and logger.warning(... host, settings.db_port, exc). If you still want a safe fallback, add a separate narrow fallback except Exception that either re-raises unexpected exceptions or logs them with a different message, but ensure the primary handler explicitly catches psycopg2 errors so DB connection problems aren’t masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@LLM/OSS/service.py`:
- Around line 122-129: Replace the broad except Exception handler with a
specific psycopg2 error handler: catch psycopg2.Error (e.g., via
`psycopg2.Error` or `from psycopg2 import Error`) when appending to
connection_errors and logging the failure in the block that currently uses
connection_errors.append(exc) and logger.warning(... host, settings.db_port,
exc). If you still want a safe fallback, add a separate narrow fallback except
Exception that either re-raises unexpected exceptions or logs them with a
different message, but ensure the primary handler explicitly catches psycopg2
errors so DB connection problems aren’t masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 855b8d4d-881d-496c-bd0d-d877d3a2edce
📒 Files selected for processing (1)
LLM/OSS/service.py
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.
🎯 배경
🔍 주요 내용
query_index.py의 search_df 스키마 정규화 로직을query_index_schema.py로 분리tests/regression/구조로 정리변경 요약(1~3줄)
검색 인덱스의 스키마 정규화 로직을 별도 모듈로 분리하고, 레거시 아티팩트 호환성 및 메타데이터 희소성에 대한 회귀 검사 스크립트들을 tests/regression에 추가했습니다. DB 연결(SSH 터널) 설정과 일부 로깅/초기화 로직도 개선되었습니다.
주요 변경점
주의/리스크
다음 액션