Add Slurm license support in workflow manager - #1665
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Slurm licenses in the Slurm workflow manager. It adds a new slurm_licenses variable, automatically resolves licenses from the configuration based on the application's required licenses if not explicitly provided, and appends the appropriate #SBATCH --licenses directive to the generated sbatch script. Additionally, comprehensive unit tests have been added to verify this behavior. Feedback on the changes suggests safely accessing app_inst.license_names using getattr to prevent potential AttributeError or TypeError if the attribute is undefined or None.
Ramble Performance Test MetricsResults produced with commit: b07493b
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1665 +/- ##
========================================
Coverage 93.35% 93.35%
========================================
Files 368 368
Lines 35423 35423
========================================
Hits 33068 33068
Misses 2355 2355 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I don't have any issues with this, but there is a merge conflict that needs to be resolved. |
The default behavior works by:
1. Check for the presence of matching license in the config
2. If license info is found, assume the app instance requires license, and use the license name to specify `{license_name}:{n_ranks}` for the sbatch license header
3. This can be overridden via the `slurm_licenses` workflow manager variable
This is the "client" side, in practice, this expects the Slurm cluster to configure the license limit properly.
5689a36 to
b07493b
Compare
Ah thanks for the reminder. Resolved the conflict. |
| mutable_mock_apps_repo, make_workspace_from_config | ||
| ): | ||
| test_config = """ | ||
| ramble: |
There was a problem hiding this comment.
nit; Can we replace this with a manage experiments config?
| slurm_licenses = f"{matched_license}:{n_ranks}" | ||
|
|
||
| if slurm_licenses: | ||
| pragmas.append(f"#SBATCH --licenses={slurm_licenses}") |
There was a problem hiding this comment.
What is the behavior if the backend portion of this isn't implemented? Is this just ignored, or do we get an error that the license isn't found?
There was a problem hiding this comment.
We will actually get an error. To skip the error, one needs to specify slurm_licenses: ''
There was a problem hiding this comment.
OK, in that case maybe it would be nice to have a variant (that defaults to false) to enable this behavior? It'd be unfortunate if licensed apps just started to not work because we don't have licensed defined in the slurm cluster.
2 participants
The default behavior works by:
{license_name}:{n_ranks}for the sbatch license headerslurm_licensesworkflow manager variableThis is the "client" side, in practice, this expects the Slurm cluster to configure the license limit properly.