Skip to content

Add Two Additional Slurm Array Examples #906

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Premas
Copy link
Contributor

@Premas Premas commented Feb 18, 2025

Pull Request

Overview

This pull request introduces two additional Slurm array examples.

Proposed Changes

  1. Add an example to count the number of words in a file line by line using a Slurm array job.
  2. Add an example to count the number of words from multiple files in a directory in parallel using a Slurm array job.

Related Issues

Fixes #693

@Premas Premas self-assigned this Feb 18, 2025
@Premas Premas added the pr: review PR is ready for review label Feb 18, 2025
Copy link
Contributor

@wwarriner wwarriner left a comment

Choose a reason for hiding this comment

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

This is a great start!

As we spoke about in our meeting, this is a great start! Let's have example 4.1 be reading data from lines in a file. To do this, you can put the contents of files you generate into one line each in the same single file.

Then we can have a 4.2 where you first generate a collection of paths using a glob based on file extension, put that into a file, one path per line. Then in each job read one path from the file and do something with it. Then 4.2 is like 4.1, with one extra preprocessing step. With that said, please still generate a complete example for 4.2.

These two jobs can do the same things (count words), but in two different approaches. One is reading data from a single file, the other is reading data from separate files with a list.

@wwarriner wwarriner added pr: changes requested Review complete, needs changes and removed pr: review PR is ready for review labels Feb 18, 2025
@Premas Premas marked this pull request as ready for review February 28, 2025 17:45
@Premas Premas added pr: review PR is ready for review and removed pr: changes requested Review complete, needs changes labels Feb 28, 2025
@Premas
Copy link
Contributor Author

Premas commented Feb 28, 2025

Thanks for reviewing. I have incorporated the following changes,

Example 1: Each array task independently reads a file line by line and processes the word count for each line.
Example 2: Multiple files are independently processed by array tasks, with each task calculating the word count for a specific file. Also, used find and globbing in example2.

Copy link
Contributor

@wwarriner wwarriner left a comment

Choose a reason for hiding this comment

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

This is great! I have a few thoughts on how to help engage readers.

And I think we'll want to be sure to include what we talked about with changing 4.1 and 4.2 to do the same thing in two different ways. One is reading files from a list in a text file. The other is reading files directly from the filesystem using find.

@wwarriner wwarriner added pr: changes requested Review complete, needs changes and removed pr: review PR is ready for review labels Mar 6, 2025
@Premas Premas added pr: merge PR is ready to merge pr: changes requested Review complete, needs changes and removed pr: changes requested Review complete, needs changes pr: merge PR is ready to merge labels Mar 7, 2025
@Premas Premas added pr: review PR is ready for review and removed pr: changes requested Review complete, needs changes labels Mar 25, 2025
Copy link
Contributor

@wwarriner wwarriner left a comment

Choose a reason for hiding this comment

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

Make sure to test the code if you haven't.

This all looks great. A bit of heading reorganization will have this ready to merge. Good work!

@wwarriner wwarriner added pr: changes requested Review complete, needs changes and removed pr: review PR is ready for review labels Mar 28, 2025
@Premas Premas added pr: review PR is ready for review and removed pr: changes requested Review complete, needs changes labels Mar 31, 2025
Copy link
Contributor

@wwarriner wwarriner left a comment

Choose a reason for hiding this comment

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

Great work Prema! This is looking good.

I think there is some room to clean up and make it look more professional. There are a few key points worth addressing for clarity as well. Good work, if you have questions we can discuss.

@@ -239,7 +239,58 @@ $ sacct -j 27099591

Array jobs are more effective when you have a larger number of similar tasks to be executed simultaneously with varied input data, unlike `srun` parallel jobs which are suitable for running a smaller number of tasks concurrently (e.g. less than 5). Array jobs are easier to manage and monitor multiple tasks through unique identifiers.
Copy link
Contributor

Choose a reason for hiding this comment

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

When I got to the last example I noticed the arrays start at "1". Please start them at "0" instead, to make them more readily compatible with constructs like bash arrays and most programming languages.

You might add a paragraph in the overall introduction to your examples about when to choose zero-indexed and one-indexed --arrays.

In my mind...

Researcher should prefer starting with zero. If their software or programming language is one-indexed (like MATLAB, R, Julia, and Fortran), then it is reasonable to start with one, unless they are using bash arrays. Then they need to consider their options. If needed, researchers can convert between them using bash arithmetic expansion $(($SLURM_ARRAY_TASK_ID + 1)) and $(($SLURM_ARRAY_TASK_ID - 1)) instead of just $SLURM_ARRAY_TASK_ID.

You might also have a subsection "key terminology" that introduces a few things, and provide a reference point. Besides $SLURM_ARRAY_TASK_ID, can you think of anything else that might benefit from being in the "key terminology" section?

All of this is my opinion. Another alternative would be to introduce these concepts in each example, keeping them independent, but that requires more writing and maintenance.

I will leave it to you to decide how to handle this.

@wwarriner wwarriner added pr: changes requested Review complete, needs changes and removed pr: review PR is ready for review labels Apr 15, 2025
Copy link
Contributor

@wwarriner wwarriner left a comment

Choose a reason for hiding this comment

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

Looks great, thank you Prema! Hard work paying off well!

@wwarriner wwarriner added pr: merge PR is ready to merge pr: changes requested Review complete, needs changes and removed pr: changes requested Review complete, needs changes pr: merge PR is ready to merge labels May 19, 2025
Copy link
Contributor

@wwarriner wwarriner left a comment

Choose a reason for hiding this comment

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

Oops I missed that there was still the --array starting at zero, my apologies! Once that's in we can call it finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: changes requested Review complete, needs changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add another example for illustrating Slurm Array job
2 participants