Add validation results in MEModelRead#191
Conversation
app/db/model.py
Outdated
| ) | ||
|
|
||
|
|
||
| class Entity(LegacyMixin, Identifiable): |
There was a problem hiding this comment.
it would be helpful if the class Entity wasn't moved around, so the diff is smaller, and it makes it easier to see the changes, could you move it back?
There was a problem hiding this comment.
done, I moved it back
app/schemas/me_model.py
Outdated
| etypes: list[ETypeClassRead] | None | ||
| morphology: ReconstructionMorphologyRead | ||
| emodel: EModelRead | ||
| validation_results: list[ValidationResultRead] | None = None |
There was a problem hiding this comment.
There is a discussion ongoing on the other PR on how to handle nested resources:
Depending on what's decided there we would potentially remove this field and instead have a
/validation_results endpoint with a memodel_id__in filter.
There was a problem hiding this comment.
I think it should be fine to add it to the list view, there are only a few validations per me_model and they only contain 1 asset is that right @AurelienJaquier ?
What do you think @eleftherioszisis ?
There was a problem hiding this comment.
Sure, fine by me!
There was a problem hiding this comment.
As of now, an me-model should have up to 7 ValidationResults, and each ValidationResult can contain 2 to 3 assets (1 or 2 figures, and a validation_details.txt giving additional information about the run of the validation).
There was a problem hiding this comment.
Actually @AurelienJaquier
The response is already quite hefty, look at all those joins.
And we already have a /validation_results enpoint which is filterable by validated_entity_id that's all the frontend needs.
Unless you need the validation results in batch, I suggest we use the endpoint above instead of nesting the data here.
return select.options(
joinedload(MEModel.species),
joinedload(MEModel.strain),
joinedload(MEModel.emodel).options(
joinedload(EModel.species),
joinedload(EModel.strain),
joinedload(EModel.exemplar_morphology),
joinedload(EModel.brain_region),
selectinload(EModel.contributions).joinedload(Contribution.agent),
selectinload(EModel.contributions).joinedload(Contribution.role),
joinedload(EModel.mtypes),
joinedload(EModel.etypes),
joinedload(EModel.created_by),
joinedload(EModel.updated_by),
selectinload(EModel.assets),
),
joinedload(MEModel.morphology).options(
joinedload(ReconstructionMorphology.brain_region),
selectinload(ReconstructionMorphology.contributions).selectinload(Contribution.agent),
selectinload(ReconstructionMorphology.contributions).selectinload(Contribution.role),
joinedload(ReconstructionMorphology.mtypes),
joinedload(ReconstructionMorphology.license),
joinedload(ReconstructionMorphology.species),
joinedload(ReconstructionMorphology.strain),
joinedload(ReconstructionMorphology.created_by),
joinedload(ReconstructionMorphology.updated_by),
selectinload(ReconstructionMorphology.assets),
),
joinedload(MEModel.brain_region),
selectinload(MEModel.contributions).joinedload(Contribution.agent),
selectinload(MEModel.contributions).joinedload(Contribution.role),
joinedload(MEModel.mtypes),
joinedload(MEModel.etypes),
joinedload(MEModel.created_by),
joinedload(MEModel.updated_by),
selectinload(MEModel.validation_results),There was a problem hiding this comment.
Ok, then I will close this PR, and just use entitysdk to search directly ValidationResult with filtering by validated_entity_id.
app/db/model.py
Outdated
| "ValidationResult", | ||
| primaryjoin=f"foreign(ValidationResult.validated_entity_id) == {cls.__name__}.id", | ||
| foreign_keys="[ValidationResult.validated_entity_id]", | ||
| cascade="all, delete-orphan", |
There was a problem hiding this comment.
I think delete-orphan is not necessary delete only works. Since the validated_entity_id is not nullable orphans are impossible.
There was a problem hiding this comment.
Ok, done in latest commit
app/db/model.py
Outdated
| primaryjoin=f"foreign(ValidationResult.validated_entity_id) == {cls.__name__}.id", | ||
| foreign_keys="[ValidationResult.validated_entity_id]", | ||
| cascade="all, delete-orphan", | ||
| lazy="dynamic", # or "select", as needed |
There was a problem hiding this comment.
I think we want 'selectin' in case we want to fetch the validation results of multiple entities at once.
But in general we're not using this we're defining the eager loading function manually here:
entitycore/app/service/memodel.py
Line 37 in f851d5b
I'm surprised this passes the tests it should raise an exception because of the raiseload("*"),
There was a problem hiding this comment.
I removed the lazy key, and added selectinload(MEModel.validation_results) in _load in latest commit
| msg = f"{cls} should be an Entity" | ||
| raise TypeError(msg) | ||
|
|
||
| return relationship( |
There was a problem hiding this comment.
I think we should pass uselist=True
There was a problem hiding this comment.
Ok, done in latest commit
app/schemas/me_model.py
Outdated
| etypes: list[ETypeClassRead] | None | ||
| morphology: ReconstructionMorphologyRead | ||
| emodel: EModelRead | ||
| validation_results: list[ValidationResultRead] | None = None |
There was a problem hiding this comment.
validation_results: list[ValidationResultRead] | None = N one
None is not a possible value, if there are no validation results sqlalchemy will return an empty list.
There was a problem hiding this comment.
I turned this line into validation_results: list[ValidationResultRead] = [] in latest commit
4 participants
I made it so the ValidationResult entities that point to an MEModel show up in the MEModelRead.
I am not sure if the code is good quality, but I tested it locally and it was working