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

feat: add requested jobs table #116

Merged
merged 1 commit into from
Feb 24, 2025
Merged

Conversation

edparris
Copy link
Contributor

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:

  • Code changes are compact and well-structured to facilitate easy review
  • Changes are documented in the README.md and other relevant documentation pages
  • PR title and description accurately reflect the changes and are detailed enough for historical tracking
  • PR contains tests that cover all new code and the code has been manual tested
  • All new dependencies are declared (if any), and no unnecessary libraries are added
  • Performance impacts (if any) of the changes are evaluated and documented
  • Security implications of the changes (if any) are reviewed and addressed
  • I have read the Contributing Guidelines and agree to follow the Code of Conduct

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Collaborator

@drduhe drduhe left a 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:
Copy link
Collaborator

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?

Copy link
Contributor Author

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

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:
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@edparris edparris force-pushed the feature/requested-jobs-table branch from 33fee3c to 9ee13e5 Compare February 17, 2025 19:21
@edparris edparris force-pushed the feature/requested-jobs-table branch from 9ee13e5 to a8e13e1 Compare February 17, 2025 22:45
Copy link
Collaborator

@drduhe drduhe left a comment

Choose a reason for hiding this comment

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

LGTM

@edparris edparris merged commit 9ca6f89 into main Feb 24, 2025
2 checks passed
@edparris edparris deleted the feature/requested-jobs-table branch February 24, 2025 21:24
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.

3 participants