refactor: chatbot service logic and routting code separate - #94
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 (5)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough이 PR은 LLM/OSS 챗 엔드포인트를 얇은 FastAPI 라우터로 분리하고 핵심 로직을 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as FastAPI Router<br/>(Open_AI_OSS.py)
participant Service as Chat Service<br/>(LLM/OSS/service.py)
participant ModeDecider as Mode Decider<br/>(LLM/OSS/modes.py)
participant Cache as Cache
participant Formatter as Formatter<br/>(LLM/OSS/formatter.py)
participant OSS as OSS Model
participant DB as Database
Client->>Router: POST /chatbot (ChatReq)
Router->>Service: chat_with_oss(req)
Service->>ModeDecider: decide_mode(user_text)
ModeDecider-->>Service: mode
Service->>Cache: lookup(key)
alt cache hit
Cache-->>Service: cached_response
else cache miss
Service->>Service: apply guard / route by mode
alt mode in ["schedule","policy","dorm","grad","topic"]
Service->>OSS: call_oss(prompt)
OSS-->>Service: raw_response
Service->>Formatter: format by mode
Formatter-->>Service: formatted_response
else mode == "oss"
Service->>OSS: call_oss(prompt)
OSS-->>Service: raw_response
Service->>Formatter: scrub_non_contact(raw_response)
Formatter-->>Service: scrubbed_response
end
Service->>Cache: store(key, response)
end
Service->>DB: log_interaction (background)
DB-->>Service: ack
Service-->>Router: response dict
Router-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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)
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: 3
🧹 Nitpick comments (1)
LLM/OSS/formatter.py (1)
116-143:load_dept_map()에서 파싱 실패가 조용히 삭제됩니다 — 부서 매칭 품질 저하를 감지하기 어렵습니다.라인 135-136 의
except Exception: continue때문에 쉼표 분리 실패, 인코딩 이슈, 예기치 않은 포맷 등 어떤 오류든 해당 줄이 소리 없이 드롭되고DEPT_MAP/DEPT_ALIAS에서 누락됩니다. 이 두 딕셔너리는 모듈 로드 시점에 1회만 채워지고(라인 140) 이후detect_dept_hint,dept_clarification_message,_parse_bullets_and_pick의 스코어링 근거가 되므로, 누락된 학과가 있어도 런타임에는 그냥 "힌트 없음" 처럼 동작해 원인 추적이 매우 어렵습니다. 최소한 오류 로깅(또는 드롭 건수 카운트) 을 추가해 주세요. 또한 모듈 import 시점의 파일 I/O 자체도 테스트 환경에서 파일 누락 시DEPT_MAP이 영구히 비게 되는 부작용이 있으니, 첫 호출 시 lazy 로드하는 방식도 고려해 볼 만합니다.♻️ 제안 패치
- try: - name, rest = line.split(",", 1) - url_part = rest.split(",")[0].strip() - base = url_part.split("?")[0] - canon = _canon_unit(name) - dept_map[canon] = {"name": name, "path_base": base} - aliases[canon] = _make_aliases(name) - except Exception: - continue + try: + name, rest = line.split(",", 1) + url_part = rest.split(",")[0].strip() + base = url_part.split("?")[0] + canon = _canon_unit(name) + dept_map[canon] = {"name": name, "path_base": base} + aliases[canon] = _make_aliases(name) + except Exception as exc: + print(f"[load_dept_map WARN] skip line {line!r}: {exc}") + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LLM/OSS/formatter.py` around lines 116 - 143, The load_dept_map function currently swallows all exceptions per-line (except Exception: continue) which silently drops entries used by DEPT_MAP/DEPT_ALIAS and harms downstream functions like detect_dept_hint, dept_clarification_message and _parse_bullets_and_pick; change load_dept_map to catch only expected parsing errors (e.g., ValueError) or at minimum log the exception and the offending line (include the raw line text and index) and increment a dropped_lines counter, then after reading the file emit a summary warning with the drop count; additionally make module-level initialization lazy (do not call load_dept_map unconditionally at import) by replacing the immediate DEPT_MAP/DEPT_ALIAS population with a lazy loader or getter that calls load_dept_map on first access so tests or missing files don’t leave the maps permanently empty.
🤖 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/modes.py`:
- Line 13: RELATION_RE currently matches bare Korean tokens like "관계" and "사이"
causing many false positives; update RELATION_RE so the suffixes are required
and add token-boundary checks or context anchors to avoid matching those words
in compound nouns. Specifically, modify RELATION_RE (the compiled pattern named
RELATION_RE) to require the (야|냐) suffix for "사이" and "관계" (remove the trailing
?), and strengthen boundaries using lookarounds (e.g., require preceding
whitespace/start and following whitespace/end/punctuation or use (?<!\S) ...
(?!\S)) or explicitly match conversational phrases like "우리 관계", "우린 무슨 관계", "너와
나", ensuring normal compound words like "인과관계" or "사이버보안학과" are not matched.
In `@LLM/OSS/service.py`:
- Around line 47-90: init_db_pool is not idempotent and can leak SSH tunnels and
DB connections; before creating a new SSHTunnelForwarder or
ThreadedConnectionPool, check for and clean up existing resources: if
_ssh_tunnel is not None call _ssh_tunnel.stop() and set _ssh_tunnel = None, and
if _db_pool is not None call _db_pool.closeall() and set _db_pool = None, then
proceed to create and assign the new instances (refer to init_db_pool,
_ssh_tunnel, _db_pool). Also update shutdown_db_pool to explicitly shutdown the
log executor by calling _log_executor.shutdown(wait=False) (and optionally set
_log_executor = None) after closing the DB pool and stopping the tunnel so the
module-level ThreadPoolExecutor doesn't keep the process alive (refer to
shutdown_db_pool and _log_executor).
In `@text_filtering/service.py`:
- Around line 45-50: get_device() currently prefers MPS over CUDA; change its
priority to CUDA > MPS > CPU to match the project's canonical ordering used in
LLM/finetune_chatbot.py, i.e., check torch.cuda.is_available() first, then
torch.backends.mps.is_available(), otherwise CPU. Also fix the logic in
text_filtering/bad_text_learning_model.py around the device selection (the
incorrect branch at or near line 129 that picks "mps" when CUDA is available) so
it follows the same CUDA > MPS > CPU checks and selects the correct torch.device
string.
---
Nitpick comments:
In `@LLM/OSS/formatter.py`:
- Around line 116-143: The load_dept_map function currently swallows all
exceptions per-line (except Exception: continue) which silently drops entries
used by DEPT_MAP/DEPT_ALIAS and harms downstream functions like
detect_dept_hint, dept_clarification_message and _parse_bullets_and_pick; change
load_dept_map to catch only expected parsing errors (e.g., ValueError) or at
minimum log the exception and the offending line (include the raw line text and
index) and increment a dropped_lines counter, then after reading the file emit a
summary warning with the drop count; additionally make module-level
initialization lazy (do not call load_dept_map unconditionally at import) by
replacing the immediate DEPT_MAP/DEPT_ALIAS population with a lazy loader or
getter that calls load_dept_map on first access so tests or missing files don’t
leave the maps permanently empty.
🪄 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: cf93db65-e51a-4af7-a786-274d84336ce6
📒 Files selected for processing (9)
.gitignoreLLM/OSS/Open_AI_OSS.pyLLM/OSS/formatter.pyLLM/OSS/modes.pyLLM/OSS/service.pyservices/__init__.pytext_filtering/service.pytext_filtering/text_filtering.pytext_filtering/text_filtering_rule.py
Comment on lines
+47
to
+90
| _ssh_tunnel: Optional[SSHTunnelForwarder] = None | ||
| _db_pool: Optional[pg_pool.ThreadedConnectionPool] = None | ||
| _log_executor = ThreadPoolExecutor(max_workers=4, thread_name_prefix="chatbot_log") | ||
|
|
||
| _client: Optional[OpenAI] = None | ||
| _client_lock = threading.Lock() | ||
|
|
||
|
|
||
| class ChatReq(BaseModel): | ||
| text: Optional[str] = None | ||
| messages: Optional[list[dict[str, str]]] = None | ||
| engine: Optional[str] = None | ||
|
|
||
|
|
||
| def init_db_pool() -> None: | ||
| global _ssh_tunnel, _db_pool | ||
| ssh_host = settings.ssh_host | ||
| db_kwargs = dict( | ||
| dbname=settings.db_name, | ||
| user=settings.db_user, | ||
| password=settings.db_password, | ||
| connect_timeout=3, | ||
| options="-c statement_timeout=5000", | ||
| ) | ||
| if ssh_host: | ||
| _ssh_tunnel = SSHTunnelForwarder( | ||
| (ssh_host, 22), | ||
| ssh_username=settings.ssh_user, | ||
| ssh_pkey=settings.ssh_key_path, | ||
| remote_bind_address=("localhost", 5433), | ||
| ) | ||
| _ssh_tunnel.start() | ||
| db_kwargs.update(host="localhost", port=_ssh_tunnel.local_bind_port) | ||
| else: | ||
| db_kwargs["host"] = "localhost" | ||
| _db_pool = pg_pool.ThreadedConnectionPool(minconn=1, maxconn=5, **db_kwargs) | ||
|
|
||
|
|
||
| def shutdown_db_pool() -> None: | ||
| if _db_pool: | ||
| _db_pool.closeall() | ||
| if _ssh_tunnel: | ||
| _ssh_tunnel.stop() | ||
|
|
There was a problem hiding this comment.
DB/터널 리소스 초기화·정리 경로 재점검 필요.
두 가지 누수 가능성이 있습니다.
init_db_pool()이 멱등하지 않습니다. 테스트에서 재호출되거나 lifespan 이 재실행(예: 리로드, 멀티워커에서의 재초기화 시나리오)되면, 기존_ssh_tunnel/_db_pool참조를 버리고 새로 할당하므로 이전 터널 스레드와 psycopg2 커넥션이 정리되지 않은 채 남습니다.shutdown_db_pool()이_log_executor를 종료하지 않습니다. 모듈 임포트 시점에 생성된ThreadPoolExecutor(max_workers=4)는 프로세스 종료 시까지 살아 있고,atexit기본 동작에 의해 인터프리터가 끝날 때 pending 작업을 블로킹으로 기다립니다. 셧다운 훅에서 명시적으로_log_executor.shutdown(wait=...)호출이 필요합니다.
🛡️ 제안 패치
def init_db_pool() -> None:
global _ssh_tunnel, _db_pool
+ if _db_pool is not None:
+ return
ssh_host = settings.ssh_host
...
def shutdown_db_pool() -> None:
- if _db_pool:
- _db_pool.closeall()
- if _ssh_tunnel:
- _ssh_tunnel.stop()
+ global _db_pool, _ssh_tunnel
+ try:
+ _log_executor.shutdown(wait=True, cancel_futures=False)
+ except Exception as exc:
+ print(f"[chatbot_log executor shutdown ERROR] {exc}")
+ if _db_pool:
+ _db_pool.closeall()
+ _db_pool = None
+ if _ssh_tunnel:
+ _ssh_tunnel.stop()
+ _ssh_tunnel = None📝 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
| _ssh_tunnel: Optional[SSHTunnelForwarder] = None | |
| _db_pool: Optional[pg_pool.ThreadedConnectionPool] = None | |
| _log_executor = ThreadPoolExecutor(max_workers=4, thread_name_prefix="chatbot_log") | |
| _client: Optional[OpenAI] = None | |
| _client_lock = threading.Lock() | |
| class ChatReq(BaseModel): | |
| text: Optional[str] = None | |
| messages: Optional[list[dict[str, str]]] = None | |
| engine: Optional[str] = None | |
| def init_db_pool() -> None: | |
| global _ssh_tunnel, _db_pool | |
| ssh_host = settings.ssh_host | |
| db_kwargs = dict( | |
| dbname=settings.db_name, | |
| user=settings.db_user, | |
| password=settings.db_password, | |
| connect_timeout=3, | |
| options="-c statement_timeout=5000", | |
| ) | |
| if ssh_host: | |
| _ssh_tunnel = SSHTunnelForwarder( | |
| (ssh_host, 22), | |
| ssh_username=settings.ssh_user, | |
| ssh_pkey=settings.ssh_key_path, | |
| remote_bind_address=("localhost", 5433), | |
| ) | |
| _ssh_tunnel.start() | |
| db_kwargs.update(host="localhost", port=_ssh_tunnel.local_bind_port) | |
| else: | |
| db_kwargs["host"] = "localhost" | |
| _db_pool = pg_pool.ThreadedConnectionPool(minconn=1, maxconn=5, **db_kwargs) | |
| def shutdown_db_pool() -> None: | |
| if _db_pool: | |
| _db_pool.closeall() | |
| if _ssh_tunnel: | |
| _ssh_tunnel.stop() | |
| _ssh_tunnel: Optional[SSHTunnelForwarder] = None | |
| _db_pool: Optional[pg_pool.ThreadedConnectionPool] = None | |
| _log_executor = ThreadPoolExecutor(max_workers=4, thread_name_prefix="chatbot_log") | |
| _client: Optional[OpenAI] = None | |
| _client_lock = threading.Lock() | |
| class ChatReq(BaseModel): | |
| text: Optional[str] = None | |
| messages: Optional[list[dict[str, str]]] = None | |
| engine: Optional[str] = None | |
| def init_db_pool() -> None: | |
| global _ssh_tunnel, _db_pool | |
| if _db_pool is not None: | |
| return | |
| ssh_host = settings.ssh_host | |
| db_kwargs = dict( | |
| dbname=settings.db_name, | |
| user=settings.db_user, | |
| password=settings.db_password, | |
| connect_timeout=3, | |
| options="-c statement_timeout=5000", | |
| ) | |
| if ssh_host: | |
| _ssh_tunnel = SSHTunnelForwarder( | |
| (ssh_host, 22), | |
| ssh_username=settings.ssh_user, | |
| ssh_pkey=settings.ssh_key_path, | |
| remote_bind_address=("localhost", 5433), | |
| ) | |
| _ssh_tunnel.start() | |
| db_kwargs.update(host="localhost", port=_ssh_tunnel.local_bind_port) | |
| else: | |
| db_kwargs["host"] = "localhost" | |
| _db_pool = pg_pool.ThreadedConnectionPool(minconn=1, maxconn=5, **db_kwargs) | |
| def shutdown_db_pool() -> None: | |
| global _db_pool, _ssh_tunnel | |
| try: | |
| _log_executor.shutdown(wait=True, cancel_futures=False) | |
| except Exception as exc: | |
| print(f"[chatbot_log executor shutdown ERROR] {exc}") | |
| if _db_pool: | |
| _db_pool.closeall() | |
| _db_pool = None | |
| if _ssh_tunnel: | |
| _ssh_tunnel.stop() | |
| _ssh_tunnel = None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@LLM/OSS/service.py` around lines 47 - 90, init_db_pool is not idempotent and
can leak SSH tunnels and DB connections; before creating a new
SSHTunnelForwarder or ThreadedConnectionPool, check for and clean up existing
resources: if _ssh_tunnel is not None call _ssh_tunnel.stop() and set
_ssh_tunnel = None, and if _db_pool is not None call _db_pool.closeall() and set
_db_pool = None, then proceed to create and assign the new instances (refer to
init_db_pool, _ssh_tunnel, _db_pool). Also update shutdown_db_pool to explicitly
shutdown the log executor by calling _log_executor.shutdown(wait=False) (and
optionally set _log_executor = None) after closing the DB pool and stopping the
tunnel so the module-level ThreadPoolExecutor doesn't keep the process alive
(refer to shutdown_db_pool and _log_executor).
Comment on lines
45
to
50
| def get_device() -> torch.device: | ||
| if torch.backends.mps.is_available(): | ||
| return torch.device("mps") | ||
| if torch.cuda.is_available(): | ||
| return torch.device("cuda") | ||
| return torch.device("cpu") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: 디바이스 선택 로직을 검색하여 잘못된 패턴이나 일관성 없는 구현을 찾습니다.
# Test 1: torch.device 호출을 모두 찾아 컨텍스트와 함께 표시
echo "=== torch.device 사용 패턴 ==="
rg -n -C3 'torch\.device\(' --type=py
# Test 2: get_device 함수 정의를 모두 찾기
echo -e "\n=== get_device 함수 정의 ==="
ast-grep --pattern 'def get_device($$$): $$$'
# Test 3: CUDA/MPS 가용성 체크 패턴 찾기
echo -e "\n=== CUDA/MPS 가용성 체크 ==="
rg -n 'torch\.(cuda|backends\.mps)\.is_available\(\)' --type=pyRepository: dongsooop/AI
Length of output: 2105
디바이스 선택 로직의 우선순위와 버그를 수정하세요.
get_device() 함수의 MPS > CUDA 우선순위는 일반적인 성능 특성과 일치하지 않습니다. 같은 프로젝트의 LLM/finetune_chatbot.py에서는 CUDA > MPS > CPU 순서를 사용하고 있으므로, 전체 코드베이스에서 일관된 우선순위 전략을 정의하고 적용해야 합니다.
또한 text_filtering/bad_text_learning_model.py:129의 로직 오류를 반드시 수정하세요. 현재 코드는 CUDA가 사용 가능할 때 "mps"를 선택하는데, 이는 명백히 잘못된 조건문입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@text_filtering/service.py` around lines 45 - 50, get_device() currently
prefers MPS over CUDA; change its priority to CUDA > MPS > CPU to match the
project's canonical ordering used in LLM/finetune_chatbot.py, i.e., check
torch.cuda.is_available() first, then torch.backends.mps.is_available(),
otherwise CPU. Also fix the logic in text_filtering/bad_text_learning_model.py
around the device selection (the incorrect branch at or near line 129 that picks
"mps" when CUDA is available) so it follows the same CUDA > MPS > CPU checks and
selects the correct torch.device string.
This was referenced Apr 25, 2026
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.
관련 이슈
Close #91
🎯 배경
🔍 주요 내용
service.py생성modes.py생성formatter.py생성변경 요약(1~3줄)
챗봇의 라우팅과 서비스(비즈니스) 로직을 분리해 가독성·유지보수성을 개선했습니다. Open_AI_OSS.py를 라우터 얇은 래퍼로 바꾸고 service.py, modes.py, formatter.py 등을 도입해 기능별 책임을 명확히 했습니다.
주요 변경점(3~7개)
주의/리스크(있으면 1~3개)
다음 액션(있으면 1~3개)