Add default_memory_per_machine attribute to Computer. #5260

yakutovicha · 2021-12-08T13:33:08Z

The value units are set to be kB.
0 value means no value has been specified.
The max_memory_kb option has the same effect but is a higher priority

aiida/cmdline/commands/cmd_computer.py

muhrin

We should normalise this to make sure that memory units are always treated as integers (currently there is a mix of float and int).

There is one outstanding question of whether to use MB or kB. Currently the code uses kB for the job options while here we are suggesting using MB. I don't think the loss of granularity is a problem, but the lack of consistency may be confusing. Any thoughts @sphuber ?

aiida/cmdline/commands/cmd_computer.py

.github/config/localhost.yaml

aiida/engine/processes/calcjobs/calcjob.py

aiida/orm/computers.py

aiida/orm/utils/builders/computer.py

aiida/cmdline/params/options/commands/computer.py

sphuber

Thanks @yakutovicha , I have given a first pass. Main thing is that I think mixing kB and MB is going to be confusing, both for the user and in the code.

.github/config/localhost.yaml

aiida/cmdline/commands/cmd_computer.py

aiida/cmdline/params/options/commands/computer.py

aiida/schedulers/datastructures.py

aiida/schedulers/plugins/lsf.py

aiida/schedulers/scheduler.py

aiida/cmdline/params/options/commands/computer.py

aiida/engine/processes/calcjobs/calcjob.py

aiida/orm/computers.py

codecov · 2021-12-09T15:55:56Z

Codecov Report

Merging #5260 (a38d025) into develop (2ee6dd9) will increase coverage by 0.02%.
The diff coverage is 90.33%.

@@             Coverage Diff             @@
##           develop    #5260      +/-   ##
===========================================
+ Coverage    81.45%   81.46%   +0.02%     
===========================================
  Files          530      530              
  Lines        37116    37139      +23     
===========================================
+ Hits         30229    30253      +24     
+ Misses        6887     6886       -1

Flag	Coverage Δ
django	`76.93% <90.33%> (+0.02%)`	⬆️
sqlalchemy	`75.93% <90.33%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/utils/builders/computer.py	`87.50% <83.34%> (-0.54%)`	⬇️
aiida/orm/computers.py	`82.18% <92.86%> (+0.74%)`	⬆️
aiida/cmdline/commands/cmd_computer.py	`81.51% <100.00%> (+0.11%)`	⬆️
aiida/cmdline/params/options/commands/computer.py	`86.67% <100.00%> (+0.46%)`	⬆️
aiida/engine/processes/calcjobs/calcjob.py	`90.18% <100.00%> (+0.37%)`	⬆️
aiida/manage/tests/pytest_fixtures.py	`93.51% <100.00%> (+0.09%)`	⬆️
aiida/schedulers/plugins/direct.py	`66.87% <0.00%> (+0.59%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ee6dd9...a38d025. Read the comment docs.

aiida/cmdline/params/options/commands/computer.py

sphuber

Thanks @yakutovicha . Still are some remaining things to be fixed, but then this should be good to go.

.github/config/localhost.yaml

.github/config/slurm-ssh.yaml

aiida/cmdline/params/options/commands/computer.py

sphuber · 2021-12-10T11:24:05Z

aiida/orm/computers.py

@@ -264,6 +264,20 @@ def _default_mpiprocs_per_machine_validator(cls, def_cpus_per_machine: Optional[
                'do not want to provide a default value.'
            )

+    @classmethod
+    def _default_memory_per_machine_validator(cls, def_memory_per_machine: Optional[int]) -> None:


Don't think you are actually using this. Did you forget to add this to Computer.validate?

This comment still stands correct? This is not being called in the current code.

I think it was correct, but now I call it in set_default_memory_per_machine as you suggest below. I also renamed it to default_memory_per_machine_validator as, presumably, it will be called outside of the class. In case we want to do the validation elsewhere.

aiida/orm/computers.py

sphuber · 2021-12-10T11:30:09Z

aiida/orm/computers.py

+        """
+        if def_memory_per_machine is None:
+            self.delete_property('default_memory_per_machine', raise_exception=False)
+        elif not isinstance(def_memory_per_machine, int):


Instead of doing the validation here again, just call _default_memory_per_machine_validator. This way you don't run the risk that the validation is inconsistent in different places (as is the case now: here you don't check that the int is positive non-zero). If you want to keep the TypeError in case it is not a valid positive integer (which I agree is better than ValidationError) you simply catch the ValidationError and reraise as TypeError.

I think it would be good to do the validation directly here. The same goes for the other set_ methods. See suggestion #5257.

Yes, I am not saying to not do the validation here, I am just saying to put the validation in one place (let's say a separate method) and have this method and the validate method both call that validation. But then we are sure it is consistent

aiida/orm/utils/builders/computer.py

tests/cmdline/commands/test_computer.py

aiida/cmdline/params/options/commands/computer.py

aiida/orm/computers.py

tests/cmdline/commands/test_computer.py

sphuber · 2021-12-13T19:54:43Z

@yakutovicha I resolved the majority of outstanding conversations except one: the validation method is not called anywhere. Finally, I added three minor new comments: two small fixes to help strings/error messages and a test that should be removed.

sphuber · 2021-12-15T07:24:17Z

@yakutovicha there are still some outstanding things that weren't addressed. For your convenience, I created a commit on top of your branch and pushed it to my fork: sphuber@214cb24

If you agree with the changes, you can simply cherry-pick it and push it to your branch, and then after we update it, I will approve and merge.

- The value is in kB. - `max_memory_kb option has the same effect, but is a higher priority.

for more information, see https://pre-commit.ci

Co-authored-by: Sebastiaan Huber <mail@sphuber.net>

…ne value.

- Call `default_memory_per_machine_validator` in `Computer.validate` - Do not handle `0` in the `ComputerBuilder` - Correct `run_cli_command` usage.

Changes addressed

yakutovicha requested a review from sphuber December 8, 2021 13:54

yakutovicha force-pushed the feat/add-memory-option-to-computer-setup branch 3 times, most recently from 1f4af8d to c8e40ca Compare December 8, 2021 15:39

yakutovicha commented Dec 8, 2021

View reviewed changes

aiida/cmdline/commands/cmd_computer.py Outdated Show resolved Hide resolved

yakutovicha requested a review from muhrin December 9, 2021 08:29

yakutovicha mentioned this pull request Dec 9, 2021

Computer, Code, Calculation handling: suggestions for improvements. #5257

Open

muhrin suggested changes Dec 9, 2021

View reviewed changes

sphuber requested changes Dec 9, 2021

View reviewed changes

yakutovicha requested review from muhrin and sphuber December 9, 2021 16:51

yakutovicha changed the title ~~Add default_mem_per_machine attribute to Computer.~~ Add default_memory_per_machine attribute to Computer. Dec 9, 2021

muhrin previously requested changes Dec 9, 2021

View reviewed changes

aiida/cmdline/params/options/commands/computer.py Outdated Show resolved Hide resolved

yakutovicha force-pushed the feat/add-memory-option-to-computer-setup branch 2 times, most recently from efb7fed to bc4ffe2 Compare December 10, 2021 11:24

sphuber requested changes Dec 10, 2021

View reviewed changes

yakutovicha force-pushed the feat/add-memory-option-to-computer-setup branch 2 times, most recently from 07f8e58 to 30a6b89 Compare December 13, 2021 15:15

yakutovicha requested a review from sphuber December 13, 2021 15:39

sphuber reviewed Dec 13, 2021

View reviewed changes

aiida/cmdline/params/options/commands/computer.py Outdated Show resolved Hide resolved

sphuber reviewed Dec 13, 2021

View reviewed changes

aiida/orm/computers.py Outdated Show resolved Hide resolved

sphuber reviewed Dec 13, 2021

View reviewed changes

tests/cmdline/commands/test_computer.py Outdated Show resolved Hide resolved

yakutovicha requested a review from sphuber December 14, 2021 15:55

yakutovicha and others added 4 commits December 15, 2021 10:19

Add default_memory_per_machine Computer setting.

5c16050

- The value is in kB. - `max_memory_kb option has the same effect, but is a higher priority.

[pre-commit.ci] auto fixes from pre-commit.com hooks

b0f1359

for more information, see https://pre-commit.ci

Modify the DEFAULT_MEMORY_PER_MACHINE option description

58ac08f

GH workflows, use non-interactive mode for the computer setup

ee49090

yakutovicha and others added 4 commits December 15, 2021 10:19

Apply suggestions from code review

1546ee0

Co-authored-by: Sebastiaan Huber <mail@sphuber.net>

Remove unnecessary test.

f1cd491

Use default_memory_per_machine_validator to validate memory per machi…

30a6170

…ne value.

Last couple of fixes:

a38d025

- Call `default_memory_per_machine_validator` in `Computer.validate` - Do not handle `0` in the `ComputerBuilder` - Correct `run_cli_command` usage.

yakutovicha force-pushed the feat/add-memory-option-to-computer-setup branch from 13f8fd3 to a38d025 Compare December 15, 2021 09:22

sphuber approved these changes Dec 15, 2021

View reviewed changes

sphuber merged commit ff1318b into aiidateam:develop Dec 15, 2021

yakutovicha deleted the feat/add-memory-option-to-computer-setup branch December 15, 2021 10:28

sphuber mentioned this pull request Dec 16, 2021

Use existing properties to set the max memory, don't add them to the resources aiidateam/aiida-hyperqueue#7

Open

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

yakutovicha mentioned this pull request Jul 3, 2023

Should the Computer have a node_max_memory property #3405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default_memory_per_machine attribute to Computer. #5260

Add default_memory_per_machine attribute to Computer. #5260

yakutovicha commented Dec 8, 2021 •

edited

Loading

muhrin left a comment

sphuber left a comment

codecov bot commented Dec 9, 2021 •

edited

Loading

sphuber left a comment

sphuber Dec 10, 2021

sphuber Dec 13, 2021

yakutovicha Dec 14, 2021 •

edited

Loading

sphuber Dec 10, 2021

yakutovicha Dec 10, 2021

sphuber Dec 10, 2021

sphuber commented Dec 13, 2021

sphuber commented Dec 15, 2021

Add default_memory_per_machine attribute to Computer. #5260

Add default_memory_per_machine attribute to Computer. #5260

Conversation

yakutovicha commented Dec 8, 2021 • edited Loading

muhrin left a comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2021 • edited Loading

Codecov Report

sphuber left a comment

Choose a reason for hiding this comment

sphuber Dec 10, 2021

Choose a reason for hiding this comment

sphuber Dec 13, 2021

Choose a reason for hiding this comment

yakutovicha Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

sphuber Dec 10, 2021

Choose a reason for hiding this comment

yakutovicha Dec 10, 2021

Choose a reason for hiding this comment

sphuber Dec 10, 2021

Choose a reason for hiding this comment

sphuber commented Dec 13, 2021

sphuber commented Dec 15, 2021

yakutovicha commented Dec 8, 2021 •

edited

Loading

codecov bot commented Dec 9, 2021 •

edited

Loading

yakutovicha Dec 14, 2021 •

edited

Loading