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

prepare_computer: Check whether default_mpiprocs_per_machine is set #79

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

sphuber
Copy link
Owner

@sphuber sphuber commented Feb 20, 2024

Fixes #78

If the localhost is created before prepare_computer is ever called, it is possible that the property default_mpiprocs_per_machine is not set. This would result in the ShellJob validation failing if the scheduler type has a job resource class that is a subclass of aiida.schedulers.datastructures.NodeNumberJobResource.

The prepare_computer function now checks that if the localhost computer already exists that the default_mpiprocs_per_machine property is defined if the scheduler has the relevant job resource class. If that is the case, the default is set to 1.

@sphuber sphuber force-pushed the fix/078/localhost-default-mpiprocs-per-machine branch from f0f086b to f192638 Compare February 20, 2024 13:33
If the `localhost` is created before `prepare_computer` is ever called,
it is possible that the property `default_mpiprocs_per_machine` is not
set. This would result in the `ShellJob` validation failing if the
scheduler type has a job resource class that is a subclass of
`aiida.schedulers.datastructures.NodeNumberJobResource`.

The `prepare_computer` function now checks that if the `localhost`
computer already exists that the `default_mpiprocs_per_machine` property
is defined if the scheduler has the relevant job resource class. If that
is the case, the default is set to 1.
@sphuber sphuber force-pushed the fix/078/localhost-default-mpiprocs-per-machine branch from f192638 to 259c643 Compare February 20, 2024 13:42
@sphuber
Copy link
Owner Author

sphuber commented Feb 20, 2024

@giovannipizzi I think this should have helped with the resource validation issue you encountered

@giovannipizzi
Copy link
Contributor

Thanks - I was more thinking to enforce this at the execution level of the specific ShellJob, more than changing the default - the user might want to run on the same computer, even with localhost, with more MPI processes in the future? is it doable?

@sphuber
Copy link
Owner Author

sphuber commented Feb 20, 2024

the user might want to run on the same computer, even with localhost, with more MPI processes in the future?

It is just the default though, nothing is preventing them from specifying more. If that is what they wanted to do, they would anyway have to explicitly specify it, and so nothing would change in that scenario, whether the computer defines a default or not.

is it doable?

I guess so. Instead of having the check in prepare_computer, I could do a check in launch_shell_job to check that if the computer doesn't define a default, then the metadata.options.resources contains the num_mpiprocs_per_machine key. But the current solution has the benefit that the "problem" is fixed even when directly launching the ShellJob and not use the launch_shell_job wrapper. If we put the solution in the wrapper, then the user would still have to manually specify the resources.

@sphuber sphuber changed the title prepare_computer: Check whether default_mpiprocs_per_machine` is set prepare_computer: Check whether default_mpiprocs_per_machine is set Feb 20, 2024
Copy link
Contributor

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

I see, I think you convinced me :-)

@sphuber sphuber merged commit 97f0b55 into master Feb 23, 2024
11 checks passed
@sphuber sphuber deleted the fix/078/localhost-default-mpiprocs-per-machine branch February 23, 2024 08:11
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.

Set default_mpiprocs_per_machine on localhost computer if it already exists and is not set
2 participants