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

Save source graphic in monarch-link-app and clear unused entries periodically #927

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

VenissaCarolQuadros
Copy link
Member

@VenissaCarolQuadros VenissaCarolQuadros commented Dec 13, 2024

Changes made to the monarch-link-app are as follows:

  1. The monarch-link-app accepts an optional additional field 'graphicBlob', the source graphic. This is required for a client receiving graphics published by the TAT or the extension to make follow up queries.
  2. A cron job has been included to clear unused entries after 60 minutes. This change is required to avoid storing IDs indefinitely and unnecessarily using too much storage on the server. The flask app has been modified to include a timestamp field to identify last updated time and the script run by the cron job uses this to identify if the entry is actively being used.

This PR involves changes required for Shared-Reality-Lab/IMAGE-browser#391 and moving the monarch-link-app to production

  1. Testing changes to include 'graphicBlob' field:
    Tested with post requests to /create and /update/<id> endpoints with and without "graphicBlob" field:
    The resulting display/<id> with "graphicBlob" and without "graphicBlob" were as follows:
  • With "graphicBlob":
{
  "renderings": [
    {
      "data": {
        "graphic": "Testing data",
        "layer": "None"
      }
    }
  ],
  "graphicBlob": "Testing source graphic"
}
  • Without "graphicBlob":
{
  "renderings": [
    {
      "data": {
        "graphic": "Testing data",
        "layer": "None"
      }
    }
  ]
}

i.e. the results were identical except for the presence of graphicBlob in /display/<id> response when it was present in the entry
2. Testing cron job:
Logs from monarch-link-app:

[2024-12-13 17:51:33 +0000] [15] [DEBUG] POST /create
DEBUG:root:Create request received
DEBUG:root:Created new channel with code 543647
[2024-12-13 17:52:39 +0000] [15] [DEBUG] GET /display/543647
DEBUG:root:Display request received
DEBUG:root:Sending tactile response
[2024-12-13 18:07:26 +0000] [15] [DEBUG] OPTIONS /create
[2024-12-13 18:07:26 +0000] [15] [DEBUG] POST /create
DEBUG:root:Create request received
DEBUG:root:Created new channel with code 156528
[2024-12-13 18:52:55 +0000] [15] [DEBUG] OPTIONS /update/156528
[2024-12-13 18:52:55 +0000] [15] [DEBUG] POST /update/156528
DEBUG:root:Update request received
DEBUG:root:Updated graphic

Logs from cron job:

2024-12-13 19:00:00 DEBUG    Cleared ID value: 543647
2024-12-13 20:00:00 DEBUG    Cleared ID value: 156528

Entry with code 543647 was deleted at 19:00:00 i.e. about an hour after it was created. Entry with code 156528 was deleted at 20:00:00 i.e. roughly an hour after it was last updated although it was created at 18:07:26.


Required Information

  • I referenced the issue addressed in this PR.
  • I described the changes made and how these address the issue.
  • I described how I tested these changes.

Coding/Commit Requirements

  • I followed applicable coding standards where appropriate (e.g., PEP8)
  • I have not committed any models or other large files.

New Component Checklist (mandatory for new microservices)

  • I added an entry to docker-compose.yml and build.yml.
  • I created A CI workflow under .github/workflows.
  • I have created a README.md file that describes what the component does and what it depends on (other microservices, ML models, etc.).

OR

  • I have not added a new component in this PR.

@VenissaCarolQuadros VenissaCarolQuadros self-assigned this Dec 13, 2024
@VenissaCarolQuadros VenissaCarolQuadros marked this pull request as ready for review December 16, 2024 16:41
Copy link
Collaborator

@JRegimbal JRegimbal left a comment

Choose a reason for hiding this comment

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

Looks fine, although minor question on something that isn't a change. Also, is there a reason why you have the cron logs going to a separate file rather than stdout? This just seems like a way to have the image size increase over time and make it harder to spot errors.

@@ -161,9 +175,12 @@ def display(id):
try:
response = Response()
response.mimetype = "application/json"
response.set_data(json.dumps({"renderings": [
response_val = {"renderings": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-blocking, but is there a reason to have this renderings array that will always be one element?

Copy link
Member Author

Choose a reason for hiding this comment

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

It just makes the schema consistent with what is expected from the tactile-svg handlers on the client end.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had forgotten to remove the redirection to the separate file! That was for testing convenience.
I've made a new commit to fix that

Copy link
Collaborator

@JRegimbal JRegimbal 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 if you've tested the change!

@VenissaCarolQuadros VenissaCarolQuadros merged commit 167fae5 into main Dec 18, 2024
2 checks passed
@VenissaCarolQuadros VenissaCarolQuadros deleted the monarch-link-app branch December 18, 2024 18:14
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