Delete files associated with anonymous msgs by yensung · Pull Request #258 · allenai/olmo-api

yensung · 2025-03-25T03:48:57Z

Closes: https://github.com/allenai/playground-issues-repo/issues/218

This PR changes a couple of things:

The files associated with anonymous user msgs would be saved under the anonymous/ folder on Google Cloud Storage. The main reason is that Google Cloud Storage allows us to add auto-deletion rules for objects in the storage. I've added a rule to automatically delete objects if they are over 30 days under the anonymous folder so that we don't have to update/maintain the existing script to do that.

The migration flow for anonymous user messages has been updated as below:
a. Moving out files associated with anonymous user msgs from the anonymous/ folder on google cloud storage
b. Set expiration_time to null, update old file_urls with new urls, and set private to false
c. Update creator from anonymous user ID to logged-in auth ID for both Message and Label tables
Fix the delete_multiple_files_by_url function in the GoogleCloudStorage file

Anonymous msg

Mar-24-2025.20-46-14.mp4

Migration

Mar-24-2025.20-46-20.mp4

Deletion

Mar-24-2025.20-46-25.mp4

mtblanton · 2025-03-25T13:40:50Z

src/message/create_message_service.py


-    # We currently want anonymous users' messages to expire after 1 day
-    message_expiration_time = datetime.now(UTC) + timedelta(days=1) if agent.is_anonymous_user else None
+    # We currently want anonymous users' messages to expire after 30 days


Is this true? I haven't heard of the requirements around anonymous messages changing.

Ahhh I thought anonymous msgs have the same time period of the term that logged-in user msgs aren't removable after 30 days. Let me change it back

Ah, the 1 day thing only exists because we need to store things in the DB to show them to our users. If we could show threads without saving to the DB we would've done that instead! 1 day is just easier with what we have now.

mtblanton · 2025-03-25T13:58:01Z

Assuming we're now allowed to keep anonymous messages for 30 days, how do you think this affects future maintenance of this application?

If we're allowed to keep anonymous messages forever, how many places do we need to update to make sure they keep working properly?

If the requirements for anonymous messages change, are we able to make that same adjustment in all those places?

Is there a way to have GCS handle deletion that doesn't involve moving things between folders?

yensung · 2025-03-25T16:31:44Z

Assuming we're now allowed to keep anonymous messages for 30 days, how do you think this affects future maintenance of this application?

We just need to update the auto-deleting rule in the GCS bucket.

If we're allowed to keep anonymous messages forever, how many places do we need to update to make sure they keep working properly?

We just need to delete the auto-deleting rule in the GCS bucket. Files related to anonymous messages could still be saved under anonymous/ folder in the GCS bucket. They'll be migrated once users log in in the playground.

If the requirements for anonymous messages change, are we able to make that same adjustment in all those places?

If the change is about the deletion time after X days, we can update the auto-deleting rule in the GCS bucket.
If the change is about not allowing anonymous users anymore, we don't have to worry about it as no files would be saved under anonymous/ folder.

Is there a way to have GCS handle deletion that doesn't involve moving things between folders?

Yes. GCS can auto-delete any objects in the bucket. However, we have to differentiate what files are created by anonymous messages. Having an anonymous/ folder and putting related files under it is the easiest way to differentiate.

mtblanton · 2025-03-25T17:21:07Z

Yes. GCS can auto-delete any objects in the bucket. However, we have to differentiate what files are created by anonymous messages. Having an anonymous/ folder and putting related files under it is the easiest way to differentiate.

@yensung does GCS have custom metadata we can use instead of having a separate folder?

I'm not a fan of having logic in a separate system not maintained in GitHub, but if we can drive the logic from this application I'd be happier with it. If we could set a time to delete a message that GCS uses to delete the message I think that'd work well.

If you're able to get something like that to work, please add some documentation about it in comments or in a readme somewhere.

mtblanton · 2025-03-28T22:40:32Z

src/dao/message.py

+                if (expiration_time is not None):
+                    expiration_time_stmt = "expiration_time = NULL," if type(expiration_time) is int else "expiration_time = COALESCE(%(expiration_time)s),"


Was this causing problems? Why won't just expiration_time=%(expiration_time)s work?

I didn't see the expiration time got reset to NULL when expiration_time is None

mtblanton · 2025-03-28T22:41:24Z

src/dao/message.py

                        "finish_reason": finish_reason,
                        "harmful": harmful,
                        "file_urls": file_urls,
+                        # assign an integer to reset expiration time to Null


I figure this is related to the expiration_time_stmt above?

I just found your PR that updates expiration time for anonymous msgs #255.
I can remove the changes to this function once the PR is merged.

src/dao/message.py

src/message/GoogleCloudStorage.py

mtblanton · 2025-03-28T22:48:06Z

src/message/GoogleCloudStorage.py

+            "duration_ms": (end_ns - start_ns) / 1_000_000,
+        })
+
+    def update_file_custom_time(self, filename: str, new_time: datetime):


I would maybe call this something like "update_file_deletion_time" so it's more clear what this is doing for our usecase?

Was naming custom_time as it aligns with the attribute name. No strong opinion here tho

custom_time makes sense if it were a lower-level function, but I think it's good to be specific in our implementation here!

mtblanton · 2025-03-28T22:48:36Z

src/message/GoogleCloudStorage.py

+            "duration_ms": (end_ns - start_ns) / 1_000_000,
+        })
+
+    def move_file(self, old_name: str, new_name: str):


Do we still need this?

mtblanton · 2025-03-28T22:49:49Z

src/message/GoogleCloudStorage.py

+        current_app.logger.info(
+            f"Migrating {filename} from anonymous to normal in the bucket:{self.bucket.name} on GoogleCloudStorage",
+        )
+        self.update_file_custom_time(filename, GCS_MAX_DATETIME_LIMIT)


It'd be good to have a comment here explaining what the custom time is doing for us

Suggested change

self.update_file_custom_time(filename, GCS_MAX_DATETIME_LIMIT)

# We're using the file's custom time to have GCS automatically delete files

# We're not able to un-set the custom time, so instead we're setting it to the furthest time possible

self.update_file_custom_time(filename, GCS_MAX_DATETIME_LIMIT)

mtblanton · 2025-03-28T22:55:03Z

src/user/user_service.py

+            filename = url.split('/')[-1]
+            whole_name = f"{msg.root}/{filename}"


We should probably use Python's URL parsing here. We can also get the full path from the URL!

Maybe something like this?

Suggested change

filename = url.split('/')[-1]

whole_name = f"{msg.root}/{filename}"

whole_name = urlparse(url).path

The url includes the bucket name. I don't think we want it here..

Ahh, right, because we're doing this within the context of a bucket. This is fine then!

src/user/user_service.py

mtblanton

great work!

yensung added 6 commits March 24, 2025 17:47

Add file renaming function

f12c432

Update finalize function and add get_by_creator

6b65f5c

Make use of storage_client

099af16

Update file storing path for files associated with anonymous messages

02d404a

Fix deletion

0de2cf9

Remove print stmts

b3fac3f

yensung requested review from arnavic and mtblanton March 25, 2025 03:49

mtblanton reviewed Mar 25, 2025

View reviewed changes

yensung added 3 commits March 27, 2025 12:59

use custom_time metadata to differentiate anonymous files on GCP

ba429a7

try with rewrite()

da585d9

Fix custom time

3a59576

mtblanton reviewed Mar 28, 2025

View reviewed changes

src/dao/message.py Show resolved Hide resolved

mtblanton reviewed Mar 28, 2025

View reviewed changes

src/message/GoogleCloudStorage.py Show resolved Hide resolved

mtblanton reviewed Mar 28, 2025

View reviewed changes

src/message/GoogleCloudStorage.py Show resolved Hide resolved

mtblanton reviewed Mar 28, 2025

View reviewed changes

src/user/user_service.py Outdated Show resolved Hide resolved

yensung added 2 commits March 31, 2025 10:43

Feedback

e45b572

Clean up

1781e34

mtblanton approved these changes Mar 31, 2025

View reviewed changes

yensung merged commit b4dd4f1 into main Mar 31, 2025
3 checks passed

yensung deleted the 218DeleteAssociatedAnonymousFiles branch March 31, 2025 20:41

mtblanton pushed a commit that referenced this pull request Jun 13, 2025

Delete files associated with anonymous msgs (#258)

977f8ca

		if (expiration_time is not None):
		expiration_time_stmt = "expiration_time = NULL," if type(expiration_time) is int else "expiration_time = COALESCE(%(expiration_time)s),"

		filename = url.split('/')[-1]
		whole_name = f"{msg.root}/{filename}"

	filename = url.split('/')[-1]
	whole_name = f"{msg.root}/{filename}"
	whole_name = urlparse(url).path

Conversation

yensung commented Mar 25, 2025

Uh oh!

mtblanton Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtblanton commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yensung commented Mar 25, 2025

Uh oh!

mtblanton commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtblanton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mtblanton Mar 25, 2025 •

edited

Loading

mtblanton commented Mar 25, 2025 •

edited

Loading

mtblanton commented Mar 25, 2025 •

edited

Loading