refactor: separate chatbot query index loading - #129
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughQuery Index 리소스 로딩 로직을 dedicated loader 모듈로 분리했습니다. 새로운 ChangesQuery Index Resource Loader 추출 및 리팩토링
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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: 1
🧹 Nitpick comments (4)
LLM/sub_model/query_index.py (1)
445-463: 🏗️ Heavy lift리소스 로딩 트리거도 import 시점에서 분리하는 편이 낫습니다.
지금 구조도
query_index.py를 import하는 순간 parquet/embedding/BM25/model이 전부 로드됩니다. 이번 PR 목적이 로딩 책임 분리라면,load_query_index_resources()호출 자체는lifecycle같은 startup 경로나initialize_query_index()로 옮겨서 import 비용과 초기화 타이밍을 분리하는 쪽이 더 일관됩니다.예시 방향
-_index_resources = load_query_index_resources() -DATA_JSON = _index_resources.paths.data_json -ART_DIR = _index_resources.paths.art_dir -SEARCH_DF_PATH = _index_resources.paths.search_df_path -EMB_PATH = _index_resources.paths.emb_path -BM25_PATH = _index_resources.paths.bm25_path -TOK_PATH = _index_resources.paths.tok_path -CONTACTS_CSV = _index_resources.paths.contacts_csv -META_PATH = _index_resources.paths.meta_path -CONTACT_KWS = _index_resources.contact_kws -embeddings = _index_resources.embeddings - -model_name = _index_resources.model_name -model = _index_resources.model -search_df = _index_resources.search_df -tokenize_kor = _index_resources.tokenizer -bm25 = _index_resources.bm25 +_index_resources = None + +def initialize_query_index(resources=None): + global _index_resources, DATA_JSON, ART_DIR, SEARCH_DF_PATH, EMB_PATH, BM25_PATH + global TOK_PATH, CONTACTS_CSV, META_PATH, CONTACT_KWS, embeddings + global model_name, model, search_df, tokenize_kor, bm25 + + _index_resources = resources or load_query_index_resources() + DATA_JSON = _index_resources.paths.data_json + ART_DIR = _index_resources.paths.art_dir + SEARCH_DF_PATH = _index_resources.paths.search_df_path + EMB_PATH = _index_resources.paths.emb_path + BM25_PATH = _index_resources.paths.bm25_path + TOK_PATH = _index_resources.paths.tok_path + CONTACTS_CSV = _index_resources.paths.contacts_csv + META_PATH = _index_resources.paths.meta_path + CONTACT_KWS = _index_resources.contact_kws + embeddings = _index_resources.embeddings + model_name = _index_resources.model_name + model = _index_resources.model + search_df = _index_resources.search_df + tokenize_kor = _index_resources.tokenizer + bm25 = _index_resources.bm25🤖 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 `@LLM/sub_model/query_index.py` around lines 445 - 463, Currently load_query_index_resources() is called at import time causing heavy startup costs; refactor by removing the top-level call and instead add an initialize_query_index() function that calls load_query_index_resources() and assigns its result to a module-level _index_resources (or returns it) and populates DATA_JSON, ART_DIR, SEARCH_DF_PATH, EMB_PATH, BM25_PATH, TOK_PATH, CONTACTS_CSV, META_PATH, CONTACT_KWS, embeddings, model_name, model, search_df, tokenize_kor, bm25, and UNIT_TOK_RE as needed; change the existing module-level assignments that reference _index_resources to either lazy getters or to None until initialize_query_index() runs, and update any consumers to call initialize_query_index() during application startup/lifecycle to ensure resources are loaded only at initialization time and that functions using these symbols verify initialization and raise a clear error if called before initialization.docs/PLANS.md (1)
92-93: ⚡ Quick win
검색 및 인덱싱핵심 파일 목록에도 loader를 같이 올려 주세요.현재 스냅샷과 설명 문단에는
query_index_loader.py가 반영됐는데, 바로 위 핵심 파일 목록에는 아직 빠져 있어서 문서 안에서 구조 설명이 살짝 엇갈립니다.문서 정리 예시
### 검색 및 인덱싱 - `LLM/sub_model/query_index.py` +- `LLM/sub_model/query_index_loader.py` - `LLM/sub_model/schedule_index.py` - `LLM/sub_model/index_utils.py` - `LLM/sub_model/build_index.py`Also applies to: 181-182
🤖 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 `@docs/PLANS.md` around lines 92 - 93, The core file list in the "검색 및 인덱싱" section is missing the loader entry; add "query_index_loader.py # 검색 인덱스 환경 경로 해석 및 아티팩트 로딩" to the 핵심 파일 목록 alongside "schedule_index.py" and likewise add the same loader entry where the same list appears again (the other occurrence mentioned around lines 181-182) so the snapshot/description and the core-file list stay consistent with the documented files.LLM/sub_model/query_index_loader.py (2)
116-116: ⚡ Quick winSentenceTransformer 모델 다운로드 동작을 문서화하세요.
SentenceTransformer(model_name)은 모델이 캐시되지 않은 경우 자동으로 다운로드합니다. 첫 실행 시 네트워크 오류나 긴 대기 시간이 발생할 수 있습니다.사용자 경험 향상을 위해:
- 모델 다운로드 동작을 문서화하거나
- 다운로드 진행 상황 로깅을 추가하거나
- 네트워크 오류에 대한 명시적 오류 처리를 추가하는 것을 권장합니다.
📝 로깅 추가 제안
+import logging + +logger = logging.getLogger(__name__) + def load_query_index_resources( paths: QueryIndexPaths | None = None, model_name: str = DEFAULT_MODEL_NAME, ) -> QueryIndexResources: paths = paths or load_query_index_paths() search_df = normalize_search_df_schema(pd.read_parquet(paths.search_df_path)) tokenizer = get_tokenizer() + logger.info(f"Loading SentenceTransformer model: {model_name}") + try: + model = SentenceTransformer(model_name) + except Exception as e: + logger.error(f"Failed to load model {model_name}: {e}") + raise + return QueryIndexResources( paths=paths, contact_kws=load_list_from_txt(paths.contact_kws_path), embeddings=load_embeddings(paths.emb_path), search_df=search_df, tokenizer=tokenizer, bm25=load_bm25(paths.bm25_path, paths.tok_path, search_df, tokenizer), model_name=model_name, - model=SentenceTransformer(model_name), + model=model, )🤖 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 `@LLM/sub_model/query_index_loader.py` at line 116, The call to SentenceTransformer(model_name) will auto-download models on first run; update the code around where model=SentenceTransformer(model_name) is created (the model initialization block) to (1) document this behavior in a comment or docstring, and (2) wrap the constructor in a try/except that logs download start/completion and any network errors (use your processLogger or similar) and either retry or surface a clear, actionable error; ensure logs include model_name and exception details so users know a long download or network failure occurred.
48-56: ⚡ Quick win경로 존재 여부 검증 추가를 고려하세요.
env_path함수가 환경 변수를 절대 경로로 변환하지만, 파일이나 디렉토리가 실제로 존재하는지 검증하지 않습니다. 모든 경로가 읽기 전용 아티팩트를 가리키므로, 존재하지 않는 경로는 나중에load_embeddings,pd.read_parquet등에서 불명확한 오류를 발생시킬 수 있습니다.경로 존재 여부를 검증하면 더 명확한 오류 메시지로 빠르게 실패할 수 있습니다.
♻️ 경로 존재 검증 추가 제안
def env_path(name: str) -> Path: val = os.getenv(name, "").strip() if not val: raise FileNotFoundError(f"{name} environment empty.") s = os.path.expandvars(val) p = Path(s) if not p.is_absolute(): p = ROOT_DIR / p - return p.resolve() + p = p.resolve() + if not p.exists(): + raise FileNotFoundError(f"{name}={p} does not exist.") + return p🤖 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 `@LLM/sub_model/query_index_loader.py` around lines 48 - 56, The env_path function should validate that the resolved path actually exists to fail early with a clear error; after computing p = Path(...).resolve() (in env_path) add a check using p.exists() and raise FileNotFoundError containing both the environment variable name and the resolved path when it doesn't exist (optionally also use p.is_file()/p.is_dir() if callers expect a specific type), so callers like load_embeddings or pd.read_parquet get an explicit, actionable error.
🤖 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 `@LLM/sub_model/query_index_loader.py`:
- Around line 86-88: The code currently uses pickle.load on BM25_PATH (inside
the path.exists() branch) which is unsafe for untrusted files; replace this by
either (a) removing pickle-based deserialization and always rebuilding the BM25
index from the tokenized corpus/search DataFrame (call the BM25 construction
routine you have where the index is built from the corpus/tokenizer), or (b) if
you must load a prebuilt artifact, add integrity verification (e.g., compare a
stored checksum/signature before loading) and switch to a safer serialization
strategy (store reconstructable source data like JSON/csv or a reproducible
tokenized corpus rather than raw pickle). Update the code that currently calls
pickle.load(path) and any code paths referencing BM25_PATH to implement the
rebuild-or-verify-and-load flow.
---
Nitpick comments:
In `@docs/PLANS.md`:
- Around line 92-93: The core file list in the "검색 및 인덱싱" section is missing the
loader entry; add "query_index_loader.py # 검색 인덱스 환경 경로 해석 및 아티팩트 로딩" to the
핵심 파일 목록 alongside "schedule_index.py" and likewise add the same loader entry
where the same list appears again (the other occurrence mentioned around lines
181-182) so the snapshot/description and the core-file list stay consistent with
the documented files.
In `@LLM/sub_model/query_index_loader.py`:
- Line 116: The call to SentenceTransformer(model_name) will auto-download
models on first run; update the code around where
model=SentenceTransformer(model_name) is created (the model initialization
block) to (1) document this behavior in a comment or docstring, and (2) wrap the
constructor in a try/except that logs download start/completion and any network
errors (use your processLogger or similar) and either retry or surface a clear,
actionable error; ensure logs include model_name and exception details so users
know a long download or network failure occurred.
- Around line 48-56: The env_path function should validate that the resolved
path actually exists to fail early with a clear error; after computing p =
Path(...).resolve() (in env_path) add a check using p.exists() and raise
FileNotFoundError containing both the environment variable name and the resolved
path when it doesn't exist (optionally also use p.is_file()/p.is_dir() if
callers expect a specific type), so callers like load_embeddings or
pd.read_parquet get an explicit, actionable error.
In `@LLM/sub_model/query_index.py`:
- Around line 445-463: Currently load_query_index_resources() is called at
import time causing heavy startup costs; refactor by removing the top-level call
and instead add an initialize_query_index() function that calls
load_query_index_resources() and assigns its result to a module-level
_index_resources (or returns it) and populates DATA_JSON, ART_DIR,
SEARCH_DF_PATH, EMB_PATH, BM25_PATH, TOK_PATH, CONTACTS_CSV, META_PATH,
CONTACT_KWS, embeddings, model_name, model, search_df, tokenize_kor, bm25, and
UNIT_TOK_RE as needed; change the existing module-level assignments that
reference _index_resources to either lazy getters or to None until
initialize_query_index() runs, and update any consumers to call
initialize_query_index() during application startup/lifecycle to ensure
resources are loaded only at initialization time and that functions using these
symbols verify initialization and raise a clear error if called before
initialization.
🪄 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: 28a75191-dcbf-427c-8a22-69cc9c7df88e
📒 Files selected for processing (3)
LLM/sub_model/query_index.pyLLM/sub_model/query_index_loader.pydocs/PLANS.md
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안에 함께 있어 검색 로직과 런타임 리소스 준비 책임이 섞여 있었습니다.🔍 주요 내용
LLM/sub_model/query_index_loader.py를 추가해 검색 인덱스 경로 해석, 키워드 로딩, 임베딩 정규화, BM25 로딩/fallback, 임베딩 모델 초기화를 담당하도록 분리했습니다.query_index.py는 기존 전역 변수와hybrid_search(),build_answer()인터페이스를 유지하면서 loader에서 준비된 리소스를 사용하도록 변경했습니다.docs/PLANS.md의 검색 및 인덱싱 설명을 갱신했습니다.변경 요약(1~3줄)
검색 인덱스 로딩/초기화 책임을 LLM/sub_model/query_index_loader.py로 분리해 모듈화와 유지보수성을 개선했습니다. hybrid_search()와 build_answer() 인터페이스는 그대로 두고, 로더가 경로 해석·임베딩·BM25·모델 초기화를 담당합니다.
주요 변경점
주의/리스크
다음 액션