-
Notifications
You must be signed in to change notification settings - Fork 187
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
Not prompt memory set for core.direct #5642
Not prompt memory set for core.direct #5642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @unkcpz . As for the testing, I would add it to tests/cmdline/commands/test_computer.py
. Run a setup command with --scheduler-type=core.direct
and make sure that the warning from the DirectJobResource
is not printed. There might be a more robust way that would monkeypatch either the option or the command to raise when it is called or the option is specified in the command.
aiida/schedulers/datastructures.py
Outdated
@classmethod | ||
@abc.abstractmethod | ||
def accepts_default_memory_per_machine(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that the existing method accepts_default_mpiprocs_per_machine
is abstract, but I think in this case it is better to just implement it on the base class. Currently, the code behaves as is the default is True
, so I would just return that here. Then you can simply update the DirectJobResource
to return False
. This way, we don't risk breaking other JobResource
implementations out there that all of a sudden would no longer be concrete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree, the potential risk you mentioned is true, I found I missing to implement the abstract method to TemplateJobResource
.
If there is a problem in determining the scheduler, return True to | ||
avoid exceptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually doesn't match the behavior. If job_resource_cls
is None
then it returns False
. If the scheduler class cannot be determined, I think it is best to return False
, because in that case the option won't be prompted and the user might just have to configure it manually afterwards. If we return True
and the scheduler doesn't actually support it, then any CalcJobs
that will be run with it, will fail I believe.
So I think the code is correct and should be kept, but the docstring should be updated (see other comments what I suggest).
""" | ||
Return True if the scheduler can accept 'default_memory_per_machine', | ||
False otherwise. | ||
|
||
If there is a problem in determining the scheduler, return True to | ||
avoid exceptions. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | |
Return True if the scheduler can accept 'default_memory_per_machine', | |
False otherwise. | |
If there is a problem in determining the scheduler, return True to | |
avoid exceptions. | |
""" | |
"""Return whether the selected scheduler type accepts `default_memory_per_machine`. | |
:return: `True` if the scheduler type accepts `default_memory_per_machine`, `False` otherwise. If the scheduler class could not be loaded `False` is returned by default. | |
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem dito for the should_call_default_mpiprocs_per_machine
docstring.
f0d33bb
to
117b5b9
Compare
@sphuber thanks! I add the test in |
@unkcpz if you fix the tests and docs, I will give another review |
7b35651
to
c6f5ffb
Compare
thanks @unkcpz |
Thanks @sphuber, I forget to request for your review again 😆 |
The warning
Warning: Physical memory limiting is not supported by the direct scheduler.
pop up if set the computer by interactive mode.The
default_memory_per_machine
should not pop up from computer setup withcore.direct
scheduler.But I am not sure where to add the unit test for this.