-
Notifications
You must be signed in to change notification settings - Fork 6
[Issue 191] Transformer Scaffolding #192
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| from abc import ABC, abstractmethod | ||
| from datetime import datetime, UTC | ||
| from typing import Annotated, Any, final | ||
| from uuid import UUID, uuid4 | ||
|
|
||
| from pydantic import Field | ||
|
|
||
| from common_grants_sdk.schemas.base import CommonGrantsBaseModel | ||
| from common_grants_sdk.schemas.models.opp_base import OpportunityBase | ||
| from common_grants_sdk.schemas.models.opp_funding import OppFunding | ||
| from common_grants_sdk.schemas.models.opp_status import OppStatus | ||
| from common_grants_sdk.schemas.models.opp_timeline import OppTimeline | ||
|
|
||
|
|
||
| class OpportunityTransformer(CommonGrantsBaseModel, ABC): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the motivation for inheriting from the Is there a reason someone implementing this function would call Some alternative patterns that I think would make more sense:
|
||
| """ | ||
| Base class for transforming arbitrary data structures | ||
| into canonical models. | ||
| """ | ||
|
|
||
| source_data: Annotated[ | ||
| dict[str, Any], | ||
| Field( | ||
| description="Arbitrary source data", | ||
| ), | ||
| ] | ||
|
|
||
| def transform_opportunity(self, id: UUID | None = None) -> OpportunityBase: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to my comments below on the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or if we're sticking with the pattern of having one instance of this transformer per record, expecting an ID value on initialization or generating and saving the UUID when the transformer is instantiated with input data. |
||
| """ | ||
| Extract and transform opportunity data from source_data | ||
| into a canonical OpportunityBase model instance. | ||
| """ | ||
|
|
||
| return OpportunityBase( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works if folks only want to use the default An alternative would be to make this a generic class to which developers can pass a sub-class of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the generic classes docs for more info |
||
| id=id or self._generate_id(), | ||
| title=self.transform_opportunity_title(), | ||
| description=self.transform_opportunity_description(), | ||
| status=self.transform_opportunity_status(), | ||
| funding=self.transform_opportunity_funding(), | ||
| key_dates=self.transform_opportunity_timeline(), | ||
| created_at=self._current_timestamp(), | ||
| last_modified_at=self._current_timestamp(), | ||
|
Comment on lines
+41
to
+42
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll likely want to allow consumers of this function to optionally provide existing |
||
| ) | ||
|
|
||
| @abstractmethod | ||
| def transform_opportunity_description(self) -> str: | ||
| """ | ||
| Extract and transform description data from source_data | ||
| into a canonical description string. | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
| @abstractmethod | ||
| def transform_opportunity_funding(self) -> OppFunding: | ||
| """ | ||
| Extract and transform funding data from source_data | ||
| into a canonical OppFunding model instance. | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
| @abstractmethod | ||
| def transform_opportunity_status(self) -> OppStatus: | ||
| """ | ||
| Extract and transform status data from source_data | ||
| into a canonical OppStatus model instance. | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
| @abstractmethod | ||
| def transform_opportunity_timeline(self) -> OppTimeline: | ||
| """ | ||
| Extract and transform timeline data from source_data | ||
| into a canonical OppTimeline model instance. | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
| @abstractmethod | ||
| def transform_opportunity_title(self) -> str: | ||
| """ | ||
| Extract and transform title data from source_data | ||
| into a canonical title string. | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
| @final | ||
| def _generate_id(self) -> UUID: | ||
| """Generate unique ID for the opportunity.""" | ||
| return uuid4() | ||
|
Comment on lines
+90
to
+93
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might either want to replace this with a deterministic UUID method (e.g. v5) or at least provide an alternative internal function that supports deterministic UUID generation. The current approach would generate two different UUIDs for the same input data. |
||
|
|
||
| @final | ||
| def _current_timestamp(self) -> datetime: | ||
| """Get current timestamp.""" | ||
| return datetime.now(UTC) | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
This doesn't align with the pattern that we're trying to establish here. Adopters shouldn't have to define a custom transformer in Python code.
The goal is to define simple transformations using a serializable mapping that is portable across languages and provide language-specific utilities to automate transformations using that mapping.
Uh oh!
There was an error while loading. Please reload this page.
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.
Let's discuss. IMO this is compatible. A transformer pattern and portable serializable mapping formats are not mutually exclusive and developers can benefit from one, the other, or both combined.
Nor should they have to learn a complicated deeply-nested mapping format if they simply want to instantiate an object from a known data source! :) IMO we should give them options.
The spirit of this PR is to provide a very simple scaffold for enabling the ADR stated goal ("flexibly map existing data structures to the canonical models") within the accepted tradeoffs ("requires a custom runtime to apply transformations in each new language or SDK") which can be extended to support any number of formats and transformers (such as the draft PR you opened today) OR used as-is without use of any mapping format.
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.
While I agree these aren't incompatible approaches, maybe we can talk tomorrow about the benefit that this transformer ABC is providing if we want to keep it as an option?
Maybe you could outline the why someone might implement this ABC versus creating their own custom transformer function / class? Typically, the main use case for ABCs in Python is to support polymorphism, providing a consistent interface for downstream consumers of objects that implement this interface. Did you anticipate someone implementing multiple versions of this transformer? Or was there some specific downstream consumer you had in mind that would create the need for a consistent interface for the transformer class?
I could see the value of providing a standard Python interface for transformations that would be consistent across CommonGrants models, like Opportunity (current) and Organization, Person, etc. (future), but it would probably make more sense to define an ABC that is generic (e.g.
ModelTransformer) rather than specific to a given model.I'll also take some time to provide feedback on some specific design choices in this transformer ABC, because even if we decide to proceed with this as an option, there are a few things I'd recommend tweaking.