-
Notifications
You must be signed in to change notification settings - Fork 0
Add validation results in MEModelRead #191
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
Conversation
app/db/model.py
Outdated
@@ -303,6 +303,74 @@ def etypes(cls) -> Mapped[list["ETypeClass"]]: | |||
) | |||
|
|||
|
|||
class Entity(LegacyMixin, Identifiable): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, I moved it back
app/schemas/me_model.py
Outdated
@@ -59,3 +60,4 @@ class MEModelRead( | |||
etypes: list[ETypeClassRead] | None | |||
morphology: ReconstructionMorphologyRead | |||
emodel: EModelRead | |||
validation_results: list[ValidationResultRead] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, fine by me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
def _load(select: Select): |
I'm surprised this passes the tests it should raise an exception because of the raiseload("*"),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should pass uselist=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done in latest commit
app/schemas/me_model.py
Outdated
@@ -59,3 +60,4 @@ class MEModelRead( | |||
etypes: list[ETypeClassRead] | None | |||
morphology: ReconstructionMorphologyRead | |||
emodel: EModelRead | |||
validation_results: list[ValidationResultRead] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I turned this line into validation_results: list[ValidationResultRead] = []
in latest commit
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