Skip to content
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

refactor: remodel DoapTarget #130

Merged
merged 15 commits into from
Aug 14, 2024
Merged

refactor: remodel DoapTarget #130

merged 15 commits into from
Aug 14, 2024

Conversation

jnussbaum
Copy link
Collaborator

No description provided.

@jnussbaum jnussbaum self-assigned this Aug 12, 2024
@jnussbaum
Copy link
Collaborator Author

@BalduinLandolt This is my try-out of your idea how the DoapTarget could be remodeled. The result is pleasing, but I have a problem with the validation of nested pydantic models which makes the tests fail.

The problem is that inside the pydantic model Doap, I have a field target of type DoapTarget. Now, this type DoapTarget should be a protocol or ABC, because I want that the code uses one of the subtypes GroupDoapTarget or EntityDoapTarget.

The problem appears when reading serialized JSON data: The model_validator doesn't know what to do with the JSON data, because DoapTarget is nothing that pydantic can call.

I tried several solutions, but nothing worked. If you don't have an idea, I'll have to close this PR.

@BalduinLandolt
Copy link
Collaborator

BalduinLandolt commented Aug 12, 2024

The problem is that inside the pydantic model Doap, I have a field target of type DoapTarget. Now, this type DoapTarget should be a protocol or ABC, because I want that the code uses one of the subtypes GroupDoapTarget or EntityDoapTarget.

The problem appears when reading serialized JSON data: The model_validator doesn't know what to do with the JSON data, because DoapTarget is nothing that pydantic can call.

I tried several solutions, but nothing worked. If you don't have an idea, I'll have to close this PR.

Not sure if this would work and make things nice... but what about

class DoapTarget(BaseModel):
    project_iri: str
    target: GroupDoapTarget | EntityDoapTarget


class GroupDoapTarget(BaseModel):
    group: Group


class EntityDoapTarget(BaseModel):
    resource_class: str | None = None
    property: str | None = None

Or if that nesting turns out to be undesireable:

type DoapTarget = GroupDoapTarget | EntityDoapTarget


class GroupDoapTarget(BaseModel):
    project_iri: str
    group: Group


class EntityDoapTarget(BaseModel):
    project_iri: str
    resource_class: str | None = None
    property: str | None = None

But maybe it's just not possible to do this in a nice way, in python... in a more functional language, instead of a union type, an enum type could be used, which would make it much nicer. But Python's enums simply&sadly just don't work as types.

Edit: Another option may be extracting the deserialization to a factory method. But that would make the use of pydantic a bit pointless.

@jnussbaum
Copy link
Collaborator Author

The problem is that inside the pydantic model Doap, I have a field target of type DoapTarget. Now, this type DoapTarget should be a protocol or ABC, because I want that the code uses one of the subtypes GroupDoapTarget or EntityDoapTarget.
The problem appears when reading serialized JSON data: The model_validator doesn't know what to do with the JSON data, because DoapTarget is nothing that pydantic can call.
I tried several solutions, but nothing worked. If you don't have an idea, I'll have to close this PR.

Not sure if this would work and make things nice... but what about

class DoapTarget(BaseModel):
    project_iri: str
    target: GroupDoapTarget | EntityDoapTarget


class GroupDoapTarget(BaseModel):
    group: Group


class EntityDoapTarget(BaseModel):
    resource_class: str | None = None
    property: str | None = None

Or if that nesting turns out to be undesireable:

type DoapTarget = GroupDoapTarget | EntityDoapTarget


class GroupDoapTarget(BaseModel):
    project_iri: str
    group: Group


class EntityDoapTarget(BaseModel):
    project_iri: str
    resource_class: str | None = None
    property: str | None = None

But maybe it's just not possible to do this in a nice way, in python... in a more functional language, instead of a union type, an enum type could be used, which would make it much nicer. But Python's enums simply&sadly just don't work as types.

Edit: Another option may be extracting the deserialization to a factory method. But that would make the use of pydantic a bit pointless.

Thanks for your input. But the problem are not python's union types, I believe. Rather, when pydantic deserializes a JSON, and then encounters a field that is of type GroupDoapTarget | EntityDoapTarget, the poor pydantic is confused and doesn't know what to do: shall it deserialize this field as a GroupDoapTarget or as an EntityDoapTarget?

Perhaps it's possible to override the built-in model_validate or something like that, but I don't know pydantic well enough.

@BalduinLandolt
Copy link
Collaborator

The problem is that inside the pydantic model Doap, I have a field target of type DoapTarget. Now, this type DoapTarget should be a protocol or ABC, because I want that the code uses one of the subtypes GroupDoapTarget or EntityDoapTarget.
The problem appears when reading serialized JSON data: The model_validator doesn't know what to do with the JSON data, because DoapTarget is nothing that pydantic can call.
I tried several solutions, but nothing worked. If you don't have an idea, I'll have to close this PR.

Not sure if this would work and make things nice... but what about

class DoapTarget(BaseModel):
    project_iri: str
    target: GroupDoapTarget | EntityDoapTarget


class GroupDoapTarget(BaseModel):
    group: Group


class EntityDoapTarget(BaseModel):
    resource_class: str | None = None
    property: str | None = None

Or if that nesting turns out to be undesireable:

type DoapTarget = GroupDoapTarget | EntityDoapTarget


class GroupDoapTarget(BaseModel):
    project_iri: str
    group: Group


class EntityDoapTarget(BaseModel):
    project_iri: str
    resource_class: str | None = None
    property: str | None = None

But maybe it's just not possible to do this in a nice way, in python... in a more functional language, instead of a union type, an enum type could be used, which would make it much nicer. But Python's enums simply&sadly just don't work as types.
Edit: Another option may be extracting the deserialization to a factory method. But that would make the use of pydantic a bit pointless.

Thanks for your input. But the problem are not python's union types, I believe. Rather, when pydantic deserializes a JSON, and then encounters a field that is of type GroupDoapTarget | EntityDoapTarget, the poor pydantic is confused and doesn't know what to do: shall it deserialize this field as a GroupDoapTarget or as an EntityDoapTarget?

Perhaps it's possible to override the built-in model_validate or something like that, but I don't know pydantic well enough.

I rest my case, if proper sum types were a thing in Python, then Pydantic would know how to handle them... but that's splitting hairs.
In any case, I don't think it's worth fighting the type system over this kind of cosmetic modelling improvements.
By the way: Pydantic provides mechanisms to model sum types: https://blog.yossarian.net/2024/08/12/Approximating-sum-types-in-Python-with-Pydantic & https://docs.pydantic.dev/latest/concepts/unions/ but I don't think these provide great mechanisms compared to languages that provide fist class support for sum types.

@jnussbaum
Copy link
Collaborator Author

I rest my case, if proper sum types were a thing in Python, then Pydantic would know how to handle them... but that's splitting hairs. In any case, I don't think it's worth fighting the type system over this kind of cosmetic modelling improvements. By the way: Pydantic provides mechanisms to model sum types: https://blog.yossarian.net/2024/08/12/Approximating-sum-types-in-Python-with-Pydantic & https://docs.pydantic.dev/latest/concepts/unions/ but I don't think these provide great mechanisms compared to languages that provide fist class support for sum types.

Thanks for your links, thanks to them I could finally solve it. Now it works :-)

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

See my comments, otherwise I think it looks pretty clean, right?

dsp_permissions_scripts/doap/doap_get.py Outdated Show resolved Hide resolved
dsp_permissions_scripts/doap/doap_model.py Show resolved Hide resolved
@jnussbaum jnussbaum merged commit d1ed804 into main Aug 14, 2024
1 check passed
@jnussbaum jnussbaum deleted the wip/subclasses-for-doap-target branch August 14, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants