Skip to content

slurm adapter robustness#431

Merged
jwhite242 merged 12 commits intollnl:developfrom
jwhite242:bugfix/slurm_cmd_fmt
Dec 5, 2023
Merged

slurm adapter robustness#431
jwhite242 merged 12 commits intollnl:developfrom
jwhite242:bugfix/slurm_cmd_fmt

Conversation

@jwhite242
Copy link
Copy Markdown
Collaborator

@jwhite242 jwhite242 commented Nov 30, 2023 •
edited
Loading

Slurm adapter bugfixes

  • Adds prescribed column lists to both squeue and sacct to avoid parsing issues due to optional user environment variables 'SQUEUE_FORMAT' and 'SACCT_FORMAT'.
  • Reduces some unneeded columns in sacct to make it more robust (not all machines have accounts, which broke the parsing)

Test additions

  • Enabled detection of slurm scheduler for live test subset (sched_slurm marker) with auto disablement of marked tests to remain quiet in github ci
  • Unit tests added for slurm adapter
    • check use of squeue and sacct job status checking options in presence of custom squeue/sacct format env vars
    • check functionality of job cancellation
  • Slurm 'hello world' integration test

@jwhite242 jwhite242 added the Bugfix Discussion/Implementation of a solution to a bug (usually a PR) label Nov 30, 2023
@jwhite242 jwhite242 self-assigned this Nov 30, 2023
@FrankD412 FrankD412 self-requested a review December 2, 2023 21:41
FrankD412
FrankD412 reviewed Dec 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@FrankD412 FrankD412 left a comment

Choose a reason for hiding this comment

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

This looks good overall. Aside from some knitpicks I think this makes sense.

Comment thread maestrowf/interfaces/script/slurmscriptadapter.py
Comment thread maestrowf/interfaces/script/slurmscriptadapter.py Outdated
Comment thread tests/conftest.py
Checks if there is a slurm instance to schedule to. NOT IMPLEMENTED YET.
"""
slurm_info_func = 'sinfo'
try:
Copy link
Copy Markdown
Member

@FrankD412 FrankD412 Dec 2, 2023

Choose a reason for hiding this comment

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

I'm almost wondering if the version checking could at some point be moved to the adapters. I think the Flux adapter currently has version check logic for verifying Maestro can use it.

Copy link
Copy Markdown
Collaborator Author

@jwhite242 jwhite242 Dec 4, 2023

Choose a reason for hiding this comment

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

Yeah, capturing that info in the adapters would be quite a useful thing for them to put in to the logs. More on point on this use case of it: do we want to have adapters themselves be able to tell if they have something to schedule to? gets a little muddy with the nested adapters -> flux in slurm/lsf/etc, but could be a potentially interesting use for dependencies too?

@jwhite242
Copy link
Copy Markdown
Collaborator Author

jwhite242 commented Dec 4, 2023

One other thing to stew on that i was playing with during testing this: maybe having an even more verbose log level would be handy to capture things like the raw sacct/squeue/etc outputs so that the logs are closer to fully reproducing errors. 'trace' level? and maybe part of that is even looking through everything and pushing some debug info up into info for things users might be interested in?

@jwhite242 jwhite242 merged commit d99d69a into llnl:develop Dec 5, 2023
@jwhite242 jwhite242 deleted the bugfix/slurm_cmd_fmt branch December 6, 2023 01:28
jwhite242 added a commit that referenced this pull request Dec 12, 2023
@jwhite242 @FrankD412
@dependabot @kinow @doutriaux1 @grosa1 @bgunnar5
1.1.10 Release (#432)
141eaaa 
1.1.10 Release (#432)

* Sync up read the docs config with dev environments using poetry (#399)
* Print usage on command line when no args are provided (#404)
* Add sacct fallback to slurm adapter to improve robustness of job tracking (#405)
* Update Flurm Job State mappings for flux versions >= 0.26 (#407)
* Bump certifi from 2021.10.8 to 2022.12.7 to address security issue (#409)
* Bump cryptography from 37.0.1 to 38.0.3 to address security issue (#410)
* Add missing shbang in unscheduled scripts from lsf adapter (#411)
* Update poetry lockfile to address dependabot flagged security issues (#412)
* Fix for Dockerfile smell DL3006 (#418)
* Port Maestro documentation to mkdocs and expand coverage of features and tutorials (#403)
* Update version info to be driven from pyproject.toml exclusively, and hook up to command line (#419)
* Pin mermaid to < 10.x due to api change (#422)
* Bump lock file certifi from 2022.12.7 to 2023.7.22 to address security issue (#426)
* Refactor flux adapter to avoid using pickle to talk to flux brokers installed in external environments (#415)
   Also adds flux integration tests to exercise against real flux brokers
* Add pager functionality to status command (#420)
* Patch broken flux job cancellation (#428)
* Insulate slurm adapters from user customization of squeue and sacct output formats (#431)
   Also adds live unit and integration tests for slurm adapter

---------

Co-authored-by: Francesco Di Natale <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Bruno P. Kinoshita <[email protected]>
Co-authored-by: Charles Doutriaux <[email protected]>
Co-authored-by: Giovanni Rosa <[email protected]>
Co-authored-by: Brian Gunnarson <[email protected]>
jwhite242 added a commit that referenced this pull request Feb 6, 2024
@jwhite242 @FrankD412
@dependabot @kinow @doutriaux1 @grosa1 @bgunnar5
Merge pull request #437 from LLNL/repo_reset/release/1.1.10
6fa62f1 
1.1.10 Release (#432)

* Sync up read the docs config with dev environments using poetry (#399)
* Print usage on command line when no args are provided (#404)
* Add sacct fallback to slurm adapter to improve robustness of job tracking (#405)
* Update Flurm Job State mappings for flux versions >= 0.26 (#407)
* Bump certifi from 2021.10.8 to 2022.12.7 to address security issue (#409)
* Bump cryptography from 37.0.1 to 38.0.3 to address security issue (#410)
* Add missing shbang in unscheduled scripts from lsf adapter (#411)
* Update poetry lockfile to address dependabot flagged security issues (#412)
* Fix for Dockerfile smell DL3006 (#418)
* Port Maestro documentation to mkdocs and expand coverage of features and tutorials (#403)
* Update version info to be driven from pyproject.toml exclusively, and hook up to command line (#419)
* Pin mermaid to < 10.x due to api change (#422)
* Bump lock file certifi from 2022.12.7 to 2023.7.22 to address security issue (#426)
* Refactor flux adapter to avoid using pickle to talk to flux brokers installed in external environments (#415)
   Also adds flux integration tests to exercise against real flux brokers
* Add pager functionality to status command (#420)
* Patch broken flux job cancellation (#428)
* Insulate slurm adapters from user customization of squeue and sacct output formats (#431)
   Also adds live unit and integration tests for slurm adapter

---------

Co-authored-by: Francesco Di Natale <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Bruno P. Kinoshita <[email protected]>
Co-authored-by: Charles Doutriaux <[email protected]>
Co-authored-by: Giovanni Rosa <[email protected]>
Co-authored-by: Brian Gunnarson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FrankD412 FrankD412 FrankD412 left review comments

Assignees

@jwhite242 jwhite242

Labels

Bugfix Discussion/Implementation of a solution to a bug (usually a PR)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jwhite242 @FrankD412