Enhanced computational resource widget with resource setup #566

superstar54 · 2023-11-21T15:09:16Z

QeApp allows a plugin to add a new computational code in the submission step. As the number of codes grows, the old Resouces and Parallelization sections are not proper anymore because the users may want to set different resources (e.g., nodes, cpus) for different codes.
There are also such discussions before, e.g., see this comment

This PR creates a new QEappComputationalResourcesWidget from one of the aiidalab-widigets-base. The new widget supports setting the resource for the selected code.

Besides, some settings are special to a code, for example, the parallelization levels (pools, images, nk, etc.) in the pw.x code. Thus, this PR creates a new PWscfWidget for the pw.x, which is inherited from the QEappComputationalResourcesWidget.

Here is the screenshot:

codecov · 2023-11-21T15:38:28Z

Codecov Report

Attention: Patch coverage is 86.61972% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 80.82%. Comparing base (f63da7d) to head (80727e5).
Report is 59 commits behind head on main.

❗ Current head 80727e5 differs from pull request most recent head e81ece7. Consider uploading reports for the commit e81ece7 to get more accurate results

Files	Patch %	Lines
src/aiidalab_qe/common/widgets.py	86.41%	11 Missing ⚠️
src/aiidalab_qe/app/submission/__init__.py	72.41%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
+ Coverage   80.73%   80.82%   +0.09%     
==========================================
  Files          49       48       -1     
  Lines        3415     3468      +53     
==========================================
+ Hits         2757     2803      +46     
- Misses        658      665       +7

Flag	Coverage Δ
python-3.10	`80.82% <86.61%> (+0.09%)`	⬆️
python-3.8	`80.87% <86.61%> (+0.09%)`	⬆️
python-3.9	`80.87% <86.61%> (+0.09%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

superstar54 · 2023-11-21T16:00:11Z

@AndresOrtegaGuerrero @mikibonacci , It would be great if you could test this PR in your plugins.

@mbercx , do you think we still need to add the number of k-pools here? Does the new QE version support setting the parallelization automatically?

…ithub.com/aiidalab/aiidalab-qe into feature/new_computational_resource_widget

AndresOrtegaGuerrero · 2023-11-22T13:25:25Z

src/aiidalab_qe/plugins/pdos/workchain.py

@@ -31,12 +32,35 @@ def check_codes(pw_code, dos_code, projwfc_code):
        )


+def update_resources(builder, codes):


Does this method, now should be included in the external plugins for the get_builder?

AndresOrtegaGuerrero · 2023-11-22T13:38:34Z

@AndresOrtegaGuerrero @mikibonacci , It would be great if you could test this PR in your plugins.

@mbercx , do you think we still need to add the number of k-pools here? Does the new QE version support setting the parallelization automatically?

I think the k-pools should be somehow controlled, for example if you want to run the Berry phase code in pw.x you cant use pools. What it could be nice is to have a dummy run. , or a method that given the structure and the input parameters, it can tell you how many k-point you will have, like that we can provide the user a suggestion for the pools to use

AndresOrtegaGuerrero · 2023-11-22T13:44:41Z

src/aiidalab_qe/app/submission/__init__.py

-                else:
-                    self._update_builder(v, max_mpi_per_pool)
+    def _update_builder(self, builder, codes):
+        """Update the resources and parallelization of the ``relax`` builder."""


This update should also affect the other plugins right ?

If other plugins use the same pw code as the relax workchain, yes.

AndresOrtegaGuerrero · 2023-11-22T13:47:39Z

src/aiidalab_qe/common/widgets.py

+        self.npool.value = 1
+
+
+class PWscfWidget(ComputationalResourcesWidget):


Maybe add docstring in this one

thanks. I added the docstring.

AndresOrtegaGuerrero · 2023-11-22T13:54:30Z

src/aiidalab_qe/plugins/pdos/workchain.py

-    pw_code = codes.get("pw")
-    dos_code = codes.get("dos")
-    projwfc_code = codes.get("projwfc")
+    pw_code = codes.get("pw")["code"]


Would this affect how you call the codes in the workchain of external plugin?

Yes, I have updated the doc.

…wrong widget is used.

mbercx · 2023-11-23T10:46:41Z

do you think we still need to add the number of k-pools here? Does the new QE version support setting the parallelization automatically?

New QE versions (I forgot from which one, but def v7.2) indeed support setting k-pools automatically.

I think the k-pools should be somehow controlled, for example if you want to run the Berry phase code in pw.x you cant use pools.

Hmm, I'm not familiar with this use case, but give running on of these calculations a try without specifying k-points and see if the header mentions k-point parallelization. I hope the automated k-pool configuration won't be used in this case then. ^^

What it could be nice is to have a dummy run. , or a method that given the structure and the input parameters, it can tell you how many k-point you will have, like that we can provide the user a suggestion for the pools to use

I haven't tried this, but figuring out the number of k-points can be a bit tricky since you need to determine the symmetries exactly like Quantum ESPRESSO does. A dummy run is useful in some cases (e.g. we do this to figure out the number of q-points in the PhParallelizeQpointsWorkChain), but I'm not sure it's worth it for pw.x k-points because typically you have so many it doesn't matter that much.

mikibonacci · 2023-11-23T15:32:59Z

src/aiidalab_qe/common/widgets.py

+        """Widget for the selection of compute resources.
+        max_num_nodes: maximum number of nodes allowed.
+        """
+        self.num_nodes = ipw.BoundedIntText(


@superstar54 @AndresOrtegaGuerrero
In principle, same code for different step may require different resources.
In my opinion this can be achieved by defining multiple QEAppComputationalResourcesWidget, e.g. one for relax, one for bands (where we may want to add the -nband...).

So we may have PwRelaxWidget, PwBandsWidget... do you think it is the right way to do it? In this way multiple codes which share the same executable (same aiida node) may be used differently by different plugins. I am thinking about for example a case in which we start from a primitive cell, then we compute something for a big supercell. Using the same resources for the simulations would be not efficient

@superstar54 @mikibonacci , there could be many examples , like in the vibroscopy plugin, the PhononWorkChain can use npools, but the Dielectric cannot , or for example in the case of the one i am developing for spin orbit coupling, i might need more resources than the ones use for PwRelax (with out SOC) , though it might create a lot of instances in the step 3

unkcpz

If you don't override the methods of a class to create a new class, but simply extend it with more methods, please don't use class inherit.
If I understand correctly, there is no modification of ComputationalResourceWidget, but add two more widgets on top of it. So why not using composite widget design? Am I miss something?

unkcpz · 2023-11-29T22:19:09Z

src/aiidalab_qe/common/widgets.py

@@ -17,7 +17,8 @@
 import traitlets
 from aiida.orm import CalcJobNode
 from aiida.orm import Data as orm_Data
-from aiida.orm import load_node
+from aiida.orm import load_code, load_node
+from aiidalab_widgets_base import ComputationalResourcesWidget as AiiDACodeWidget


The name AiiDACodeWidget easily get confused with AiiDACodeSetup in AWB: https://github.com/aiidalab/aiidalab-widgets-base/blob/aba0bdb3dc7a7e1af268145a0fd643194e681523/aiidalab_widgets_base/computational_resources.py#L1086 which is for detailed code setup specificly.

I didn't see the point of renaming the class.

unkcpz · 2023-11-29T22:23:53Z

src/aiidalab_qe/common/widgets.py

+            self.num_cpus,
+        )
+
+    @traitlets.observe("value")


I know this value comes from ComputationalResourceWidget but the interface is not clearly exposed.
Instead of inherit from ComputationalResourceWidget, this case fit the composite object quite well. Please consider using ComputationalResourceWidget as sub-widget in this widget you defined.

AndresOrtegaGuerrero · 2023-12-13T16:06:15Z

do you think we still need to add the number of k-pools here? Does the new QE version support setting the parallelization automatically?

New QE versions (I forgot from which one, but def v7.2) indeed support setting k-pools automatically.

I think the k-pools should be somehow controlled, for example if you want to run the Berry phase code in pw.x you cant use pools.

Hmm, I'm not familiar with this use case, but give running on of these calculations a try without specifying k-points and see if the header mentions k-point parallelization. I hope the automated k-pool configuration won't be used in this case then. ^^

What it could be nice is to have a dummy run. , or a method that given the structure and the input parameters, it can tell you how many k-point you will have, like that we can provide the user a suggestion for the pools to use

I haven't tried this, but figuring out the number of k-points can be a bit tricky since you need to determine the symmetries exactly like Quantum ESPRESSO does. A dummy run is useful in some cases (e.g. we do this to figure out the number of q-points in the PhParallelizeQpointsWorkChain), but I'm not sure it's worth it for pw.x k-points because typically you have so many it doesn't matter that much.

I think the number of k-points ("having so many") can be debatable when using a system with a big unit cell

mbercx · 2023-12-13T16:34:38Z

I think the number of k-points ("having so many") can be debatable when using a system with a big unit cell

Sure, that's true. But anyways QE should now be able to figure out how to parallellize over the k-points itself, so adding a dummy run to any of the aiida-quantumespresso work chains is currently not on the agenda.

AndresOrtegaGuerrero · 2023-12-13T16:47:50Z

I think the number of k-points ("having so many") can be debatable when using a system with a big unit cell

Sure, that's true. But anyways QE should now be able to figure out how to parallellize over the k-points itself, so adding a dummy run to any of the aiida-quantumespresso work chains is currently not on the agenda.

Even for the pools? QE can now do this?

mbercx · 2023-12-13T17:35:28Z

Even for the pools? QE can now do this?

Yup ^^. Give it a try with QE v7.2

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

superstar54 · 2024-04-23T09:03:28Z

Hi @AndresOrtegaGuerrero , I added the support for --ntasks-per-node and --cpus-per-task, please try and review the PR. Here is a demo:

qeapp-computational-resource.mp4

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

AndresOrtegaGuerrero · 2024-04-23T11:50:06Z

src/aiidalab_qe/common/widgets.py

+        self.npool.value = 1
+
+
+class PWscfWidget(QEAppComputationalResourcesWidget):


One question Xing, if i have an external plugin, can i set an independent pw.x for it ? or the pw.x is for all the plugins?

Only one pw.x for all the plugins. We can add plugin code setting panel in the future.

But lets assume the external plugin has,
my_pw_code =QEAppComputationalResourcesWidget(
description="pw.x new",
default_calc_job_plugin="quantumespresso.pw",
)
Will this give an independent pw.x code for the use of that external plugin ?

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

superstar54 · 2024-04-23T13:19:57Z

I only have a small request on the naming convention and probably also add a small test for the widget that when the checkbox ticked you test the k-point override is shown.

Hi @unkcpz , thanks for the review. I changed the name as you suggested and added a test for the new widget.

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

AndresOrtegaGuerrero

LGTM! , just be sure to communicate to the external plugins, so we make the change for the release . Great work !

updated as suggested by the reviewer. And the PR is approved by other.

unkcpz

Thanks @superstar54. I think I am late for the party.

Just add two small comments.

unkcpz · 2024-04-24T18:59:03Z

src/aiidalab_qe/common/widgets.py

+    @property
+    def parameters(self):
+        return self.get_parameters()
+
+    def get_parameters(self):
+        """Return the parameters."""
+        parameters = {
+            "code": self.code_selection.value,
+            "nodes": self.num_nodes.value,
+            "cpus": self.num_cpus.value,
+        }
+        parameters.update(self.resource_detail.parameters)
+        return parameters
+
+    @parameters.setter
+    def parameters(self, parameters):
+        self.set_parameters(parameters)
+
+    def set_parameters(self, parameters):
+        """Set the parameters."""
+        self.code_selection.value = parameters["code"]
+        if "nodes" in parameters:
+            self.num_nodes.value = parameters["nodes"]
+        if "cpus" in parameters:
+            self.num_cpus.value = parameters["cpus"]
+        if "ntasks_per_node" in parameters:
+            self.resource_detail.ntasks_per_node.value = parameters["ntasks_per_node"]
+        if "cpus_per_task" in parameters:
+            self.resource_detail.cpus_per_task.value = parameters["cpus_per_task"]


You either use property getter and setter or use get_ and set_, no?
You can check the list of https://realpython.com/python-getter-setter/#deciding-whether-to-use-getters-and-setters-or-properties-in-python and see which one you gonna to use.

Thanks for your comment. It's a common way to use the parameters.setter interface, and then in another function set_parameters implement the real thing. In this pattern, the derived classes can easily modify or extend the setting logic by overriding a dedicated method, without needing to alter the property's interface. As you can see in the PwCodeResourceSetupWidget class.

unkcpz · 2024-04-24T19:29:59Z

src/aiidalab_qe/plugins/pdos/workchain.py

+    builder.scf.pw.metadata.options.resources = {
+        "num_machines": codes.get("pw")["nodes"],
+        "num_mpiprocs_per_machine": codes.get("pw")["ntasks_per_node"],
+        "num_cores_per_mpiproc": codes.get("pw")["cpus_per_task"],
+    }
+    builder.scf.pw.parallelization = orm.Dict(dict=codes["pw"]["parallelization"])
+    builder.nscf.pw.metadata.options.resources = {
+        "num_machines": codes.get("pw")["nodes"],
+        "num_mpiprocs_per_machine": codes.get("pw")["ntasks_per_node"],
+        "num_cores_per_mpiproc": codes.get("pw")["cpus_per_task"],
+    }
+    builder.nscf.pw.parallelization = orm.Dict(dict=codes["pw"]["parallelization"])
+    builder.dos.metadata.options.resources = {
+        "num_machines": codes.get("dos")["nodes"],
+        "num_mpiprocs_per_machine": codes.get("dos")["ntasks_per_node"],
+        "num_cores_per_mpiproc": codes.get("dos")["cpus_per_task"],
+    }
+    builder.projwfc.metadata.options.resources = {
+        "num_machines": codes.get("projwfc")["nodes"],
+        "num_mpiprocs_per_machine": codes.get("projwfc")["ntasks_per_node"],
+        "num_cores_per_mpiproc": codes.get("projwfc")["cpus_per_task"],


This part seems a bit verbose and codes are repeating, consider to simplify it a bit?

builder.scf.pw.metadata.options.resources five dots attributes call looks frightening and I think it makes plugin developer easy to make mistakes.

For override the resources, maybe there already functions to do this in aiida-quantumespresso? @mbercx Maybe you know it?

Somehow my previous comment on this was eaten by GitHub. I do it again.

Thanks for the suggestion. Indeed, one can use the options to set resources in the get_builder_from_protocol. and I agree that this should be the recommended way. I opened an issue on this.

superstar54 added 4 commits November 21, 2023 14:26

add new ComputationalResourcesWidget with nodes and cpus

9d952b9

use new widget in submission

b7d1752

update test

85d888c

fix test for pdos

4664bcf

superstar54 requested review from mbercx, unkcpz, AndresOrtegaGuerrero and mikibonacci November 21, 2023 15:52

superstar54 mentioned this pull request Nov 21, 2023

Improve ComputationalResourcesWidget aiidalab/aiidalab-widgets-base#542

Open

superstar54 added 2 commits November 22, 2023 13:15

fix code not exist when setting

63a6a35

Merge branch 'feature/new_computational_resource_widget' of https://g…

4edf38b

…ithub.com/aiidalab/aiidalab-qe into feature/new_computational_resource_widget

AndresOrtegaGuerrero reviewed Nov 22, 2023

View reviewed changes

backward compatibility for v2023.11

38f5db2

AndresOrtegaGuerrero reviewed Nov 22, 2023

View reviewed changes

superstar54 added 3 commits November 22, 2023 15:00

change name to QEAppComputationalResourcesWidget, and add blocker if …

4ec0817

…wrong widget is used.

only add blocker for selected codes

97c48ec

update doc, ingore hidden codes

cb1ac30

mikibonacci reviewed Nov 23, 2023

View reviewed changes

unkcpz changed the title ~~Feature/new computational resource widget~~ Enhanced computational resource widget with resource setup Nov 29, 2023

unkcpz requested changes Nov 29, 2023

View reviewed changes

superstar54 and others added 7 commits April 5, 2024 19:10

Merge branch 'main' into feature/new_computational_resource_widget

2dbfc3d

Merge branch 'main' into feature/new_computational_resource_widget

5ee2b24

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

a25abe4

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

delete empty resource.py file

f8d6a85

update XAS plugin

029e957

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

56ab9bb

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

add setup resource detail

ad0c844

superstar54 requested a review from AndresOrtegaGuerrero April 23, 2024 09:03

superstar54 force-pushed the feature/new_computational_resource_widget branch from 4904813 to a387712 Compare April 23, 2024 09:12

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

4a0cb83

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

superstar54 force-pushed the feature/new_computational_resource_widget branch from a387712 to 4a0cb83 Compare April 23, 2024 09:42

AndresOrtegaGuerrero reviewed Apr 23, 2024

View reviewed changes

superstar54 requested a review from AndresOrtegaGuerrero April 23, 2024 12:57

superstar54 and others added 2 commits April 23, 2024 13:15

rename PwCodeResourceSetupWidget, add test

14e6f7c

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

9a70a27

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

superstar54 requested a review from unkcpz April 23, 2024 13:20

superstar54 and others added 3 commits April 23, 2024 16:39

Merge branch 'main' into feature/new_computational_resource_widget

724982b

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

5720107

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

Merge branch 'main' into feature/new_computational_resource_widget

e81ece7

superstar54 added this to the v2024.4.0 milestone Apr 24, 2024

AndresOrtegaGuerrero approved these changes Apr 24, 2024

View reviewed changes

superstar54 merged commit 99c059d into main Apr 24, 2024
18 checks passed

superstar54 deleted the feature/new_computational_resource_widget branch April 24, 2024 18:36

unkcpz reviewed Apr 24, 2024

View reviewed changes

unkcpz mentioned this pull request Apr 29, 2024

Codes are not automatically selected #698

Closed

superstar54 mentioned this pull request May 25, 2024

Remove hard coded _update_builder #486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced computational resource widget with resource setup #566

Enhanced computational resource widget with resource setup #566

superstar54 commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

superstar54 commented Nov 21, 2023

AndresOrtegaGuerrero Nov 22, 2023

AndresOrtegaGuerrero commented Nov 22, 2023

AndresOrtegaGuerrero Nov 22, 2023

superstar54 Nov 22, 2023

AndresOrtegaGuerrero Nov 22, 2023

superstar54 Nov 22, 2023

AndresOrtegaGuerrero Nov 22, 2023

superstar54 Nov 22, 2023

mbercx commented Nov 23, 2023

mikibonacci Nov 23, 2023 •

edited

Loading

AndresOrtegaGuerrero Nov 23, 2023

unkcpz left a comment

unkcpz Nov 29, 2023

unkcpz Nov 29, 2023

AndresOrtegaGuerrero commented Dec 13, 2023

mbercx commented Dec 13, 2023

AndresOrtegaGuerrero commented Dec 13, 2023

mbercx commented Dec 13, 2023

superstar54 commented Apr 23, 2024

AndresOrtegaGuerrero Apr 23, 2024

superstar54 Apr 23, 2024

AndresOrtegaGuerrero Apr 23, 2024

superstar54 Apr 23, 2024

superstar54 commented Apr 23, 2024

AndresOrtegaGuerrero left a comment

unkcpz left a comment

unkcpz Apr 24, 2024

superstar54 Apr 24, 2024

unkcpz Apr 24, 2024

superstar54 Apr 24, 2024

		@@ -31,12 +32,35 @@ def check_codes(pw_code, dos_code, projwfc_code):
		)


		def update_resources(builder, codes):

		self.npool.value = 1


		class PWscfWidget(ComputationalResourcesWidget):

		self.npool.value = 1


		class PWscfWidget(QEAppComputationalResourcesWidget):

Enhanced computational resource widget with resource setup #566

Enhanced computational resource widget with resource setup #566

Conversation

superstar54 commented Nov 21, 2023 • edited Loading

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

superstar54 commented Nov 21, 2023

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbercx commented Nov 23, 2023

mikibonacci Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresOrtegaGuerrero commented Dec 13, 2023

mbercx commented Dec 13, 2023

AndresOrtegaGuerrero commented Dec 13, 2023

mbercx commented Dec 13, 2023

superstar54 commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 commented Apr 23, 2024

AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

mikibonacci Nov 23, 2023 •

edited

Loading