-
Notifications
You must be signed in to change notification settings - Fork 3
CU-869ah00vt Fix memory leak (mc-service) #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a44703b
09a4f9f
7cd6cc5
bdaeaf7
6428a16
6a6eef4
db6a131
73d8c4b
6dc73f8
393d182
f573b4d
6bd3fda
9493de4
1c75c68
d40b261
9cbe7fd
ea8f55d
8d2947f
4534172
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,52 @@ | ||
import logging | ||
from functools import lru_cache | ||
from typing import Annotated | ||
from typing import Annotated, Optional | ||
|
||
from fastapi import Depends | ||
from fastapi import Depends, Request | ||
|
||
from medcat_service.config import Settings | ||
from medcat_service.nlp_processor.medcat_processor import MedCatProcessor | ||
|
||
log = logging.getLogger(__name__) | ||
|
||
processor_singleton: Optional[MedCatProcessor] = None | ||
settings_singleton: Optional[Settings] = None | ||
|
||
@lru_cache | ||
def get_settings() -> Settings: | ||
settings = Settings() | ||
log.debug("Using settings: %s", settings) | ||
return settings | ||
|
||
def get_settings(request: Request) -> Settings: | ||
_settings = request.app.state.settings | ||
log.debug("Using settings: %s", _settings) | ||
return _settings | ||
|
||
@lru_cache | ||
def get_medcat_processor(settings: Annotated[Settings, Depends(get_settings)]) -> MedCatProcessor: | ||
log.debug("Creating new Medcat Processsor using settings: %s", settings) | ||
return MedCatProcessor(settings) | ||
|
||
def set_global_settings(settings: Settings) -> None: | ||
vladd-bit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
global settings_singleton | ||
settings_singleton = settings | ||
|
||
|
||
def get_global_settings() -> Settings: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? Doesn't seem to be called anywhere? |
||
if settings_singleton is None: | ||
raise RuntimeError("Settings have not been initialised yet") | ||
return settings_singleton | ||
|
||
|
||
def set_global_processor(proc: MedCatProcessor): | ||
global processor_singleton | ||
processor_singleton = proc | ||
|
||
|
||
def get_medcat_processor(request: Request) -> MedCatProcessor: | ||
proc = getattr(request.app.state, "medcat", None) | ||
log.debug("Getting MedCatProcessor from app.state: %s", proc) | ||
if proc is None: | ||
raise RuntimeError("MedCatProcessor is not initialised on app.state") | ||
return proc | ||
|
||
|
||
def get_global_processor() -> MedCatProcessor: | ||
if processor_singleton is None: | ||
raise RuntimeError("MedCatProcessor has not been initialised yet") | ||
return processor_singleton | ||
|
||
|
||
SettingsDep = Annotated[Settings, Depends(get_settings)] | ||
MedCatProcessorDep = Annotated[MedCatProcessor, Depends(get_medcat_processor)] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,55 @@ | ||
import logging | ||
from contextlib import asynccontextmanager | ||
|
||
import gradio as gr | ||
from fastapi import FastAPI, Request | ||
from fastapi.responses import JSONResponse | ||
|
||
from medcat_service.config import Settings | ||
from medcat_service.demo.gradio_demo import io | ||
from medcat_service.dependencies import get_settings | ||
from medcat_service.dependencies import set_global_processor, set_global_settings | ||
from medcat_service.nlp_processor.medcat_processor import MedCatProcessor | ||
from medcat_service.routers import admin, health, process | ||
from medcat_service.types import HealthCheckFailedException | ||
|
||
settings = get_settings() | ||
|
||
app = FastAPI( | ||
title="MedCAT Service", | ||
summary="MedCAT Service", | ||
contact={ | ||
@asynccontextmanager | ||
async def lifespan(app: FastAPI): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the more general comment: Is this really the way to go? I really don't understand how marts suggestion wouldn't have worked to make a single global settings object, and then stick with the lru_cache from before. I don't see how that could ever make a new Settings object and trigger the cache with a new input Reason I'm really hesitant is that your change here is basically saying "Following FastAPI documentation causes memory leaks" which I dont think is correct. It's also saying "Dont use FastAPI dependencies" in this project, which also doesnt seem right - it's really not a unique project...
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not suggesting FastAPI deps and guides are wrong or that the cache cant work. Since we already converted m-cat proc to singleton and got rid of the cache, I opted to do the same for settings (as per Mart's suggestion), thats all, but if we want to revert to cache for settings only thats fine. We cannot have both singleton and @lru_cache, that would not be consistent and would not add any value. Regarding the lifespan, I opted for it because we got explicit control over when and where the processor gets created ( there is the potential problem of having the model loaded as soon as the app starts though). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify - I mean, taking a step back, is it not possible to keep it all basically how it was before, just with the settings being explicitly global? dependencies.py _def_settings: Optional[Settings] = None
def get_settings() -> Settings:
global _def_settings
if _def_settings is None:
_def_settings = Settings()
return settings
...
@lru_cache
def get_medcat_processor(settings: Annotated[Settings, Depends(get_settings)]) -> MedCatProcessor:
log.debug("Creating new Medcat Processsor using settings: %s", settings)
return MedCatProcessor(settings) Then there are no changes needed anywhere else (I think?) |
||
|
||
log = logging.getLogger(__name__) | ||
log.debug("Starting MedCAT Service lifespan setup") | ||
|
||
# allow overriding settings and medcat processor for testing | ||
settings = getattr(app.state, "settings", None) | ||
if settings is None: | ||
settings = Settings() | ||
app.state.settings = settings | ||
vladd-bit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
medcat = getattr(app.state, "medcat", None) | ||
if medcat is None: | ||
medcat = MedCatProcessor(settings) | ||
app.state.medcat = medcat | ||
mart-r marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
app.state.title = "MedCAT Service", | ||
app.state.summary = "MedCAT Service", | ||
app.state.contact = { | ||
"name": "CogStack Org", | ||
"url": "https://cogstack.org/", | ||
"email": "[email protected]", | ||
}, | ||
license_info={ | ||
app.state.license_info = { | ||
"name": "Apache 2.0", | ||
"identifier": "Apache-2.0", | ||
}, | ||
root_path=settings.app_root_path, | ||
) | ||
app.state.root_path = settings.app_root_path | ||
|
||
set_global_settings(settings) | ||
set_global_processor(medcat) | ||
vladd-bit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
log.debug("MedCAT Service lifespan setup complete") | ||
|
||
yield | ||
|
||
app = FastAPI(lifespan=lifespan) | ||
|
||
app.include_router(admin.router) | ||
app.include_router(health.router) | ||
|
@@ -35,9 +62,9 @@ | |
async def healthcheck_failed_exception_handler(request: Request, exc: HealthCheckFailedException): | ||
return JSONResponse(status_code=503, content=exc.reason.model_dump()) | ||
|
||
|
||
if __name__ == "__main__": | ||
# Only run this when directly executing `python main.py` for local dev. | ||
import os | ||
|
||
import uvicorn | ||
uvicorn.run("medcat_service.main:app", host="0.0.0.0", port=int(os.environ.get("SERVER_PORT", 8000))) |
Uh oh!
There was an error while loading. Please reload this page.