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

Enable specification of target hostname for a dragon task #660

Merged
merged 24 commits into from
Aug 26, 2024

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Aug 8, 2024

Description

This PR adds two features:

  1. Ability to specify hostnames that tasks should run on
  2. Enable tasks colocation

Specifying Hostnames

The existing DragonRunRequest supported the ability to specify a hostname when creating a policy used to run a task. However, the hostnames were not exposed to clients.

This ticket allows clients to pass a list of hosts that will be used in place of the default "first available host" behavior.

Task Colocation

The prior system for finding nodes to execute a task worked worked only with unassigned nodes. Any node assigned a task could not be assigned another task.

This ticket adds a more capable prioritizer class that enables clients using hostnames to colocate tasks. It also retains the capability to return open nodes when no hostname is specified.

@ankona ankona changed the title 740 Enable specification of target hostname for a dragon task Aug 8, 2024
@ankona ankona marked this pull request as ready for review August 8, 2024 18:01
@ankona ankona force-pushed the 740 branch 3 times, most recently from 5163932 to d351bde Compare August 9, 2024 15:46
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 65.60284% with 97 lines in your changes missing coverage. Please review.

Please upload report for BASE (mli-feature@f7ef49b). Learn more about missing BASE report.

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

Impacted file tree graph

@@              Coverage Diff               @@
##             mli-feature     #660   +/-   ##
==============================================
  Coverage               ?   79.26%           
==============================================
  Files                  ?       98           
  Lines                  ?     7914           
  Branches               ?        0           
==============================================
  Hits                   ?     6273           
  Misses                 ?     1641           
  Partials               ?        0           
Files with missing lines Coverage Δ
smartsim/_core/launcher/dragon/dragonLauncher.py 50.00% <100.00%> (ø)
smartsim/_core/launcher/step/dragonStep.py 94.59% <100.00%> (ø)
smartsim/settings/dragonRunSettings.py 94.44% <100.00%> (ø)
smartsim/_core/launcher/dragon/pqueue.py 90.10% <90.10%> (ø)
smartsim/_core/launcher/dragon/dragonBackend.py 1.96% <0.00%> (ø)
---- 🚨 Try these New Features:

Copy link
Contributor

@mellis13 mellis13 left a comment

Choose a reason for hiding this comment

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

Just some initial comments while you are making updates. Love the robust commenting and documentation! It really helped make the review process easier and makes the code more maintainable.

Copy link
Contributor

@AlyssaCote AlyssaCote left a comment

Choose a reason for hiding this comment

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

You resolved all of my previous comments with your new updates! This looks great me, just a few clean up comments.

Copy link
Contributor

@mellis13 mellis13 left a comment

Choose a reason for hiding this comment

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

Thanks for all of the updates. Just a couple of comments but overall love the improvements!

@ankona ankona requested a review from al-rigazzi August 15, 2024 18:02
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

LGTM, this looks awesome! The Backend is slowly but steadily getting much more readable, thanks!

@ankona ankona merged commit ef034d5 into CrayLabs:mli-feature Aug 26, 2024
38 of 42 checks passed
@ankona ankona deleted the 740 branch September 25, 2024 15:48
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.

4 participants