Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mantis deskewing with multiprocessing and slurm #47

Merged
merged 74 commits into from
Jul 10, 2023
Merged

Mantis deskewing with multiprocessing and slurm #47

merged 74 commits into from
Jul 10, 2023

Conversation

edyoshikun
Copy link
Contributor

@edyoshikun edyoshikun commented Jun 13, 2023 •
edited by talonchandler
Loading

This PR has an example using slurmkit to replace the bash scripts.

2022-06-21 @talonchandler's summary:

  • fixes parallelized deskew and phase deconvolution  #13 (deskew here, phase reconstruction handled by recOrder's CLI)
  • uses multiprocessing to parallelize over T and C via the mantis deskew CLI
  • uses a naive loop to deskew a list of positions with mantis deskew input.zarr/*/*/* (no slurm necessary)
  • includes an example of parallelizing over P using slurm and slurmkit
  • upgrades to python=3.10 to support slurmkit

ieivanov and others added 20 commits May 1, 2023 15:08
@ieivanov
prototype parallel deskew implementation
3062cc4
@edyoshikun
Merge branch 'main' of github.com:czbiohub/mantis into feature/parall…
5606cd2 
…el-deskew
@edyoshikun
added scipy dependency
2b61e4e
@edyoshikun
refactoring deskew to comply to our single position standard.
60651ad 
this is adds the skeleton to multiprocessing per position over T and C.
@edyoshikun
adding napari and iohub dependencies
32bb3bc
@edyoshikun
fixing mistake on changing this file
54e20c4
@talonchandler
revive accidental deletion
ee7c575
@talonchandler
fix input-output mixup bug
57fc884
@talonchandler
cleanup
94b6cb4
@talonchandler
whoops
b10417c
@talonchandler
remove vestigial param
0353f18
@edyoshikun
process positions sequentially and parellize T and C.
e8dcc43
@talonchandler
change reader mode
7bf7cb6
@edyoshikun
have a flag for slurm
c06aa67
@edyoshikun
adding the option 1 : creating an empty array beforehand
e10fc93
@edyoshikun
cleaning up the deskew cli function.
ed8bb2e
@edyoshikun
adding the fixes that ensure that the creation of the arrays follow n…
e5e1fbf 
…atsort

this also speeds the deskewing by adjusting cpus per task,
@edyoshikun
Merge branch 'feature/parallel-deskew' of github.com:czbiohub/mantis …
c8d8fcf 
…into iohub_slurm_option1
@edyoshikun
adding comments and the natsort
74bec5f
@edyoshikun
slurmkit example to consolidate mantis slurm jobs
3f80a26
@edyoshikun edyoshikun mentioned this pull request Jun 13, 2023
Closed
@edyoshikun edyoshikun changed the title Slurmkit for deskewing create-then-fill-slurmkit Jun 13, 2023
@edyoshikun edyoshikun marked this pull request as ready for review June 13, 2023 18:14
@edyoshikun
renamed to slurm deskew,
e02171c 
this function depends soley on deskew_cli(),
removed the click inputs,
cleaned the code.
@edyoshikun
removed the slurm flag and cleaned the code
9c57c94
@edyoshikun
Copy link
Contributor Author

edyoshikun commented Jun 14, 2023 •
edited
Loading

@talonchandler thank you for the call! I think it was super useful because I really like this new implementation where the mantis.deskew.deskew() does not rely on slurm and can be kept separate (removed the --slurm flag), and the slurmkit option -> slurm_deskew() simplifies #46 (3 bash and 2 python scripts).

Notes we should discuss:

  • This file now depends only on the mantis.deskew.py. The imported functions are deskew_cli,get_deskew_params, create_empty_zarrand get_output_paths, but I am not sure if they should live in deskew.pyorutil.py`.
  • Have this file live under analysis/slurm (?)
  • I removed all the CLI from slurm_deskew() there was no need to do that as shown at the end of the file
  • Decide on what uses click.echo,print() or logging.
  • Since I removed the --slurm flag from the mantis.deskew.deskew(), I don't think the batch scripts from create-then-fill #46 will work.

@talonchandler talonchandler mentioned this pull request Jun 22, 2023
Closed
4 tasks
@talonchandler talonchandler changed the title create-then-fill-slurmkit Mantis deskewing with multiprocessing and slurm Jun 22, 2023
@ieivanov
Copy link
Collaborator

Fantastic, thank you!

I understand that the upgrade to python 3.10 is required by the slurmkit dependency. Should we upgrade the python version and test separately, probably together with #39? I'll port over your changed from here and merge that branch first, after testing that the acquisition engine works without problems (I'm hoping to soon write automated tests for that too).

@edyoshikun
Copy link
Contributor Author

I tested this on /hpc/projects/comp.micro/mantis/2023_05_10_PCNA_RAC1/timelapse_2_3/7-slurmkit and worked fine. This LGTM and can get merged once we confirm that python 3.10 does not cause any other issues. Thank you both!

ieivanov
ieivanov reviewed Jun 23, 2023
View reviewed changes
mantis/cli/deskew.py Outdated Show resolved Hide resolved
@ieivanov
Copy link
Collaborator

@edyoshikun could you take a look at 9240cc4?

talonchandler
talonchandler approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@talonchandler talonchandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the os -> pathlib changes!

@ieivanov
Copy link
Collaborator

I think keep_overhang should be part of the deskew settings yaml file. This will simplify the func args quite a bit. I'll make that change

@ieivanov
Copy link
Collaborator

ieivanov commented Jun 23, 2023 •
edited
Loading

I think I'm happy with this PR now. Here is a summary of my last changes. Thanks a lot for your work on this!

Before we merge this I'll finish #39 and them will merge main into this branch

@edyoshikun
Copy link
Contributor Author

edyoshikun commented Jun 23, 2023 •
edited
Loading

This LGTM thank you guys! @ieivanov and I ran the test using the same datset as above /hpc/projects/comp.micro/mantis/2023_05_10_PCNA_RAC1/timelapse_2_3/7-slurmkit and worked well. Happy to get this merged and start working on the next steps for analysis. Looking back at the code history, this last version is clean!

@edyoshikun edyoshikun merged commit 0831536 into main Jul 10, 2023
ieivanov added a commit that referenced this pull request Jul 11, 2023
@ieivanov
Revert "Mantis deskewing with multiprocessing and slurm (#47)"
d62d0ca 
This reverts commit 0831536.
@ieivanov ieivanov mentioned this pull request Jul 11, 2023
Closed
@ieivanov
Copy link
Collaborator

Not quite ready yet, let me first test and merge #39

@ieivanov
Copy link
Collaborator

Actually, it's ok, we can do this out of order too. I haven't seen any issues with python 3.10 in my tests on #39

@ieivanov ieivanov deleted the use_slurmkit branch August 19, 2023 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ieivanov ieivanov ieivanov left review comments

@talonchandler talonchandler talonchandler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

parallelized deskew and phase deconvolution
3 participants
@edyoshikun @talonchandler @ieivanov