fix: optimize database queries

[Bento007]

Reason for Change

• Avoid calling database queries in for loops. This will reduce the amount of time spent making network calls from O(n) to constant time.
• bug(curation api): 504 returned from GET collection and datasets #7190

Changes

• Modify persistance layer calls to avoid database calls within a for loop. The queries are instead batched together into a single call. In the future we may need to split up the calls into batches to avoid waiting for large queries to finish.
• change print statements to logging statements
• add timing metrics to response to diagnose slow database calls.

Testing steps

• All unit tests pass.
• verified timing metrics show up in responses

Note

• These change would benefit from load testing but we currently lack the tooling. This would help determine when or if we should start batch the requests to the database.

[github-actions]

Deployment Summary

• backend_url: https://pr-7191-backend.rdev.single-cell.czi.technology
• backend_de_url: https://pr-7191-backend-de.rdev.single-cell.czi.technology
• backend_wmg_url: https://pr-7191-backend-wmg.rdev.single-cell.czi.technology
• frontend_url: https://pr-7191-frontend.rdev.single-cell.czi.technology
• delete_db_task_definition_arn: arn:aws:ecs:us-west-2:***:task-definition/dp-rdev-pr-7191-deletion:1
• migrate_db_task_definition_arn: arn:aws:ecs:us-west-2:***:task-definition/dp-rdev-pr-7191-migration:1

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.74%. Comparing base (ed8cd5e) to head (9f8934e).

❗ Current head 9f8934e differs from pull request most recent head 82d4b03

Please upload reports for the commit 82d4b03 to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7191   +/-   ##
=======================================
  Coverage   92.73%   92.74%           
=======================================
  Files         191      191           
  Lines       16066    16083   +17     
=======================================
+ Hits        14899    14916   +17     
  Misses       1167     1167           

Flag	Coverage Δ
unittests	92.74% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nayib-jose-gloria]

we should add an empty list check, to avoid opening a session / querying for nothing in case all are DatasetVersion instances

[nayib-jose-gloria]

the tests are passing so maybe i'm misunderstanding, but this query looks like we're getting List[DatasetVersion] rather than List[DatasetVersionId], no? And dataset_version_ids_to_delete_from_s3 expects Ids

[Bento007]

You're totally right, but that is how the original code was written. I change it to query for DatasetVersion.id and it works. Not sure how it managed to work before, because it was returning a list[row] not a list[string].

[nayib-jose-gloria]

test in rdev and ensure no regressions but looks good to me!

[Bento007]

I added some timing metric to the queries and the two slowed queries are in get_all_mapped_collection_versions. Fetching all of the canonical collections take about seconds at worst. Fetching all current collection versions to 20 seconds at worst.

single-cell-data-portal/backend/layers/persistence/persistence.py

Line 475 in 0f84837

with ServerTiming.time("get-cc-all"), log_time_taken("get-cc-all"):

I also did some performance and found the endpoint on average took over 60 seconds to return