Skip to content

Conversation

@al-rigazzi
Copy link
Collaborator

This PR addresses an inconstent internal host name representation in the Dragon backend.

affinity = dragon_policy.Policy.Affinity.SPECIFIC
gpu_affinity = run_request.policy.gpu_affinity

logger.debug(f"Affinity: {affinity}, {cpu_affinity}, {gpu_affinity}")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to leave this in, can we clarify the message so we don't rely on remembering the order of the debug output:

logger.debug(f"Affinity: {affinity}")
logger.debug(f"CPU affinity: {cpu_affinity}")
logger.debug(f"GPU affinity: {gpu_affinity}")

or do the equivalent in a single line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, let me do this!

Copy link
Contributor

@ankona ankona Jul 18, 2024

Choose a reason for hiding this comment

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

Per meeting w/dragon team, the affinity parameter is also being dropped and is not used. I should removed affinity and leave only cpu/gpu affinity lists behind. I think that will clear up your ordering comment?

derp. i see that there were no in-message labels. agreed w/your assessment on order

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed they are dropping the affinity parameter. We can still specify CPU and GPU affinity at the same time (and probably want to know which is which) right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we'll update it as soon as the new version is out.

Copy link
Contributor

@ankona ankona left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing my bug

@codecov
Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.18%. Comparing base (c0584cc) to head (c6ca057).
Report is 16 commits behind head on develop.

Files with missing lines Patch % Lines
smartsim/_core/launcher/dragon/dragonBackend.py 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #642       +/-   ##
============================================
+ Coverage    65.44%   77.18%   +11.73%     
============================================
  Files           83       83               
  Lines         6283     6284        +1     
============================================
+ Hits          4112     4850      +738     
+ Misses        2171     1434      -737     
Files with missing lines Coverage Δ
smartsim/_core/launcher/dragon/dragonBackend.py 2.20% <0.00%> (-0.01%) ⬇️

... and 34 files with indirect coverage changes

@al-rigazzi al-rigazzi merged commit d7d979e into CrayLabs:develop Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants