Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Renamed sched.test_cmd to sched.launch and added alias #751

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

debonisd
Copy link
Collaborator

@debonisd debonisd commented Mar 1, 2024

Code review checklist:

  • Code is generally sensical and well commented
  • Variable/function names all telegraph their purpose and contents
  • Functions/classes have useful doc strings
  • Function arguments are typed
  • Added/modified unit tests to cover changes.
  • New features have documentation added to the docs.
  • New features and backwards compatibility breaks are noted in the RELEASE.md

@debonisd debonisd requested a review from Paul-Ferrell March 1, 2024 19:53
@debonisd debonisd linked an issue Mar 1, 2024 that may be closed by this pull request
@@ -42,6 +42,7 @@
'set_status': ['status_set'],
'result': ['results'],
'list_cmd': ['list'],
'launch': ['test_cmd'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are aliases for Pavilion commands, not the sched.test_cmd variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the alias in the for test_cmd->launch. It should probably go in the base class for SchedulerVariables.

@@ -111,7 +111,7 @@ command.
| | | 1.
tasks_total | True | 180 | Total tasks to create, based on number of nodes
| | | actually acquired.
test_cmd | True | srun -N 5 -w no | Construct a cmd to run a process under this
launch | True | srun -N 5 -w no | Construct a cmd to run a process under this
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a (formerly sched.test_cmd) to the docs here.

@@ -155,7 +155,7 @@ def partition(self):

return self._sched_config['partition'] or ''

def _test_cmd(self):
def _launch(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're still missing the alias.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrap the alias as a @dfr_var_method - that way it will work regardless of whether or not the function is deferred in any child class.

debonisd and others added 3 commits March 21, 2024 09:44
added to documentation table that used to be sched.test_cmd
this is the REAL fix to creating an alias for the older way of launching
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soft deprecate sched.test_cmd
2 participants