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

System variant: no scheduler #435

Closed
wants to merge 18 commits into from
Closed

Conversation

pearce8
Copy link
Collaborator

@pearce8 pearce8 commented Nov 14, 2024

Enable a way to generate run scripts without scheduler instructions, for interactive runs or composing CI runs.

./bin/benchpark system init --dest=tioga-system tioga rocm=551 compiler=cce ~gtl scheduler=mpi
  • Base class only has the no scheduler instructions option (mpi).
  • The AWS system doesn't need to define a new scheduler; should the scheduler code be removed from it?
  • The AWS system needs a dry run (should be addressed in response to Make AWS dynamic system equivalent to legacy #455)
  • Derived class still specifies the scheduler on the system, which will be used by default (when the scheduler is not specified during system init).
  • Is there a way to clean up the code in the system-specific system.py? values=("flux", "mpi") with "flux" being the default option seems brittle.
  • Is there a way to make the scheduler a binary variant? i.e., scheduler=True uses flux, scheduler=False uses mpi.

@@ -33,8 +33,8 @@ class Tioga(System):

def initialize(self):
super().initialize()

self.scheduler = "flux"
if self.spec.variants["scheduler"][0]=="yes":
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if self.spec.satisfies("schedule=yes"):

@@ -33,8 +33,8 @@ class Tioga(System):

def initialize(self):
super().initialize()

self.scheduler = "flux"
if self.spec.variants["scheduler"][0]=="yes":
Copy link
Collaborator

Choose a reason for hiding this comment

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

variant("scheduler", values=("flux", "mpi"), default="flux", sticky=True, description="...")

@@ -82,7 +90,7 @@ def initialize(self):
self.sys_cores_per_node = None
self.sys_gpus_per_node = None
self.sys_mem_per_node = None
self.scheduler = None
self.scheduler = "mpi"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.scheduler = "mpi"
self.scheduler = self.spec.variants["scheduler"].value

Comment on lines 76 to 81
variant(
"scheduler",
default="yes",
values=("yes", "none"),
description="Use the scheduler on the system, or generate scripts without the scheduler commands",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
variant(
"scheduler",
default="yes",
values=("yes", "none"),
description="Use the scheduler on the system, or generate scripts without the scheduler commands",
)
variant(
"scheduler",
default="mpi",
values=("mpi",),
description="Use the scheduler on the system, or generate scripts without the scheduler commands",
)

Copy link
Collaborator Author

@pearce8 pearce8 Nov 26, 2024

Choose a reason for hiding this comment

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

I reverted the changes from @dyokelson. @becker33 the dry run now shows the error: the value of the variant is 'mpi' when the concretizer expects 'None'. @scheibelp if you are able to help fix this, please do.

@pearce8 pearce8 marked this pull request as draft November 22, 2024 21:28
@pearce8 pearce8 force-pushed the pearce8-variant-noscheduler branch from 3946c3b to d3c1ce3 Compare November 26, 2024 00:52
lib/benchpark/system.py Outdated Show resolved Hide resolved
lib/benchpark/system.py Outdated Show resolved Hide resolved
@scheibelp
Copy link
Collaborator

Now that Tioga has a new variant, the hash location of the Ramble workspace has changed (sorry - #450 ought to help with this)

@pearce8 pearce8 marked this pull request as ready for review November 26, 2024 21:57
@pearce8 pearce8 marked this pull request as draft November 26, 2024 21:58
@pearce8
Copy link
Collaborator Author

pearce8 commented Dec 9, 2024

As per issue #485, we will do this differently.

@pearce8 pearce8 closed this Dec 9, 2024
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