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

Enhanced Signal Management #535

Merged
merged 11 commits into from
Apr 5, 2024

Conversation

MattToast
Copy link
Member

Fixes unfalsifiable test that tests SmartSim's custom SIGINT signal handler. Adds infrastructure to make the test pass again.

@MattToast MattToast added type: refactor Issues focused on refactoring existing code bug: minor A minor bug labels Mar 28, 2024
@MattToast MattToast requested a review from ashao March 28, 2024 18:46
@MattToast MattToast self-assigned this Mar 28, 2024
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.76%. Comparing base (d462fd2) to head (bbd3f5c).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #535      +/-   ##
===========================================
+ Coverage    90.70%   90.76%   +0.06%     
===========================================
  Files           65       65              
  Lines         4498     4528      +30     
===========================================
+ Hits          4080     4110      +30     
  Misses         418      418              
Files Coverage Δ
smartsim/_core/control/controller.py 85.91% <100.00%> (ø)
smartsim/_core/control/jobmanager.py 94.15% <ø> (-0.04%) ⬇️
smartsim/_core/utils/helpers.py 93.70% <100.00%> (+1.74%) ⬆️

Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Looks really good and thanks for fixing this up. It was both a developer and user annoyance. Two comments:

Could you change the name of this PR so that it reflects the effect that these changes have?

I'm not quite seeing the need to have enforce unique callables within the SignalInterceptionStack class itself. I can see that with multiple start calls, you're going to register multiple signals. I'm going back and forth on if it's 100% necessary that that the class enforces uniqueness (and add to some of its complexity) or whether there's a world where it might be useful to have multiple identical callbacks. For example, the current implementation prevents you from popping something multiple times adding it as a callback.

For the one place that this class is used, it's clearly incorrect for the signal handle to be registered more than once. I'm about 60% on the side of that it is the Controller's responsiblity to make sure it doesn't do wonky things when registering a callback and not the class' responsibility. Happy to be argued the other way though

smartsim/_core/utils/helpers.py Outdated Show resolved Hide resolved
smartsim/_core/utils/helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Outdated Show resolved Hide resolved
smartsim/_core/control/controller.py Outdated Show resolved Hide resolved
@MattToast MattToast changed the title Fix Unfalsifiable Signal Test Enhanced Signal Management Apr 4, 2024
Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for making hte changes

@MattToast MattToast merged commit 505de50 into CrayLabs:develop Apr 5, 2024
44 checks passed
@MattToast MattToast deleted the fix-unfalsifiable-test branch June 7, 2024 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: minor A minor bug non-user-facing type: refactor Issues focused on refactoring existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants