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

Add customization for fetching SLURM job id #320

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

BramVanroy
Copy link
Contributor

SLURM job IDs are hard to fetch. The current implementation does not flexibly allow for users to retrieve the right ID based on their printed output (which may be different depending on the implementation). As an example, when submitting an sbatch job, the printed string on our system is

Submitted batch job 9914862 on cluster dodrio

The current default implementation simply splits this string on space and selects the last item. In my case that does not make sense, so it greatly depends on how this info is printed.

I therefore suggest to allow the pipeline constructor to have a job_id_retriever Callable argument. Default behavior is the same as it is now. However, if a user provides a callable, they can choose how the process output is returned.

So in my case, I would add

def job_id_retriever(job_id: str) -> str:
    return re.search(r"Submitted batch job (\d+)", job_id).group(1)

SlurmPipelineExecutor(
    ...,
    job_id_retriever=job_id_retriever,
    ...
)

This funcion will be used to postprocess the retrieved submission text.

As said, the default behavior is the same.

@guipenedo
Copy link
Collaborator

guipenedo commented Jan 10, 2025

Hi, would the recently merged job_id_position not be enough in this case?

@BramVanroy
Copy link
Contributor Author

Hi, would the recently merged job_id_position not be enough in this case?

In my case yes, but I figured - since I had this change done already - I'd do a PR to allow for extended user modification. To catch strings like, e.g. "Submitted job #123" or "Started id-123", or languages that do not use spaces as word delimiters, or whichever odd variation might be out there.

@guipenedo
Copy link
Collaborator

partial seems to be missing an import

@BramVanroy
Copy link
Contributor Author

partial seems to be missing an import

Fixed!

@guipenedo guipenedo merged commit 0c3df50 into huggingface:main Jan 24, 2025
3 of 4 checks passed
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.

2 participants