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

Minor fixes to full_wlm tests #283

Merged
merged 3 commits into from
May 9, 2023

Conversation

al-rigazzi
Copy link
Collaborator

This PR addresses some minor errors encountered on new systems when running make test-full.

@al-rigazzi al-rigazzi requested a review from MattToast May 4, 2023 18:27
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #283 (a558a64) into develop (f0ed512) will decrease coverage by 0.06%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #283      +/-   ##
===========================================
- Coverage    87.64%   87.58%   -0.06%     
===========================================
  Files           60       60              
  Lines         3374     3374              
===========================================
- Hits          2957     2955       -2     
- Misses         417      419       +2     
Impacted Files Coverage Δ
smartsim/_core/config/config.py 98.38% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

LGTM and everything is passing on my end!

I found one little potential anti-pattern in test_slurm_allocation they maybe worth cleaning up, but not strictly necessary to fix here.

I also made some notes about potential features to add to BatchSettings in the future, but both are probably well out-of-scope for this PR, so feel free to ignore them for now!

alloc = slurm.get_allocation(nodes=1, time="00:05:00")
account = wlmutils.get_test_account()
if not account:
account = None
Copy link
Member

@MattToast MattToast May 4, 2023

Choose a reason for hiding this comment

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

This feels a bit like an anti-pattern. Should wlmutils.get_test_account and/or CONFIG.test_account just return None if SMARTSIM_TEST_ACCOUNT is not set, instead of an empty string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How come you always have such good ideas. Indeed, updated.

orc.batch_settings.set_walltime("00:02")
else:
orc.batch_settings.set_walltime("00:02:00")
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR, more-so just a note to myself for a potential addition to BatchSettings class:

This feels like there is probably a way to unify these set_walltime calls so that we do not need to jump based on launcher. Maybe something akin to RunSettings.set_time.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of a sudden, you reminded me that we can actually specify time like that in LSF, because we convert it to hh:mm format on the fly. Removing outdated lines.


test_account = wlmutils.get_test_account()
if test_account:
orc.batch_settings.set_account(test_account)
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR, more-so a note to myself for a potential BatchSettings feature:

Iirc, for SBatchSettings, if set_account receives a false-y value (i.e. empty str or None), a no-op is made.

Is this not the case for all BatchSettings derivatives? Should we consider unifying this behavior for script portability??

Copy link
Collaborator Author

@al-rigazzi al-rigazzi May 5, 2023

Choose a reason for hiding this comment

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

It should be, indeed. I updated LSF, which was the only one with a slightly different behavior (due to the fact that accounts are always needed on LSF).

@al-rigazzi al-rigazzi merged commit 9227e24 into CrayLabs:develop May 9, 2023
@al-rigazzi al-rigazzi deleted the fix-full-wlm-tests branch May 9, 2023 13:18
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