Dataops 467: Removed runfolder directory created in delivery_runfolders_for_project_workflow #57

nkongenelly · 2024-09-16T12:35:05Z

What problems does this PR solve?
The code implemented here fixes the duplication of staging runfolders in the staging service

How has the changes been tested?
test_integration_dds.py integration file has also been updated to reflect the bug before and made to pass with the fix implemented here

Verification protocol: https://snpseq.atlassian.net/wiki/spaces/AR/pages/3639574530/arteria-delivery+v3.2.1

Reasons for careful code review
The code implementation should only affect the /stage/project/runfolders/(.+) endpoint

This PR contains code that could remove data

Aratz

Nice work!

I have one question regarding the fix itself, and some additional comments:

Test TestIntegration.test_can_return_flowcells is failing, can you look into it, and also fix .github/workflows/test.yml so that it runs the tests on pull requests as well? (basically instead of on: [push] is should be on: [push, pull_request])
There are a lot of added/removed blank lines. I suspect this was not meant to be committed. In general, it is good practice to check what changes you're committing (eg with git diff before writing the commit). Could you please clean it up a little bit so that this PR only contains changes related to the double runfolder issue?

delivery/services/delivery_service.py

Aratz · 2024-09-16T15:57:37Z

delivery/handlers/staging_handlers.py

@@ -30,7 +30,6 @@ def _construct_response_from_project_and_status(self, staging_order_projects_and

        return link_results, id_results

-


This line should stay (see https://peps.python.org/pep-0008/#blank-lines)

Aratz · 2024-09-16T15:58:42Z

delivery/services/delivery_service.py

@@ -184,7 +190,6 @@ def deliver_all_runfolders_for_project(self, project_name, mode):
                                                          batch_nbr=batch_nbr)

        self.delivery_sources_repo.add_source(source)
-


This could line could stay too

Aratz · 2024-09-16T15:59:02Z

delivery/services/organise_service.py

@@ -29,7 +29,6 @@ def __init__(self, runfolder_service, file_system_service=FileSystemService()):
        """
        self.runfolder_service = runfolder_service
        self.file_system_service = file_system_service
-


Same here, see PEP8

Aratz · 2024-09-16T16:01:50Z

delivery/services/organise_service.py

@@ -116,6 +115,7 @@ def organise_project(
                    organised_project_runfolder_path,
                    lanes))
        # symlink the project files
+


No line should be added here, the comment above should belong to the same block as the code that follows, see https://peps.python.org/pep-0008/#block-comments

Aratz · 2024-09-16T16:04:01Z

delivery/services/staging_service.py

+                                 execution_result.stdout,
+                                 re.MULTILINE)


This was correctly indented before :( lines 105 and 106 should be aligned with the first parameter of the search function above in this case.

Aratz · 2024-09-16T16:04:28Z

delivery/services/staging_service.py

@@ -149,7 +147,6 @@ def stage_order(self, stage_order):
                                 "external_program_service": self.external_program_service,
                                 "staging_repo": self.staging_repo,
                                 "session_factory": self.session_factory}
-


This line could be kept.

tests/integration_tests/test_integration_dds.py

Aratz · 2024-09-16T16:08:31Z

tests/resources/readme/README.md

Why is this removed?

Aratz · 2024-09-16T16:08:46Z

tests/resources/runfolders/160930_ST-E00216_0111_BH37CWALXX/Projects/ABC_123/test_file

Same question here

nkongenelly · 2024-09-17T11:52:16Z

Thank you so much for reviewing this PR. I have now updated the code with the suggestions you made, apart from the symlink part in delivery_service that I have replied to your comment above.

nkongenelly · 2024-09-23T09:43:24Z

@Aratz , This PR is now ready for re-review.

Aratz

Nice! I think the code looks great now, just have some comments about the tests.

Also, I would really recommend you set up a a linter in your text editor and/or run pycodestyle or any other linter on the changes you made before you push them. As of now, the code in general is very unlinted, so it is hard to enforce that with continuous integration, but it would be great if we could at least make sure that everything we touch is linted :)

tests/integration_tests/test_integration_dds.py

Aratz

Great!

matrulda

Looks great! Just some minor comments about the test.

matrulda · 2024-10-11T08:41:30Z

tests/integration_tests/test_integration_dds.py

+            # Assert the staged folder structure has only one runfolder folder
+            temp_staging_dir = f"/tmp/{staging_order_ids.get('JKL_123')}/JKL_123"
+            for runfolder in os.listdir(temp_staging_dir):
+                self.assertFalse(set(runfolder).issuperset(set(os.listdir(f"{temp_staging_dir}/{runfolder}"))))


Just a question here, is it necessary to check this using sets? Since listdir returns a list, can't you do this:

Suggested change

self.assertFalse(set(runfolder).issuperset(set(os.listdir(f"{temp_staging_dir}/{runfolder}"))))

self.assertFalse(runfolder in os.listdir(f"{temp_staging_dir}/{runfolder}"))

tests/integration_tests/test_integration_dds.py

nkongenelly · 2024-10-11T11:55:52Z

@matrulda , thank you so much for the review. I have now pushed the changes

matrulda · 2024-10-11T12:53:36Z

@nkongenelly Great, looks good 👍

nkongenely and others added 3 commits September 16, 2024 09:52

Testing

8f04e9b

DATAOPS-467:Removed_extra_runfolder

6c1d85c

Removed unwanted code and cleaned u files

5693bf1

nkongenelly requested a review from matrulda September 16, 2024 12:35

Aratz reviewed Sep 16, 2024

View reviewed changes

nkongenelly changed the title ~~Dataops 467~~ Dataops 467: Removed runfolder directory created in delivery_runfolders_for_project_workflow Sep 17, 2024

nkongenelly added 2 commits September 17, 2024 13:39

Code formatting and changes after PR review

6a033a7

Refactored code

b59ee2b

nkongenelly added 4 commits September 19, 2024 15:27

Adding integration tests between organise and staging serveice

4ef7603

Added test to check runfolders are not duplicated

6286d01

Refactored code

b3d437c

Correcting integration tests

0d26c3e

Aratz reviewed Sep 25, 2024

View reviewed changes

tests/integration_tests/test_integration_dds.py Outdated Show resolved Hide resolved

tests/integration_tests/test_integration_dds.py Outdated Show resolved Hide resolved

tests/integration_tests/test_integration_dds.py Show resolved Hide resolved

nkongenelly added 2 commits September 27, 2024 15:17

Testing reformatting changes

91fc8cf

Testing reformatting changes

36614d7

Aratz self-requested a review October 2, 2024 11:47

Aratz approved these changes Oct 2, 2024

View reviewed changes

matrulda approved these changes Oct 11, 2024

View reviewed changes

Reformatting anf changed how to assert a folder is not in directory

c5e980d

nkongenelly merged commit 4e427a3 into arteria-project:master Oct 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dataops 467: Removed runfolder directory created in delivery_runfolders_for_project_workflow #57

Dataops 467: Removed runfolder directory created in delivery_runfolders_for_project_workflow #57

nkongenelly commented Sep 16, 2024 •

edited

Loading

Aratz left a comment

Aratz Sep 16, 2024

Aratz Sep 16, 2024

Aratz Sep 16, 2024

Aratz Sep 16, 2024

Aratz Sep 16, 2024

Aratz Sep 16, 2024

Aratz Sep 16, 2024

Aratz Sep 16, 2024

nkongenelly commented Sep 17, 2024

nkongenelly commented Sep 23, 2024

Aratz left a comment

Aratz left a comment

matrulda left a comment

matrulda Oct 11, 2024

nkongenelly commented Oct 11, 2024

matrulda commented Oct 11, 2024

		@@ -30,7 +30,6 @@ def _construct_response_from_project_and_status(self, staging_order_projects_and

		return link_results, id_results

		@@ -184,7 +190,6 @@ def deliver_all_runfolders_for_project(self, project_name, mode):
		batch_nbr=batch_nbr)

		self.delivery_sources_repo.add_source(source)

	self.assertFalse(set(runfolder).issuperset(set(os.listdir(f"{temp_staging_dir}/{runfolder}"))))
	self.assertFalse(runfolder in os.listdir(f"{temp_staging_dir}/{runfolder}"))

Dataops 467: Removed runfolder directory created in delivery_runfolders_for_project_workflow #57

Dataops 467: Removed runfolder directory created in delivery_runfolders_for_project_workflow #57

Conversation

nkongenelly commented Sep 16, 2024 • edited Loading

Aratz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkongenelly commented Sep 17, 2024

nkongenelly commented Sep 23, 2024

Aratz left a comment

Choose a reason for hiding this comment

Aratz left a comment

Choose a reason for hiding this comment

matrulda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkongenelly commented Oct 11, 2024

matrulda commented Oct 11, 2024

nkongenelly commented Sep 16, 2024 •

edited

Loading