-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add requested jobs table #116
Conversation
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.
Looks good overall - I will follow up in slack about whether it makes sense to have this table variant from our others using the Mixin, but I do like the approach!
from typing import Any, Dict | ||
|
||
|
||
def numeric_to_decimal(value: Any) -> Any: |
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.
Can we update these "Any" type hints to reflect the supported operators? I see in the comments we mention it's an int or float but probably support Decimal as well?
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.
They are Any types. If you send a value in that isn't a numeric it is still a valid parameter it just isn't modified.
return value | ||
|
||
|
||
def decimal_to_numeric(value: Any) -> Any: |
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.
Same
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.
Same
return value | ||
|
||
|
||
class DataclassDDBMixin: |
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.
Following our organizational pattern it might be nice to move this out into it's own file since it is a class rather than a function and call it dataclass_ddb_mixin.py
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'll rename the whole thing to dataclass_ddb_mixin.py. The one class per file thing is a Java convention not a Python one and we have plenty of cases where classes/functions/etc. are all grouped meaningfully under a single module.
33fee3c
to
9ee13e5
Compare
9ee13e5
to
a8e13e1
Compare
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.
LGTM
This change adds a new job status table to the ModelRunner database module. This table will be used by a future load based scheduler as temporary storage for image processing jobs that have been pulled from the SQS queue but are not completed.
Note that this table is implemented using different DDB utilities than our other tables. Instead of using the DDBHelper this class interacts with the boto3 DynamoDB table resource directly. The intent was to provide more robust encapsulation for behaviors like conditional updates and table scans. Generalizing those operations to a helper class risks unexpected side effects if those utilities are updated when tuning future tables. Each row is implemented as a dataclass (ImageRequestStatusRecord) but the reusable logic of converting dataclasses to/from dictionaries that can be used as DynamoDB Items is injected using a new Mixin, DataclassDDBMixin. This makes it easy to reuse truly generic logic (e.g. converting numeric values to Decimals to ensure DDB compatibility) with very little overhead.
Checklist
Before you submit a pull request, please make sure you have the following:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.