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.

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

How to use the SLEAP HPC module with SLURM #40

Merged
merged 29 commits into from
Nov 24, 2023
Merged

How to use the SLEAP HPC module with SLURM #40

merged 29 commits into from
Nov 24, 2023

Conversation

niksirbi
Copy link
Member

@niksirbi niksirbi commented Sep 27, 2023

This PR adds two how-to guides:

  1. A very long and (hopefully) comprehensive guide on how to use the HPC SLEAP module for remote training and inference. An earlier version of this guide existed in a separate repo and had been reviewed by @sfmig and @lochhh. I've incorporated much of the feedback, and added more structure to the guide, with dropdowns, troubleshooting sections etc. I've also made sure that it's up to date with the latest version of the SLEAP module.
  2. A "SLURM arguments primer", that explains the most commonly used SLURM directives, and what values they should take. SLURM has its own documentation, but it's quite extensive and it's hard to find things in there (especially for non-technical people). So I thought making a shorter list of the type "the n most useful arguments" would be of value.

Other than that, there are some small bits and pieces, like:

  • adding sphinx-copybutton to enable copy-pasting from our code-blocks
  • Renaming the docs workflow with a shorter name (just "Docs"), because the separate jobs of that workflow already have descriptive names, and the actions on CI end up with very long names with much duplication

I would suggest @lochhh looks at the SLEAP part, because she has seen an earlier version of it already, and @adamltyson reviews the SLURM bit, as it might be useful/relevant for the upcoming HPC course.

The best way to review this is to directly look at the rendered website build. I've temporarily overridden the workflow rules to allow publication from this PR, which means that I have to:

  • Reset the docs workflow to its original state after this is reviewed and before merging

@niksirbi niksirbi changed the title How to use the SLEAP HPC module How to use the SLEAP HPC module with SLURM Nov 22, 2023
@niksirbi niksirbi marked this pull request as ready for review November 22, 2023 12:52
@adamltyson
Copy link
Member

adamltyson commented Nov 22, 2023

Out of interest, why this way, rather than the reviewers building locally

Just my neglect. I started working on this very long ago, when not all of us were super familiar with sphinx local builds and I intended to make life easier for reviewers.

But now that we are all such Sphinx pros, there is no need to do that, and I should stop doing it. I had just forgotten that it was configured "the old way" on this branch, and I only realised after the build went public 😨

@adamltyson
Copy link
Member

@niksirbi the bigger problem is the continued insane GitHub bug that ended up with you updating my comment!?!

@niksirbi
Copy link
Member Author

@niksirbi the bigger problem is the continued insane GitHub bug that ended up with you updating my comment!?!

exactly, it almost happened again, but I realised it in time now

Copy link
Member

@adamltyson adamltyson left a comment

Choose a reason for hiding this comment

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

Very nice. I only skimmed the SLEAP section (as suggested), but everything looks good. Some tiny suggestions.

docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/programming/SLURM-arguments.md Outdated Show resolved Hide resolved
docs/source/programming/SLURM-arguments.md Outdated Show resolved Hide resolved
docs/source/programming/SLURM-arguments.md Outdated Show resolved Hide resolved
docs/source/programming/SLURM-arguments.md Show resolved Hide resolved
docs/source/programming/SSH-SWC-cluster.md Outdated Show resolved Hide resolved
@adamltyson
Copy link
Member

@niksirbi I also don't get why we can edit each others comments anyway. I can use the dropdown to edit your messages using the GitHub UI. Is it because of the permissions we have for the repo?

@niksirbi
Copy link
Member Author

@niksirbi I also don't get why we can edit each others comments anyway. I can use the dropdown to edit your messages using the GitHub UI. Is it because of the permissions we have for the repo?

Maybe because we are all admins. Should I set neuroinformatics-all as maintainers?

Applied Adam's suggestions from code review

Co-authored-by: Adam Tyson <[email protected]>
@adamltyson
Copy link
Member

Maybe because we are all admins. Should I set neuroinformatics-all as maintainers?

yeah that sounds good. Especially as this is the new, larger set of contributors.

@niksirbi
Copy link
Member Author

Maybe because we are all admins. Should I set neuroinformatics-all as maintainers?

yeah that sounds good. Especially as this is the new, larger set of contributors.

Done, do you still have permission to edit my posts (you shouldn't now)?

@adamltyson
Copy link
Member

Done, do you still have permission to edit my posts (you shouldn't now)?

I do, but I'm admin of the whole org, so I guess that's expected (but still weird).

@niksirbi
Copy link
Member Author

Done, do you still have permission to edit my posts (you shouldn't now)?

I do, but I'm admin of the whole org, so I guess that's expected (but still weird).

FYI isaacs/github#266

@adamltyson
Copy link
Member

Crazy. Anyway, yell at me if I accidentality edit any comments and I'll try to be more careful. I really don't think we should be editing what other people say!

Copy link
Contributor

@lochhh lochhh left a comment

Choose a reason for hiding this comment

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

🚀

docs/source/programming/SSH-SWC-cluster.md Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Outdated Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Show resolved Hide resolved
docs/source/data_analysis/HPC-module-SLEAP.md Show resolved Hide resolved
@niksirbi niksirbi merged commit 94fd09b into main Nov 24, 2023
3 checks passed
@niksirbi niksirbi deleted the sleap-module branch November 24, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants