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

Avoid checking for alloc_specs if it is not defined #202

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

al-rigazzi
Copy link
Collaborator

This PR mitigates issues when a Slurm or PBS system is set up so that not all environment variables are populated.
If the alloc_specs fixture is not defined (see commit 4bd5e23), the functions using it won't be testsd.

@al-rigazzi al-rigazzi requested review from ashao, ben-albrecht and mellis13 and removed request for mellis13 and ashao June 3, 2022 16:11
Copy link
Contributor

@ben-albrecht ben-albrecht left a comment

Choose a reason for hiding this comment

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

Looks good, barring a request for comment inline

@@ -8,6 +8,8 @@


Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a short comment here explaining why alloc_specs might not be defined? E.g.,

# Slurm or PBS system can sometimes be set up such that not all 
#   environment variables associated with 'alloc_specs' are populated,
#   so 'alloc_specs'-related tests only run when the environment variables are set

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add it to the tests. This feature is still under-documented, anyhow.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #202 (2f56c33) into develop (4bd5e23) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #202      +/-   ##
===========================================
- Coverage    84.12%   84.06%   -0.07%     
===========================================
  Files           58       58              
  Lines         3244     3244              
===========================================
- Hits          2729     2727       -2     
- Misses         515      517       +2     
Impacted Files Coverage Δ
smartsim/_core/launcher/taskManager.py 79.88% <0.00%> (-1.19%) ⬇️

@al-rigazzi al-rigazzi merged commit 4299e7d into CrayLabs:develop Jun 3, 2022
@al-rigazzi al-rigazzi deleted the wlm_helpers_hotfix branch June 3, 2022 20:46
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