Fix missing ion channel models in memodel's emodel #183
Conversation
| "holding_current": 0, | ||
| "threshold_current": 0, | ||
| }, | ||
| ).json()["id"] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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] | |||
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@AurelienJaquier could you please provide some insight here?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Or just ignore it for now. I am not eager rewriting the sdk for a single thing that needs to be fixed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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_modelsYou 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
Use expanded EModel schema everywhere
Fixes openbraininstitute/entitysdk#49