Skip to content

Fix missing ion channel models in memodel's emodel #183

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix missing ion channel models in memodel's emodel #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions app/schemas/emodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,4 @@ class EModelRead(
mtypes: list[MTypeClassRead] | None
etypes: list[ETypeClassRead] | None
exemplar_morphology: ExemplarMorphology


class EModelReadExpanded(EModelRead, AssetsMixin):
ion_channel_models: list[IonChannelModelWAssets]
Copy link
Collaborator

@GianlucaFicarelli GianlucaFicarelli May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, in the get_many endpoint we return all the ion_channel_models, with all the assets (that are not paginated because they are nested entities).
Do we have an idea of the numbers of nested entities, and how it affects the response time and size?
Should we consider reducing the default number of results, that's now set to 100 ?

Copy link
Contributor Author

@eleftherioszisis eleftherioszisis May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AurelienJaquier could you please provide some insight here?

Copy link
Contributor

@AurelienJaquier AurelienJaquier May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each ion channel model should have only one asset: the mod file.
The e-models we currently have on the platform each have a small number of ion channel models (around 10). In the future, we will probably have e-models with genetic ion channals. Those e-models will have a larger number of ion channel models (>30 if I recall correctly).

Copy link
Contributor

@g-bar g-bar May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AurelienJaquier I don't understand why you need the ion channel models in the list view of MEModel, do you do some sort of bulk processing of memodels that need the ion channel models for all.

Otherwise you can just do one extra call to get the emodel from the me.model.emodel.id?

Copy link
Contributor

@g-bar g-bar May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe for validation?

Copy link
Contributor

@g-bar g-bar May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back to pragmatism, we could handle the MEModel.emodel lazily which seems particularly problematic for now. And decide later on a more general solution if / when the need arises?

Copy link
Contributor Author

@eleftherioszisis eleftherioszisis May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just ignore it for now. I am not eager rewriting the sdk for a single thing that needs to be fixed.

Copy link
Collaborator

@GianlucaFicarelli GianlucaFicarelli May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid lazy implicit loading in entitysdk because:

  • it has side effects that are hidden to the user
  • it doesn't fit well with the async version (that's not available yet, but it would be needed for integration in async services), since you cannot await on an attribute

Having the single expanded parameter could work, to avoid adding the complexity of multiple expanded models, although at the moment the parameters is available only in the get_one endpoint.

Copy link
Contributor

@g-bar g-bar May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has side effects that are hidden to the user

The entirety of entitysdk is predicated on hiding the complexity of a rest api from end users, otherwise we could do away with it and everyone just does calls directly to the api, furthermore it can be documented that that field is loaded lazily.

it doesn't fit well with the async version

@properties can be made awaitable if they return a coroutine.

single expanded parameter could work

Yeah I agree.

Ultimately for now I think we should ignore it we're splitting hairs over some nested data that's not even needed by anyone.

Maybe for now emit a warning when users access MEModel.emodel to made them aware that they need to make an extra call to get the full emodel @eleftherioszisis ?

Copy link
Collaborator

@GianlucaFicarelli GianlucaFicarelli May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entirety of entitysdk is predicated on hiding the complexity of a rest api from end users, otherwise we could do away with it and everyone just does calls directly to the api, furthermore it can be documented that it does an extra call.

I don't expect that a web service is called when accessing a property, but I can expect one or more calls to be executed when calling a method.
The usual expectation of accessing a property is that it returns quickly, but a web call is blocking.
In the PEP-8, it's mentioned to avoid properties for computationally expensive operations, but a web call seems not too different to me:

Note 2: Avoid using properties for computationally expensive operations; the attribute notation makes the caller believe that access is (relatively) cheap.

@properties can be made awaitable if they return a coroutine.

In this case you should know which attributes are lazy and which not, and do something that looks quite unusual like:

x = await model.ion_channel_models

You can also have a look at the result of this draft in entity-management for a possible implementation that allows lazy-loading with properties and async code, but we weren't happy with it: BlueBrain/entity-management#26

10 changes: 5 additions & 5 deletions app/service/emodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
from app.dependencies.db import SessionDep
from app.filters.emodel import EModelFilterDep
from app.queries.common import router_create_one, router_read_many, router_read_one
from app.schemas.emodel import EModelCreate, EModelRead, EModelReadExpanded
from app.schemas.emodel import EModelCreate, EModelRead
from app.schemas.types import ListResponse

if TYPE_CHECKING:
Expand Down Expand Up @@ -61,13 +61,13 @@ def read_one(
user_context: UserContextDep,
db: SessionDep,
id_: uuid.UUID,
) -> EModelReadExpanded:
) -> EModelRead:
return router_read_one(
id_=id_,
db=db,
db_model_class=EModel,
authorized_project_id=user_context.project_id,
response_schema_class=EModelReadExpanded,
response_schema_class=EModelRead,
apply_operations=_load,
)

Expand Down Expand Up @@ -96,7 +96,7 @@ def read_many(
with_search: SearchDep,
facets: FacetsDep,
in_brain_region: InBrainRegionDep,
) -> ListResponse[EModelReadExpanded]:
) -> ListResponse[EModelRead]:
morphology_alias = aliased(ReconstructionMorphology, flat=True)
agent_alias = aliased(Agent, flat=True)
created_by_alias = aliased(Agent, flat=True)
Expand Down Expand Up @@ -170,7 +170,7 @@ def read_many(
apply_filter_query_operations=None,
apply_data_query_operations=_load,
pagination_request=pagination_request,
response_schema_class=EModelReadExpanded,
response_schema_class=EModelRead,
name_to_facet_query_params=name_to_facet_query_params,
filter_model=emodel_filter,
filter_joins=filter_joins,
Expand Down
1 change: 1 addition & 0 deletions app/service/memodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ def _load(select: Select):
joinedload(EModel.createdBy),
joinedload(EModel.updatedBy),
selectinload(EModel.assets),
selectinload(EModel.ion_channel_models),
),
joinedload(MEModel.morphology).options(
joinedload(ReconstructionMorphology.brain_region),
Expand Down
34 changes: 17 additions & 17 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,27 +444,27 @@ def emodel_id(create_emodel_ids: CreateIds) -> str:

@pytest.fixture
def create_memodel_ids(
db, morphology_id, brain_region_id, species_id, strain_id, emodel_id, agents
db, client, morphology_id, brain_region_id, species_id, strain_id, emodel_id, agents
) -> CreateIds:
def _create_memodel_ids(count: int):
memodel_ids: list[str] = []
for i in range(count):
memodel_id = add_db(
db,
MEModel(
name=f"{i}",
description=f"{i}_description",
brain_region_id=brain_region_id,
species_id=species_id,
strain_id=strain_id,
morphology_id=morphology_id,
emodel_id=emodel_id,
authorized_public=False,
authorized_project_id=PROJECT_ID,
holding_current=0,
threshold_current=0,
),
).id
memodel_id = assert_request(
client.post,
url="/memodel",
json={
"name": f"{i}",
"description": f"{i}_description",
"brain_region_id": str(brain_region_id),
"species_id": str(species_id),
"strain_id": str(strain_id),
"morphology_id": str(morphology_id),
"emodel_id": str(emodel_id),
"authorized_public": False,
"holding_current": 0,
"threshold_current": 0,
},
).json()["id"]
Copy link
Collaborator

@GianlucaFicarelli GianlucaFicarelli May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to execute a call instead of adding the record to the db?
My main concern is that this pattern could be more expensive, and slow down the tests if used in several tests and loops, but it should be measured to see if it's negligible or not.

Copy link
Contributor Author

@eleftherioszisis eleftherioszisis May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason is registering the createdBy/updatedBy that I then check on the tests. We can assume it works but I am more reassured with the explicit checks.


add_contributions(db, agents, memodel_id)

Expand Down
78 changes: 44 additions & 34 deletions tests/test_emodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,59 +1,69 @@
import itertools as it
import uuid

import pytest
from fastapi.testclient import TestClient

from app.db.model import EModel
from app.db.types import EntityType

from .conftest import CreateIds, EModelIds
from .utils import create_reconstruction_morphology_id
from .utils import assert_request, create_reconstruction_morphology_id
from tests.routers.test_asset import _upload_entity_asset

ROUTE = "/emodel"


def test_create_emodel(client: TestClient, species_id, strain_id, brain_region_id, morphology_id):
response = client.post(
ROUTE,
json={
"brain_region_id": str(brain_region_id),
"species_id": species_id,
"strain_id": strain_id,
"description": "Test EModel Description",
"name": "Test EModel Name",
"iteration": "test iteration",
"score": -1,
"seed": -1,
"exemplar_morphology_id": morphology_id,
},
)
assert response.status_code == 200, f"Failed to create emodel: {response.text}"
data = response.json()
assert data["brain_region"]["id"] == str(brain_region_id), (
f"Failed to get id for emodel: {data}"
)
assert data["species"]["id"] == species_id, f"Failed to get species_id for emodel: {data}"
assert data["strain"]["id"] == strain_id, f"Failed to get strain_id for emodel: {data}"
@pytest.fixture
def json_data(db, emodel_id):
emodel = db.get(EModel, emodel_id)
return {
"brain_region_id": str(emodel.brain_region_id),
"species_id": str(emodel.species_id),
"strain_id": str(emodel.strain_id),
"description": emodel.description,
"name": emodel.name,
"iteration": emodel.iteration,
"score": emodel.score,
"seed": emodel.seed,
"exemplar_morphology_id": str(emodel.exemplar_morphology_id),
}


def _assert_read_response(data, json_data):
assert data["name"] == json_data["name"]
assert data["description"] == json_data["description"]
assert data["brain_region"]["id"] == json_data["brain_region_id"]
assert data["species"]["id"] == json_data["species_id"]
assert data["strain"]["id"] == json_data["strain_id"]
assert data["createdBy"]["id"] == data["updatedBy"]["id"]
assert data["exemplar_morphology"]["id"] == json_data["exemplar_morphology_id"]
assert data["iteration"] == json_data["iteration"]
assert data["score"] == json_data["score"]
assert data["seed"] == json_data["seed"]
assert "ion_channel_models" in data
assert "assets" in data

response = client.get(ROUTE)
assert response.status_code == 200, f"Failed to get emodels: {response.text}"
data = response.json()["data"]
assert data[0]["createdBy"]["id"] == data[0]["updatedBy"]["id"]

def test_create_emodel(client: TestClient, json_data):
data = assert_request(client.post, url=ROUTE, json=json_data).json()
_assert_read_response(data, json_data)

data = assert_request(client.get, url=ROUTE).json()["data"]
_assert_read_response(data[0], json_data)

def test_get_emodel(client: TestClient, emodel_id: str):

def test_get_emodel(client: TestClient, emodel_id: str, json_data):
_upload_entity_asset(client, EntityType.emodel, uuid.UUID(emodel_id))

response = client.get(f"{ROUTE}/{emodel_id}")
data = assert_request(
client.get,
url=f"{ROUTE}/{emodel_id}",
).json()
_assert_read_response(data, json_data)

assert response.status_code == 200
data = response.json()
assert data["id"] == emodel_id
assert "assets" in data
assert len(data["assets"]) == 1
assert "ion_channel_models" in data
assert data["createdBy"]["id"] == data["updatedBy"]["id"]


def test_missing(client):
Expand Down
94 changes: 49 additions & 45 deletions tests/test_memodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import operator as op
import uuid

import pytest
from fastapi.testclient import TestClient

from app.db.model import MEModel
Expand All @@ -9,28 +10,58 @@
from .conftest import CreateIds, MEModels
from .utils import (
PROJECT_ID,
assert_request,
check_brain_region_filter,
create_reconstruction_morphology_id,
)

ROUTE = "/memodel"


def test_get_memodel(client: TestClient, memodel_id):
response = client.get(f"{ROUTE}/{memodel_id}")
assert response.status_code == 200
data = response.json()
assert data["id"] == memodel_id
assert "morphology" in data
assert "emodel" in data
assert "brain_region" in data
assert "species" in data
assert "strain" in data
@pytest.fixture
def json_data(db, memodel_id):
me_model = db.get(MEModel, memodel_id)
return {
"brain_region_id": str(me_model.brain_region_id),
"species_id": str(me_model.species_id),
"strain_id": str(me_model.strain_id),
"description": me_model.description,
"name": me_model.name,
"morphology_id": str(me_model.morphology_id),
"emodel_id": str(me_model.emodel_id),
"holding_current": me_model.holding_current,
"threshold_current": me_model.threshold_current,
}


def _assert_read_response(data, json_data):
assert data["name"] == json_data["name"]
assert data["description"] == json_data["description"]
assert data["brain_region"]["id"] == json_data["brain_region_id"]
assert data["species"]["id"] == json_data["species_id"]
assert data["strain"]["id"] == json_data["strain_id"]
assert data["createdBy"]["id"] == data["updatedBy"]["id"]
assert data["threshold_current"] == json_data["threshold_current"]
assert data["holding_current"] == json_data["holding_current"]
assert data["emodel"]["id"] == json_data["emodel_id"]
assert "ion_channel_models" in data["emodel"]
assert data["morphology"]["id"] == json_data["morphology_id"]
assert "assets" in data["emodel"]
assert "assets" in data["morphology"]
assert "mtypes" in data
assert "etypes" in data
MEModelRead.model_validate(data)


def test_get_memodel(client: TestClient, memodel_id, json_data):
data = assert_request(
client.get,
url=f"{ROUTE}/{memodel_id}",
).json()
_assert_read_response(data, json_data)
assert data["id"] == memodel_id


def test_missing(client):
response = client.get(f"{ROUTE}/{uuid.uuid4()}")
assert response.status_code == 404
Expand All @@ -41,43 +72,16 @@ def test_missing(client):

def test_create_memodel(
client: TestClient,
species_id: str,
strain_id: str,
brain_region_id: int,
morphology_id: str,
emodel_id: str,
json_data,
):
response = client.post(
ROUTE,
json={
"brain_region_id": str(brain_region_id),
"species_id": species_id,
"strain_id": strain_id,
"description": "Test MEModel Description",
"name": "Test MEModel Name",
"morphology_id": morphology_id,
"emodel_id": emodel_id,
"holding_current": 0,
"threshold_current": 0,
},
)
assert response.status_code == 200, f"Failed to create memodel: {response.text}"
data = response.json()
assert data["brain_region"]["id"] == str(brain_region_id), (
f"Failed to get id for memodel: {data}"
)
assert data["species"]["id"] == species_id, f"Failed to get species_id for memodel: {data}"
assert data["strain"]["id"] == strain_id, f"Failed to get strain_id for memodel: {data}"
assert "assets" in data["emodel"]
assert "assets" in data["morphology"]
assert data["createdBy"]["id"] == data["updatedBy"]["id"]
data = assert_request(client.post, url=ROUTE, json=json_data).json()
_assert_read_response(data, json_data)

response = client.get(f"{ROUTE}/{data['id']}")
assert response.status_code == 200, f"Failed to get morphologys: {response.text}"
data = response.json()
assert "assets" in data["emodel"]
assert "assets" in data["morphology"]
assert data["createdBy"]["id"] == data["updatedBy"]["id"]
data = assert_request(client.get, url=f"{ROUTE}/{data['id']}").json()
_assert_read_response(data, json_data)

data = assert_request(client.get, url=ROUTE).json()["data"][0]
_assert_read_response(data, json_data)


def test_facets(client: TestClient, faceted_memodels: MEModels):
Expand Down