♻️ decouple override settings of builder #382

unkcpz · 2023-04-07T17:53:51Z

address #383

settle all comments with XXX notations.
codecov badge.
~~second test for insulator be more specific, and test only some of the parameters.~~
having two versions of node viewer in order to show the old workchain results backward compatibly.
test pass but have an exception same as Restore backwards compatibility for some widgets aiidalab-widgets-base#471 (review). See if can work around it or find out why that is happening (memory leak issue?). The test of py-3.10 hang up, passed but not exit.
The builder_parameters should include information therefore can be passed to set_parameters for loading the process to widget.
~~_update_settings method is added for each setting, but these methods are not called. Thus, when loading a previous calculation, widget values are not restored in the GUI.~~ will be addressed in Using update_settings to recover setting widgets from process #397 or by new plugin design of @superstar54

codecov · 2023-04-07T17:59:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2029267). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #382   +/-   ##
=========================================
  Coverage          ?   50.69%           
=========================================
  Files             ?       16           
  Lines             ?     1803           
  Branches          ?        0           
=========================================
  Hits              ?      914           
  Misses            ?      889           
  Partials          ?        0

Flag	Coverage Δ
python-3.10	`50.69% <0.00%> (?)`
python-3.8	`50.77% <0.00%> (?)`
python-3.9	`50.77% <0.00%> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

unkcpz · 2023-04-14T22:48:16Z

Hi @superstar54 @AndresOrtegaGuerrero, I think this PR is starting shaping. Before go into more deep test, I hope you two can have a look if this make everything much clear.

AndresOrtegaGuerrero · 2023-04-15T09:44:43Z

aiidalab_qe/workflows.py

        self.ctx.current_structure = self.inputs.structure
        self.ctx.current_number_of_bands = None
        self.ctx.scf_parent_folder = None

+        # logic based on the properties input


Maybe add some logic if properties is None, in that scenario is just a single point ?

Yes, if the properties list is an empty list, there is nothing actually running.
I try to move the logic outside of the workflow. In this workflow, the sub-workflows are run as independently as possible.
Not sure this is a good strategy, worth to discuss.

aiidalab_qe/steps.py

superstar54 · 2023-04-18T13:31:25Z

Hi @unkcpz Thanks for refactoring the code. One comment on the builder_parameters. The role of the variable should be

store the parameters for the report (workflow summary).
store the parameters to restore all the widgets settings in the four steps. This is important but has not been implemented yet. Please consider this in the PR.

unkcpz · 2023-04-18T19:38:15Z

@superstar54 thanks!

store the parameters to restore all the widgets settings in the four steps. This is important but has not been implemented yet. Please consider this in the PR.

True, I didn't add it yet. Will add it.

danielhollas · 2023-04-18T21:23:46Z

@unkcpz just FYI in case it's useful, when I was refactoring my app in similar vain, I've chosen to use Dataclasses to hold the combined settings of the various input widgets. I've found it really useful to ensure that all the settings are processed. Of course QeApp is quite a bit more complex than my app, but perhaps it might be useful here as well.

See for example code in this file.

https://github.com/ispg-group/aiidalab-ispg/blob/7bbff87cb1576d43aca8921cfd34825963f3e159/aiidalab_ispg/optimization_steps.py#L38

Would be happy to hear what you think.

mbercx

Quite a big PR, so I didn't look into every line of code in much details, but it all looks great! Much cleaner, great work @unkcpz!

I still left some comments/questions, but will already approve.

aiidalab_qe/workflows/__init__.py

qe.ipynb

aiidalab_qe/app/legacy_report.py

aiidalab_qe/app/steps.py

aiidalab_qe/steps.py

unkcpz · 2023-05-15T07:30:00Z

Hi @superstar54 @AndresOrtegaGuerrero, this PR is ready from my point of view. Can you have another look at it? Thanks!
I'd try to ask you to update your implementations (XPS from Xing and total_charge PR from Andres) based on this PR as a test? It gives us more information on is this refactoring makes its goal to let the new widgets and new properties integration easier.

AndresOrtegaGuerrero · 2023-05-15T12:48:00Z

In the last meeting , we discussed about if we should consider backwards compatibility should be consider, because from now on, more changes should be consider ?

mbercx · 2023-05-15T12:54:15Z

To supplement @AndresOrtegaGuerrero's message: I mainly raised the fact that maintaining backwards compatibility is perhaps an undue burden. I don't think there are that many heavy users yet who would lose any important data with this change. I think our policy until further notice (or until public release, tbd) should be to throw backwards compatibility to the wind so we are not limited in making improvements to the interface, and the code does not become more difficult to maintain.

unkcpz · 2023-05-15T12:59:39Z

No problem, I can remove the legacy_report.py, please thumb up on this comment if you all agree @mbercx @superstar54 @AndresOrtegaGuerrero

superstar54

Hi @unkcpz, Thanks for implementing this. I added a few special suggestions to the review.
Here are some general comments:

_update_settings method is added for each setting, but these methods are not called. Thus, when loading a previous calculation, widget values are not restored in the GUI.
In the test, the app tested as a whole in the submit step (kind of integration test). It's nice if you also add tests for each setting (unit test), e.g. workchain, smearing, especially to override the value and then test it.

superstar54 · 2023-05-15T15:03:59Z

tests/conftest.py

-        pass
-    return sdir
+def generate_upf_data():
+    """Return a `UpfData` instance for the given element a file for which should exist in `tests/fixtures/pseudos`."""


Where is this folder tests/fixtures/pseudos?

It is useless, at least for test here. I copy the fixture from qe plugin and I think it is also not used there. Open a PR at aiidateam/aiida-quantumespresso#933, let's see if it passes all tests.

The fixture is used in aiida-quantumespresso for sure, but not necessary here, so I update the docstring only.

superstar54 · 2023-05-15T15:10:45Z

tests/conftest.py

-    )
-    return url, token
+    with tempfile.TemporaryDirectory() as dirpath:
+        for values in elements.values():


It is not necessary to generate the whole pseudo family. Selecting a few elements is enough for the test.

Since there is no much code for this function and the upf file is actually a fake file of every element, I think it is okay to keep it as it is.

superstar54 · 2023-05-15T15:12:08Z

tests/conftest.py


-    return execute
+@pytest.fixture(scope="session", autouse=True)
+def sssp(aiida_profile, generate_upf_data):


What is this sssp fixture used for? In the builder, we only need to pass the name of the pseudo family, right?

When creating the builder, it calls get_builder_from_protocol from aiida-quantumespresso and it require to having pseudo family set.
But it is true the fixture is not need to be loaded explicitly here since it is already load from conftest.py for the session scope.

superstar54 · 2023-05-15T15:21:12Z

tests/conftest.py

+@pytest.fixture()
+def workchain_settings_generator():
+    """Return a function that generates a workchain settings dictionary."""
+    from aiidalab_qe.app.steps import WorkChainSettings
+
+    def _workchain_settings_generator(**kwargs):
+        workchain_settings = WorkChainSettings()
+        workchain_settings._update_settings(**kwargs)
+        return workchain_settings
+
+    return _workchain_settings_generator
+
+
+@pytest.fixture()
+def smearing_settings_generator():
+    """Return a function that generates a smearing settings dictionary."""
+    from aiidalab_qe.app.steps import SmearingSettings
+
+    def _smearing_settings_generator(**kwargs):
+        smearing_settings = SmearingSettings()
+        smearing_settings._update_settings(**kwargs)
+        return smearing_settings
+
+    return _smearing_settings_generator
+
+
+@pytest.fixture()
+def kpoints_settings_generator():
+    """Return a function that generates a kpoints settings dictionary."""


These three fixtures share the same pattern. Is it possible to generate the settings using one fixture?

It can I assume but will make the code even not easy to understand. It can be a settings_generator with passing workchain, smearing or kpoints_settings with the kargs. Sometimes it is not bad pattern to repeat some code if it makes things clear and easy to implement. I'd argue to keep in this way.

superstar54 · 2023-05-16T03:51:15Z

tests/test_submit_qe_workchain/test_create_builder_default.yml

+        npool: 1
+      parameters:
+        CONTROL:
+          calculation: scf


The calculation should be bands. Please check this file.

This is an issue found with running on aiida-quantumespresso==4.2.0. After discussing with @superstar54 offline, we think bump the aiida-quantumespresso version to 4.3.0 fix this issue and the change can be included in this PR.

aiidalab_qe/app/steps.py

superstar54 · 2023-05-16T04:21:08Z

aiidalab_qe/app/steps.py

+
+            if (
+                self.smearing_settings.override_protocol_smearing.value
+                and self.workchain_settings.electronic_type.value == "metal"


Please remove the condition for metal here. Otherwise, the user can still set the override in the GUI, but will find it not working in the calculation. You can disable the override in the GUI when the structure is a metal.

By the way, the smearing should also work for the insulator, right?

By the way, the smearing should also work for the insulator, right?

No, if select insulator, the occupations is set to fixed, the smearing and degauss are not in the input.

AndresOrtegaGuerrero

Everything looks fine to me!

unkcpz · 2023-05-17T09:29:38Z

Per @superstar54's comment:

_update_settings method is added for each setting, but these methods are not called. Thus, when loading a previous calculation, widget values are not restored in the GUI.

I tried with using builder_parameters to recover the widgets, but it is a bit more complicate than I thought. The problem is the key store in the builder_parameters extra attributes are not same for the key used in setting widgets. There are a further step to convert the paramters from setting widget to builder_parameters, to bring it back I think we need a more systematical way to do it and I prefer to do it in a separate PR. If you agree, I’ll merge it.

Moreover, when I look at the refactored architecture in #383, setting widgets directly from builder_parameters is not ideal since it bring the entanglment between viewer and controller which is we want to avoid from first place. Instead, we need introduce and use the dataclass for all the settings object and having bilateral converters between setting wdigets and builder_parameters and having this dataclass in the middle.

@superstar54 is working on introducing the new "plugin-like"way of adding new properties calculation, and it will be handled in the new design by the get and set methods of the panel.

misc: docstring and authors to AiiDAlab team Test added

Compact tests fix bug Adding back report_legacy for backward compatibility r-marnik

Remove are XXX by fixes or create OP mild dependencies

workaround r-andres bump AWB to released 2.0 (#394) `aiidalab-widgets-base` 2.0 is released, the version is bump here. Jinja2 and importlib are from aiida-quantumespresso, therefore the version is loosely pinned. The pybtex has to be added to work around the installation issue. Move module imports from __init__.py of package to app modules (#393) Remove report legacy support pinning qe plugin so test pass

unkcpz marked this pull request as draft April 7, 2023 17:53

unkcpz changed the base branch from master to refac/test-on-get-parameters-and-builder April 7, 2023 23:23

unkcpz force-pushed the refac/builder-chore-decouple-override-settings branch 4 times, most recently from faacdc0 to f49fd4e Compare April 14, 2023 22:36

unkcpz requested review from superstar54 and AndresOrtegaGuerrero April 14, 2023 22:46

AndresOrtegaGuerrero reviewed Apr 15, 2023

View reviewed changes

superstar54 requested changes Apr 18, 2023

View reviewed changes

aiidalab_qe/steps.py Outdated Show resolved Hide resolved

mbercx self-requested a review April 21, 2023 12:47

unkcpz changed the title ~~0x3 ♻️ decouple override settings of builder~~ ♻️ decouple override settings of builder May 5, 2023

unkcpz mentioned this pull request May 5, 2023

0x2: ♻️ unit test on get parameters and builder #381

Closed

unkcpz force-pushed the refac/builder-chore-decouple-override-settings branch from 7a84df3 to c39eb86 Compare May 5, 2023 15:34

unkcpz changed the base branch from refac/test-on-get-parameters-and-builder to master May 5, 2023 15:34

unkcpz force-pushed the refac/builder-chore-decouple-override-settings branch from c39eb86 to b997299 Compare May 5, 2023 15:38

mbercx approved these changes May 6, 2023

View reviewed changes

AndresOrtegaGuerrero reviewed May 6, 2023

View reviewed changes

aiidalab_qe/steps.py Show resolved Hide resolved

unkcpz marked this pull request as ready for review May 8, 2023 09:50

unkcpz force-pushed the refac/builder-chore-decouple-override-settings branch from d15f8d7 to 17a18b4 Compare May 8, 2023 09:57

unkcpz mentioned this pull request May 8, 2023

0x1: Move QeAppWorkChain into the workflow module #379

Merged

2 tasks

superstar54 requested changes May 16, 2023

View reviewed changes

unkcpz requested review from superstar54 and AndresOrtegaGuerrero May 16, 2023 13:10

AndresOrtegaGuerrero approved these changes May 16, 2023

View reviewed changes

unkcpz mentioned this pull request May 16, 2023

Using update_settings to recover setting widgets from process #397

Closed

unkcpz force-pushed the refac/builder-chore-decouple-override-settings branch from 5094e97 to deda4ba Compare May 17, 2023 10:10

unkcpz added 8 commits May 17, 2023 10:12

New workflow structure

5485dbd

Widgets of step compatible with new workflow

f062d51

compatible node_view, report widget and workflow summary template

2e21a5d

misc: pseudo widget

9942b91

misc: docstring and authors to AiiDAlab team Test added

r-xing

a24b7ea

Compact tests fix bug Adding back report_legacy for backward compatibility r-marnik

Update dependencies

afc150d

Remove are XXX by fixes or create OP mild dependencies

Fix bands-scf issue and bump aiida-quantumespresso version to 4.3.0

9a92cc9

unkcpz force-pushed the refac/builder-chore-decouple-override-settings branch from deda4ba to 9a92cc9 Compare May 17, 2023 10:13

superstar54 approved these changes May 17, 2023

View reviewed changes

unkcpz merged commit 431ab4a into master May 17, 2023
11 checks passed

unkcpz deleted the refac/builder-chore-decouple-override-settings branch May 17, 2023 11:17

This was referenced May 17, 2023

Feature: tot charge #377

Closed

♻️ Refactoring the arch for easily implement new properties and add new features #383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ decouple override settings of builder #382

♻️ decouple override settings of builder #382

unkcpz commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

unkcpz commented Apr 14, 2023

AndresOrtegaGuerrero Apr 15, 2023

unkcpz Apr 17, 2023

superstar54 commented Apr 18, 2023

unkcpz commented Apr 18, 2023

danielhollas commented Apr 18, 2023 •

edited

Loading

mbercx left a comment •

edited

Loading

unkcpz commented May 15, 2023 •

edited

Loading

AndresOrtegaGuerrero commented May 15, 2023

mbercx commented May 15, 2023

unkcpz commented May 15, 2023

superstar54 left a comment

superstar54 May 15, 2023 •

edited

Loading

unkcpz May 16, 2023

unkcpz May 16, 2023

superstar54 May 15, 2023

unkcpz May 16, 2023

superstar54 May 15, 2023

unkcpz May 16, 2023

superstar54 May 15, 2023

unkcpz May 16, 2023

superstar54 May 16, 2023 •

edited

Loading

unkcpz May 16, 2023

superstar54 May 16, 2023

unkcpz May 16, 2023

AndresOrtegaGuerrero left a comment

unkcpz commented May 17, 2023

♻️ decouple override settings of builder #382

♻️ decouple override settings of builder #382

Conversation

unkcpz commented Apr 7, 2023 • edited Loading

codecov bot commented Apr 7, 2023 • edited Loading

Codecov Report

unkcpz commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 commented Apr 18, 2023

unkcpz commented Apr 18, 2023

danielhollas commented Apr 18, 2023 • edited Loading

mbercx left a comment • edited Loading

Choose a reason for hiding this comment

unkcpz commented May 15, 2023 • edited Loading

AndresOrtegaGuerrero commented May 15, 2023

mbercx commented May 15, 2023

unkcpz commented May 15, 2023

superstar54 left a comment

Choose a reason for hiding this comment

superstar54 May 15, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 May 16, 2023 • edited Loading

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

AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

unkcpz commented May 17, 2023

unkcpz commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

danielhollas commented Apr 18, 2023 •

edited

Loading

mbercx left a comment •

edited

Loading

unkcpz commented May 15, 2023 •

edited

Loading

superstar54 May 15, 2023 •

edited

Loading

superstar54 May 16, 2023 •

edited

Loading