Speed up rm bundles #4312

AndrewJGaut · 2022-11-17T21:29:57Z

Reasons for making this change

Speed up rm bundles since it's very slow on the Stanford instance (and is slow in general on any instance that has lots of bundles).

Related issues

#4217

AndrewJGaut · 2022-11-24T05:54:07Z

Speedup on dev for deleting bundles:
Original code: ~2.48 seconds
Updated code: ~0.243 seconds

AndrewJGaut · 2022-11-24T05:57:12Z

Note: the reason it was so slow prior is because it would recompute the user disk in entirety each time a bundle was removed (_get_disk_used(user_id) was called, which searches for all bundles owned by the user). To alleviate this, we now subtract the sum of the sizes of the deleted bundles from the user disk used instead, which is much faster.

…lab-worksheets into fix/4217-speed-up-rm-bundle

percyliang · 2022-11-26T05:03:00Z

codalab/model/bundle_model.py

@@ -2708,6 +2708,14 @@ def increment_user_time_used(self, user_id, amount):
        user_info['time_used'] += amount
        self.update_user_info(user_info)

+    def increment_user_disk_used(self, user_id, amount):


Add type hints.

percyliang · 2022-11-26T05:03:33Z

codalab/rest/bundles.py

+        # OK, now let's add our change.
+        bundle_data_sizes = local.model.get_bundle_metadata(relevant_uuids, 'data_size')
+        local.model.increment_user_disk_used(
+            request.user.user_id, (-1) * sum(map(int, bundle_data_sizes.values()))


Can just do -sum...

Modified this to use something different anyways

percyliang · 2022-11-26T05:04:26Z

codalab/rest/bundles.py

-        local.model.update_user_disk_used(request.user.user_id)
+        # Just decrement the user disk used by the sum of sizes of bundles deleted
+        # OK, now let's add our change.
+        bundle_data_sizes = local.model.get_bundle_metadata(relevant_uuids, 'data_size')


Does this diff produce the same answer as the above? Just wondering if there are subtle differences?

It did not, it turns out, due to the following edge cases:

cl rm -d: Here, the bundle contents are removed but not the bundle metadata. For a given bundle B, we must take care to decrement the user disk quota when cl rm -d B is run, but to not decrement it again when cl rm B is run (since that second cl rm just deletes the metadata and not anything from disk).

Files uploaded with symlinks: These shouldn't affect user disk quota regardless, so when we remove them there should be no decrement to the user disk quota.

The new changes fix this.

percyliang · 2022-11-26T05:04:34Z

codalab/rest/bundles.py

@@ -1314,7 +1315,12 @@ def delete_bundles(uuids, force, recursive, data_only, dry_run):
            local.model.delete_bundles(relevant_uuids)

        # Update user statistics
-        local.model.update_user_disk_used(request.user.user_id)
+        # Just decrement the user disk used by the sum of sizes of bundles deleted
+        # OK, now let's add our change.


Delete this comment line?

percyliang

Ah, very nice! Wonder if we could add a test for this.

percyliang · 2022-11-26T05:07:06Z

codalab/rest/bundles.py

-        local.model.update_user_disk_used(request.user.user_id)
+        # Just decrement the user disk used by the sum of sizes of bundles deleted
+        # OK, now let's add our change.
+        bundle_data_sizes = local.model.get_bundle_metadata(relevant_uuids, 'data_size')


Also check the case where some bundles that are being deleted don't have data (e.g., a bundle which has had cl rm -d ... done on it)

Checked that case with the new tests.

…-only

wwwjn · 2022-11-30T06:47:55Z

codalab/lib/path_util.py

@@ -310,20 +310,24 @@ def remove(path):

        if not FileSystems.exists(path):
            FileSystems.delete([path])
-        return
+        return True  # not sure about this one


return True look good to me

wwwjn · 2022-11-30T06:49:24Z

codalab/lib/path_util.py

    elif os.path.isdir(path):
        try:
            shutil.rmtree(path)
+            return True
        except shutil.Error:


If there is an error, it will have no return value here? Should we use finally here?

Good catch! I added a return False statement at the end of the function to catch any other cases.

AndrewJGaut · 2022-12-04T21:54:08Z

I made some changes to catch edge cases.

Previously, there were no test cases that I saw for verifying that disk quota was incremented and decremented properly upon file upload and deletion. So, I added test cases to check that disk quota is incremented and decremented properly upon:

normal file upload and deletion
normal file upload and deletion with rm -d before deletion with rm
symlinked file upload and deletion

This helps to verify that my rm modification still runs correctly and to verify that our code handles disk quota usage correctly in various cases.

AndrewJGaut · 2022-12-04T23:24:23Z

Now ready for final review @percyliang @wwwjn

Thanks for the reviews!

…ix/4217-speed-up-rm-bundle

…lab-worksheets into fix/4217-speed-up-rm-bundle

added in some timing code for this

f092646

AndrewJGaut changed the title ~~added in some timing code for this~~ Speed up rm bundles Nov 17, 2022

AndrewJGaut added 6 commits November 17, 2022 13:54

add more precise timing code

5847974

minor change

c1347f7

minor changes

ad40d6a

remove debug statement

c4d53e3

fix minor bug

a7c4b23

minor formatting changes

8cadd5a

AndrewJGaut and others added 3 commits November 23, 2022 21:57

Merge branch 'master' into fix/4217-speed-up-rm-bundle

d3b1946

clean up code; delete timing logging statements

10468b0

Merge branch 'fix/4217-speed-up-rm-bundle' of github.com:codalab/coda…

9d37ca7

…lab-worksheets into fix/4217-speed-up-rm-bundle

AndrewJGaut marked this pull request as ready for review November 25, 2022 05:59

AndrewJGaut requested a review from wwwjn November 25, 2022 05:59

percyliang reviewed Nov 26, 2022

View reviewed changes

AndrewJGaut added 5 commits November 28, 2022 11:20

modify method of obtaining solution to account for symlink and --data…

8065d78

…-only

fix earlier issues with code. it works now

fc0dc05

Added test for symlinks

7301ed7

minor changes

5da242e

minor change

c637062

wwwjn reviewed Nov 30, 2022

View reviewed changes

AndrewJGaut added 3 commits December 1, 2022 16:35

Attempt to fix issue with Azure storage deletion

cec09e6

fix merge conflict

849a580

fix merge conflict again

681bd8e

AndrewJGaut added 5 commits December 3, 2022 17:31

Fix issue with Azure storage and bundle location

503b757

Fix another issue with Azure storage

a88bef8

fixing azure error

e82f54f

Finally got the Azure tests to pass!

4f85cd6

minor formatting change

895feca

minor changes

f47f4c9

wwwjn approved these changes Dec 5, 2022

View reviewed changes

AndrewJGaut and others added 17 commits December 5, 2022 09:34

Merge branch 'master' into fix/4217-speed-up-rm-bundle

2ed46bf

Merge branch 'master' of github.com:codalab/codalab-worksheets into f…

f700ac0

…ix/4217-speed-up-rm-bundle

minor modification

efd2956

Merge branch 'fix/4217-speed-up-rm-bundle' of github.com:codalab/coda…

6d1984e

…lab-worksheets into fix/4217-speed-up-rm-bundle

Merge branch 'master' into fix/4217-speed-up-rm-bundle

783b431

minor change

8b64d42

Merge branch 'fix/4217-speed-up-rm-bundle' of github.com:codalab/coda…

f30bf4a

…lab-worksheets into fix/4217-speed-up-rm-bundle

minor change to tests

e24b47b

remove debug statmentes

3cf2bdf

minor modification

96a5852

Change order of tests

7db87ba

add in extra test to find issue

cc8d352

Fix issue with earlier merged PR

00cdaf7

Remove debug statements

81a42f5

minor formatting change

e3ae1cd

Add in file location from Jiani's PR

c217119

Merge branch 'master' into fix/4217-speed-up-rm-bundle

bb30ccb

AndrewJGaut merged commit e6e1e0e into master Dec 7, 2022

AndrewJGaut deleted the fix/4217-speed-up-rm-bundle branch December 7, 2022 20:56

AndrewJGaut mentioned this pull request Dec 7, 2022

removing bundles on codalab.stanford.edu is extremely slow #4217

Closed

AndrewJGaut mentioned this pull request Jan 12, 2023

Disk quota tests #4350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up rm bundles #4312

Speed up rm bundles #4312

AndrewJGaut commented Nov 17, 2022 •

edited

Loading

AndrewJGaut commented Nov 24, 2022

AndrewJGaut commented Nov 24, 2022

percyliang Nov 26, 2022

AndrewJGaut Dec 4, 2022

percyliang Nov 26, 2022

AndrewJGaut Dec 4, 2022

percyliang Nov 26, 2022

AndrewJGaut Dec 4, 2022

percyliang Nov 26, 2022

AndrewJGaut Dec 4, 2022

percyliang left a comment

percyliang Nov 26, 2022

AndrewJGaut Dec 4, 2022

wwwjn Nov 30, 2022

wwwjn Nov 30, 2022

AndrewJGaut Dec 4, 2022

AndrewJGaut commented Dec 4, 2022

AndrewJGaut commented Dec 4, 2022

Speed up rm bundles #4312

Speed up rm bundles #4312

Conversation

AndrewJGaut commented Nov 17, 2022 • edited Loading

Reasons for making this change

Related issues

AndrewJGaut commented Nov 24, 2022

AndrewJGaut commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

percyliang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewJGaut commented Dec 4, 2022

AndrewJGaut commented Dec 4, 2022

AndrewJGaut commented Nov 17, 2022 •

edited

Loading