955 - Download Config Validation for Batch Jobs #1016

avrohomgottlieb · 2024-12-04T22:19:11Z

Issue Number

Closes #955

Purpose/Implementation Notes

This PR removes the submitting of invalid jobs to Batch.

We had originally just submitted all project (or sample) download_config combinations to Batch, and let the invalid ones return early. However, spinning up Batch instances with jobs that will not create a computed file is not an optimal solution (and is not cost effective). As such, this PR tightens up dispatch_to_batch by first validating which download configs produce libraries with projects and samples and only then sending them through.

Changes are also applied to loader::generate_computed_files to maintain consistency among the load_data and dispatch_to_batch+generate_computed_file workflows.

The patches in test_loader had to be slightly refactored in order to accommodate for this change as well. When the dust settled, the constants GENERATED_PROJECT_DOWNLOAD_CONFIGS and GENERATED_SAMPLE_DOWNLOAD_CONFIGS were removed because they were no longer necessary.

Types of changes

What types of changes does your code introduce?

New feature (non-breaking change which adds functionality)

Functional tests

List out the functional tests you've completed to verify your changes work locally.

Checklist

Lint and unit tests pass locally with my changes

Screenshots

N/A

…ad_configs in dispatch_to_batch and loader:::generate_computed_files

…s no longer necessary

…r used GENERATED_{PROJECT, SAMPLE}_DOWNLOAD_CONFIGS constants from common.py

avrohomgottlieb · 2024-12-05T17:28:46Z

Because this PR touches Library::get_project_libraries_from_download_config and Library::get_sample_libraries_from_download_config, I'd like to take the opportunity to move the first method to the Project model and change the name to Project::get_libraries, and the same for the second method, renaming it Sample::get_libraries.

davidsmejia · 2024-12-05T19:07:49Z

Because this PR touches Library::get_project_libraries_from_download_config and Library::get_sample_libraries_from_download_config, I'd like to take the opportunity to move the first method to the Project model and change the name to Project::get_libraries, and the same for the second method, renaming it Sample::get_libraries.

This sounds good to me, we should make download_config optional then and return all libraries if not passed.

davidsmejia · 2024-12-05T18:47:23Z

api/scpca_portal/models/computed_file.py

-        if not libraries.exists():
-            return


I think instead of deleting this we should throw an error here. We should test that we fail this as well.

davidsmejia · 2024-12-05T19:04:19Z

api/scpca_portal/models/project.py

@@ -269,6 +269,13 @@ def get_download_config_file_paths(self, download_config: Dict) -> List[Path]:
            file_path for file_path in data_file_path_objects if file_path.parent.name != "merged"
        ]

+    def get_valid_download_configs(self) -> List[str]:


Let's break this up into two different properties project.valid_download_configs and project.valid_download_config_names

…or config names and the other for configs, in the Project and Sample models

… throw error if no libraries exist

…o {Project, Sample}:::get_libraries

…te empty test_library

…::get_{project, sample}_file

davidsmejia · 2024-12-06T16:04:46Z

api/scpca_portal/models/computed_file.py

        if not libraries.exists():
-            return
+            raise ValueError(
+                "Invalid request: no libraries exist with this sample-download_config combination."


Suggested change

"Invalid request: no libraries exist with this sample-download_config combination."

"Unable to find libraries for download_config."

davidsmejia · 2024-12-06T16:05:11Z

api/scpca_portal/models/computed_file.py

        if not libraries.exists():
-            return
+            raise ValueError(
+                "Invalid request: no libraries exist with this project-download_config combination."


Suggested change

"Invalid request: no libraries exist with this project-download_config combination."

"Unable to find libraries for download_config."

davidsmejia · 2024-12-06T16:06:56Z

api/scpca_portal/models/project.py

+            return self.libraries.all()
+
+        if download_config not in common.PROJECT_DOWNLOAD_CONFIGS.values():
+            raise ValueError("Invalid download configuration passed. Unable to retrieve libraries.")


Suggested change

raise ValueError("Invalid download configuration passed. Unable to retrieve libraries.")

raise ValueError("Invalid download_config passed. Unable to retrieve libraries.")

davidsmejia · 2024-12-06T16:09:03Z

api/scpca_portal/models/sample.py

+            return self.libraries.all()
+
+        if download_config not in common.SAMPLE_DOWNLOAD_CONFIGS.values():
+            raise ValueError("Invalid download configuration passed. Unable to retrieve libraries.")


Suggested change

raise ValueError("Invalid download configuration passed. Unable to retrieve libraries.")

raise ValueError("Invalid download_config passed. Unable to retrieve libraries.")

davidsmejia · 2024-12-06T16:10:35Z

api/scpca_portal/test/models/test_project.py

Let's replace the download config implementations with references to common here.

… in common

avrohomgottlieb added 4 commits December 4, 2024 16:06

add Project::get_valid_download_configs

14c5d47

add Sample::get_valid_download_configs

4189e8b

call Project::get_valid_download_configs and Sample::get_valid_downlo…

071a3af

…ad_configs in dispatch_to_batch and loader:::generate_computed_files

remove library check in ComputedFile::get_project|sample_file as it i…

d23db18

…s no longer necessary

avrohomgottlieb changed the title ~~Avrohom/955 add batch job download config validation~~ 955 - Add batch job download config validation Dec 4, 2024

avrohomgottlieb changed the title ~~955 - Add batch job download config validation~~ 955 - Download Config Validation for Batch Jobs Dec 4, 2024

update patches on computed file tests in test_loader, remove no longe…

d9fc9c7

…r used GENERATED_{PROJECT, SAMPLE}_DOWNLOAD_CONFIGS constants from common.py

vercel bot deployed to Preview December 5, 2024 17:05 View deployment

avrohomgottlieb marked this pull request as ready for review December 5, 2024 17:34

avrohomgottlieb requested a review from davidsmejia as a code owner December 5, 2024 17:34

davidsmejia reviewed Dec 5, 2024

View reviewed changes

avrohomgottlieb added 9 commits December 5, 2024 15:49

convert get_valid_download_configs methods to property methods, one f…

a7e6f1c

…or config names and the other for configs, in the Project and Sample models

integrate new valid_download_config property methods

45dbda6

add existence check back in get_{project, sample}_file, but this time…

fe42d8e

… throw error if no libraries exist

add get_libraries methods to Project and Sample models

c4735c8

update references from Library::get_{project, sample}_libraries_... t…

3ddc42f

…o {Project, Sample}:::get_libraries

move Project::get_library tests from test_library to test_project

c94450b

move Sample::get_library tests from test_library to test_sample, dele…

add9b0b

…te empty test_library

delete Library::get_{project, sample}_libraries_from_download_config

5680e77

add test checking for throwing of no libraries errors in ComputedFile…

dc90349

…::get_{project, sample}_file

vercel bot deployed to Preview December 6, 2024 15:00 View deployment

update test class names in test_project and test_sample

faf9636

vercel bot deployed to Preview December 6, 2024 15:07 View deployment

davidsmejia approved these changes Dec 6, 2024

View reviewed changes

avrohomgottlieb added 2 commits December 6, 2024 12:25

update error messages

a8d0b0a

replace explicit download_configs with references to download_configs…

224e6cb

… in common

vercel bot deployed to Preview December 6, 2024 17:37 View deployment

avrohomgottlieb merged commit 7368829 into dev Dec 6, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

955 - Download Config Validation for Batch Jobs #1016

955 - Download Config Validation for Batch Jobs #1016

avrohomgottlieb commented Dec 4, 2024 •

edited

Loading

avrohomgottlieb commented Dec 5, 2024 •

edited

Loading

davidsmejia commented Dec 5, 2024

davidsmejia Dec 5, 2024

davidsmejia Dec 5, 2024

davidsmejia Dec 6, 2024

davidsmejia Dec 6, 2024

davidsmejia Dec 6, 2024

davidsmejia Dec 6, 2024

davidsmejia Dec 6, 2024

	"Invalid request: no libraries exist with this sample-download_config combination."
	"Unable to find libraries for download_config."

	raise ValueError("Invalid download configuration passed. Unable to retrieve libraries.")
	raise ValueError("Invalid download_config passed. Unable to retrieve libraries.")

955 - Download Config Validation for Batch Jobs #1016

955 - Download Config Validation for Batch Jobs #1016

Conversation

avrohomgottlieb commented Dec 4, 2024 • edited Loading

Issue Number

Purpose/Implementation Notes

Types of changes

Functional tests

Checklist

Screenshots

avrohomgottlieb commented Dec 5, 2024 • edited Loading

davidsmejia commented Dec 5, 2024

davidsmejia Dec 5, 2024

Choose a reason for hiding this comment

davidsmejia Dec 5, 2024

Choose a reason for hiding this comment

davidsmejia Dec 6, 2024

Choose a reason for hiding this comment

davidsmejia Dec 6, 2024

Choose a reason for hiding this comment

davidsmejia Dec 6, 2024

Choose a reason for hiding this comment

davidsmejia Dec 6, 2024

Choose a reason for hiding this comment

davidsmejia Dec 6, 2024

Choose a reason for hiding this comment

avrohomgottlieb commented Dec 4, 2024 •

edited

Loading

avrohomgottlieb commented Dec 5, 2024 •

edited

Loading