feat: remove seurat [do not merge yet] #7380

joyceyan · 2024-11-05T21:55:08Z

Reason for Change

https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-data-portal/7366

Changes

removes all code relevant to seurat. for dataset metadata updates, i'm just setting the dataset RDS version to be skipped always.

Testing steps

i updated the unit tests, and uploaded a dataset to the rdev to verify it went through the step function okay. it's available here: https://pr-7380-frontend.rdev.single-cell.czi.technology/collections/06b40cf0-f576-4e91-aeba-4f06925ae36d

Checklist 🛎️

Add product, design, and eng as reviewers for rdev review
For UI changes, add screenshots/videos, so the reviewers know what you expect them to see
For UI changes, add e2e tests to prevent regressions
For UI changes, verify impacted analytics events still work

Notes for Reviewer

🚨 we don't want to deploy this until we're ready to start the schema 5.3.0 migration

github-actions · 2024-11-05T21:55:22Z

Deployment Summary

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

codecov · 2024-11-07T19:26:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (46431eb) to head (be81eea).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7380      +/-   ##
==========================================
+ Coverage   92.90%   92.94%   +0.03%     
==========================================
  Files         194      192       -2     
  Lines       16831    16671     -160     
==========================================
- Hits        15637    15495     -142     
+ Misses       1194     1176      -18

Flag	Coverage Δ
unittests	`92.94% <100.00%> (+0.03%)`	⬆️

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.

joyceyan · 2024-11-07T22:11:19Z

backend/layers/processing/dataset_metadata_update.py

-                f"Cannot find RDS artifact uri for {current_dataset_version_id}, and Conversion Status is not SKIPPED."
-            )
-            self.update_processing_status(new_dataset_version_id, DatasetStatusKey.RDS, DatasetConversionStatus.FAILED)
+        # Mark all RDS conversions as skipped


this seemed like the simplest backwards compatible solution

this approach is consistent since we treat metadata updates as new versions, but we should double-check with Lattice/Brian that they're aware + cool with it

https://czi-sci.slack.com/archives/C07AV4NU9D2/p1731535747847619

nayib-jose-gloria · 2024-11-08T14:17:12Z

i updated the unit tests, but i'm not sure what the best way is to test the terraform changes and verify it's correct.

easiest would be to test an upload in the rdev and make sure it skips seurat and all other actions are behaving normally (can publish, FE looks normal, can update a collection DOI, etc.). Migration is basically just an orchestrator that runs the 'migrate' script then kicks off the regular ingestion pipeline so if regular ingestion works we can expect migration to work as well.

i assume i should also be making a corresponding change to single-cell-infra based on this comment: https://github.com/chanzuckerberg/single-cell-data-portal/blob/main/.happy/terraform/modules/sfn/main.tf#L1 ?

I think this might be outdated? Link is dead and I don't recall this code being duplicated in infra

if we merged this change and deployed it, this would mean we would no longer generate seurat conversions for any new uploads in addition to any schema version migrations we run, right? so we could theoretically launch this change without it being directly tied to a schema release?

Correct. We'll want to remove the seurat-specific code in the validator in the next release, which will require an update here as well since currently the 'validate' command returns a tuple of (is_valid, errors, is_seurat_convertible) and we'll want to change that to just (is_valid, errors). But it won't block launch if we just ignore the third return value for now

nayib-jose-gloria · 2024-11-08T14:29:39Z

backend/layers/processing/dataset_metadata_update.py

@@ -34,7 +33,6 @@
 from backend.layers.thirdparty.uri_provider import UriProvider

 base = importr("base")


r-specific, can remove this!

nayib-jose-gloria · 2024-11-08T14:29:44Z

backend/layers/processing/dataset_metadata_update.py

@@ -9,7 +9,6 @@

 import scanpy
 import tiledb
-from rpy2.robjects import StrVector
 from rpy2.robjects.packages import importr


r-specific, can remove this!

nayib-jose-gloria

https://github.com/chanzuckerberg/single-cell-data-portal/blob/main/backend/layers/processing/schema_migration.py#L235

we have some error handling specific to seurat failures that we can remove for simplicity in the migration code

nayib-jose-gloria

I think it makes sense to set RDS status as skipped for everything for now, since the FE changes are pending and this lets us decouple without blocking. But I think it'd be useful to write a follow-up maintenance issue to deprecate RDS status (while still maintaining the assets in s3/surfacing them in the API)

joyceyan · 2024-11-08T18:07:17Z

I think it makes sense to set RDS status as skipped for everything for now, since the FE changes are pending and this lets us decouple without blocking. But I think it'd be useful to write a follow-up maintenance issue to deprecate RDS status (while still maintaining the assets in s3/surfacing them in the API)

i'm actually inclined to say that we should just keep rds_status indefinitely. there isn't any meaningful benefit to removing it since there's really only one place we're reading it from, and removing it seems like it would just cause more headache than it's worth, especially if it's a required field on the db level.

nayib-jose-gloria · 2024-11-08T20:18:18Z

i'm actually inclined to say that we should just keep rds_status indefinitely. there isn't any meaningful benefit to removing it since there's really only one place we're reading it from, and removing it seems like it would just cause more headache than it's worth, especially if it's a required field on the db level.

the DB stores a dataset status JSON, but doesn't codify what its composed of. So we could get rid of rds status without having to update the DB. Agreed it opens us up to more regression risk to remove it, but it seems like for long-term code readability it would be confusing to keep a status indicator for an artifact type we don't plan on processing anymore. I think its not p0 but worth at least tracking as something to clean-up at some point

nayib-jose-gloria

I'm not exactly sure what the syntax error is, but there seems to be one still in the ingestion SFN. Successful jobs are being marked as failed in AWS due to one.

nayib-jose-gloria

Note: there's some additional seurat related downloads in the processing base image that we should remove as well. but I think that can safely be follow-on work and we can move ahead with this PR for the functional change. thank you!

joyceyan · 2024-11-15T21:55:39Z

@nayib-jose-gloria Good catch, I created a ticket for that here: https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-data-portal/7390

github-actions · 2024-11-30T02:03:48Z

This PR has not seen any activity in the past 2 weeks; if nobody comments or reviews it in the next 3 days, this PR will be closed.

github-actions · 2024-12-03T02:07:57Z

This PR was closed because it has been inactive for 17 days, 3 days since being marked as stale. Please re-open if you still need this to be addressed.

github-actions · 2024-12-19T02:05:27Z

This PR has not seen any activity in the past 2 weeks; if nobody comments or reviews it in the next 3 days, this PR will be closed.

github-actions · 2025-01-04T01:59:56Z

This PR has not seen any activity in the past 2 weeks; if nobody comments or reviews it in the next 3 days, this PR will be closed.

github-actions · 2025-01-07T02:02:15Z

This PR was closed because it has been inactive for 17 days, 3 days since being marked as stale. Please re-open if you still need this to be addressed.

github-actions · 2025-01-26T02:01:12Z

This PR has not seen any activity in the past 2 weeks; if nobody comments or reviews it in the next 3 days, this PR will be closed.

joyceyan · 2025-01-28T19:12:02Z

Closing this PR since the work for this has been folded into the 5.3 feature branch: main...feature/schema-5-3

remove things

a017251

joyceyan force-pushed the joyce/rm-seurat branch from 2058266 to a017251 Compare November 7, 2024 20:12

joyceyan commented Nov 7, 2024

View reviewed changes

nayib-jose-gloria reviewed Nov 8, 2024

View reviewed changes

remove a few unnecessary things

7f23733

joyceyan added 4 commits November 12, 2024 15:21

fix sfn to transition to success

7c401e2

update sfn success handler

3170b74

attempt to fix parameters

fadb929

pass in full event json

3fb2e94

joyceyan changed the title ~~feat: remove seurat [wip]~~ feat: remove seurat [do not merge yet] Nov 13, 2024

This was referenced Nov 13, 2024

Stop running seurat conversions as part of the SFN #7383

Open

Remove the is_seurat_convertible return from cellxgene-schema #7384

Closed

Merge branch 'main' into joyce/rm-seurat

55fe9af

nayib-jose-gloria requested changes Nov 15, 2024

View reviewed changes

joyceyan added 4 commits November 15, 2024 09:46

fix passing in job def name

56b9fd0

add back in batch

05459c0

Merge branch 'main' into joyce/rm-seurat

752b700

Merge branch 'main' into joyce/rm-seurat

b0f03e7

nayib-jose-gloria approved these changes Nov 15, 2024

View reviewed changes

github-actions bot added the stale label Nov 30, 2024

github-actions bot added the autoclosed label Dec 3, 2024

github-actions bot closed this Dec 3, 2024

nayib-jose-gloria reopened this Dec 3, 2024

github-actions bot removed autoclosed stale labels Dec 4, 2024

github-actions bot added the stale label Dec 19, 2024

Merge branch 'main' into joyce/rm-seurat

be81eea

github-actions bot removed the stale label Dec 20, 2024

github-actions bot added the stale label Jan 4, 2025

github-actions bot added the autoclosed label Jan 7, 2025

github-actions bot closed this Jan 7, 2025

nayib-jose-gloria reopened this Jan 10, 2025

github-actions bot removed autoclosed stale labels Jan 11, 2025

github-actions bot added the stale label Jan 26, 2025

Bento007 removed the stale label Jan 28, 2025

nayib-jose-gloria changed the base branch from main to feature/schema-5-3 January 28, 2025 19:10

nayib-jose-gloria changed the base branch from feature/schema-5-3 to main January 28, 2025 19:10

joyceyan closed this Jan 28, 2025

joyceyan reopened this Jan 28, 2025

joyceyan closed this Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove seurat [do not merge yet] #7380

feat: remove seurat [do not merge yet] #7380

joyceyan commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

joyceyan Nov 7, 2024

nayib-jose-gloria Nov 8, 2024

joyceyan Nov 13, 2024

nayib-jose-gloria commented Nov 8, 2024

nayib-jose-gloria Nov 8, 2024

nayib-jose-gloria Nov 8, 2024

nayib-jose-gloria left a comment

nayib-jose-gloria left a comment

joyceyan commented Nov 8, 2024

nayib-jose-gloria commented Nov 8, 2024

nayib-jose-gloria left a comment •

edited

Loading

nayib-jose-gloria left a comment

joyceyan commented Nov 15, 2024

github-actions bot commented Nov 30, 2024

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 19, 2024

github-actions bot commented Jan 4, 2025

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 26, 2025

joyceyan commented Jan 28, 2025

		@@ -34,7 +33,6 @@
		from backend.layers.thirdparty.uri_provider import UriProvider

		base = importr("base")

feat: remove seurat [do not merge yet] #7380

feat: remove seurat [do not merge yet] #7380

Conversation

joyceyan commented Nov 5, 2024 • edited Loading

Reason for Change

Changes

Testing steps

Checklist 🛎️

Notes for Reviewer

github-actions bot commented Nov 5, 2024

Deployment Summary

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

joyceyan Nov 7, 2024

Choose a reason for hiding this comment

nayib-jose-gloria Nov 8, 2024

Choose a reason for hiding this comment

joyceyan Nov 13, 2024

Choose a reason for hiding this comment

nayib-jose-gloria commented Nov 8, 2024

nayib-jose-gloria Nov 8, 2024

Choose a reason for hiding this comment

nayib-jose-gloria Nov 8, 2024

Choose a reason for hiding this comment

nayib-jose-gloria left a comment

Choose a reason for hiding this comment

nayib-jose-gloria left a comment

Choose a reason for hiding this comment

joyceyan commented Nov 8, 2024

nayib-jose-gloria commented Nov 8, 2024

nayib-jose-gloria left a comment • edited Loading

Choose a reason for hiding this comment

nayib-jose-gloria left a comment

Choose a reason for hiding this comment

joyceyan commented Nov 15, 2024

github-actions bot commented Nov 30, 2024

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 19, 2024

github-actions bot commented Jan 4, 2025

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 26, 2025

joyceyan commented Jan 28, 2025

joyceyan commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

nayib-jose-gloria left a comment •

edited

Loading