Migrate widgets to 2.x #344

unkcpz · 2022-08-29T09:06:41Z

computer_resource widget
Drop dep aiida-core <=1.x
Remove all deprecate widgets
~~bump AWB version to 2.0.0~~
computer/code setup widget support aiida-code-registry for both 1.x and 2.x core datatype.
Add tests to test computer/code setup from code registry failed for incompatibility.

Blocking the issue: aiidalab/aiidalab-qe#279

If we implement this change means it is not compatible with 1.x anymore.
I think we need to set more strict dependency for aiida-core and bump the version of this PR to 2.0.0.

@yakutovicha @csadorf, any thoughts?

csadorf · 2022-08-29T10:01:57Z

If we implement this change means it is not compatible with 1.x anymore. I think we need to set more strict dependency for aiida-core and bump the version of this PR to 2.0.0.

@yakutovicha @csadorf, any thoughts?

Yes, if this change requires aiida-core >= 2.0.0 then this needs to be reflected in the declared dependencies.

yakutovicha · 2022-08-29T10:08:18Z

@yakutovicha @csadorf, any thoughts?

if those changes are enough, I am very happy :)

unkcpz · 2022-08-29T10:30:03Z

We are going to deprecate some widgets such as computer.py::SshComputerSetup for AWB 2.0.0, I will also remove them. Any objection?
But always smoke tests are very useful for such changes, is that possible to have the test framework ready for this PR? @csadorf. Let me know how to add it and I'll add the tests.

EDIT: I am wrong about it, there are some tests running and failed, I'll take a look and fix all.

unkcpz · 2022-08-29T10:42:40Z

if those changes are enough, I am very happy :)

Sadly, I this may not enough, we need to have a way to separate aiida-code-registry to support both 1.x and 2.x config files.

unkcpz · 2022-08-31T21:33:39Z

Tests are all passed. But the computer/code setup configs from https://github.com/aiidateam/aiida-code-registry not working anymore.
One possible way is we also release aiida-core-registry with a tag for instance 1.x and make the config database support select data by the tag.
Do you have any better way? @csadorf @yakutovicha

csadorf · 2022-09-01T13:26:18Z

Tests are all passed. But the computer/code setup configs from https://github.com/aiidateam/aiida-code-registry not working anymore. One possible way is we also release aiida-core-registry with a tag for instance 1.x and make the config database support select data by the tag. Do you have any better way? @csadorf @yakutovicha

Can you elaborate on what exactly is not working? Maybe it is possible to do an automated migration? I'd prefer that over maintaining multiple versions for different "target AiiDA versions".

unkcpz · 2022-09-01T20:40:49Z

Can you elaborate on what exactly is not working?

From aiida 1.x to 2.x the transport and scheduler are changed from an endpoint, for example, ssh to core.ssh and slurm to core.slurm. They are hard coded in the config file.

I'd prefer that over maintaining multiple versions for different "target AiiDA versions".

This is for sure a good option but will increase the complexity of the aiida-code-registry which is already not very convenient to add a new entity for HPC resources.

yakutovicha · 2022-09-02T08:43:56Z

This is for sure a good option but will increase the complexity of the aiida-code-registry which is already not very convenient to add a new entity for HPC resources.

Ok, let me tell you my perspective on this.

If the registry is not convenient, we should make it convenient. What are the issues with adding a new code/computer there? I am happy to add a code/computer template or anything to make it more user-friendly.
Can you put some suggestions to the registry in a form of an issue? If things are not working as expected, we (as developers) should not be silent about them.
I believe that @csadorf is talking about an automated migration within AWB. The problem is - at some point the registry will have to be also migrated, which makes it unavoidable to keep old and new configs there. Tbh, I lean a bit towards the suggestion of @unkcpz. We might automatically generate the computer/code config files that are suited for the given version of AiiDA. It won't mean maintaining two registries, it would rather mean having one registry that automatically generates config files for AiiDA-1.x and AiiDA-2.x. If we agree, I can take care of this approach.

unkcpz · 2022-09-02T13:35:41Z

If the registry is not convenient, we should make it convenient. What are the issues with adding a new code/computer there? I am happy to add a code/computer template or anything to make it more user-friendly.

No issue with adding new code/computer. I misunderstood that @csadorf ask to have for same configs files have two versions of aiida supported.
But I have some ideas in mind on how to improve the aiida-code-registry, I will make an AEP for it.

I believe that @csadorf is talking about an automated migration within AWB.

In this case, we need to think about which version to keep in main branch and doing transfer in ABW from 1.x -> 2.x or vice versa. So I'd vote to auto-generate config files AiiDA-1.x and AiiDA-2.x.

@csadorf Sorry that I didn't read Sasha's comment carefully and did not make it very clear in the materials cloud meeting.

csadorf · 2022-09-02T14:16:30Z

As I explained in the MC meeting today, we need to interpret the code registry as a database that is providing information for the automated configuration of AiiDA codes. The specific semantics (ssh vs core.ssh) are in that sense completely irrelevant to the database. As long as the information needed to configure codes in aiida-core 1.x and 2.x remains the same we can always do an automated translation between the different semantics, there is no need to maintain two different versions of the same database.

That translation can happen at various points (at the database level itself), when we read from the database, when we actually configure the codes. Is there any kind of API regarding the registry? Are there any other consumers despite the AWB library? If not, then I would suggest to simply migrate the whole database, otherwise, just do a translation here in the library code. In either case there is no need and we should not maintain multiple database versions.

danielhollas · 2022-09-06T15:05:57Z

Just a note, it would be great if you could release one more AWB version before this is merged. :-)

yakutovicha · 2022-09-07T08:30:23Z

Just a note, it would be great if you could release one more AWB version before this is merged. :-)

I believe you prefer to have #348 merged before the release, right?

danielhollas · 2022-09-07T10:40:19Z

I believe you prefer to have #348 merged before the release, right?

Yeah, that would be nice. More importantly for me, I am also working on a follow-up on #339, to support multiple structure upload in StructureUploadWidget. I'll submit a PR today (EDIT: #353) and then you can judge whether it can be merged quickly or will need more thought and should not block release. Thanks!

unkcpz · 2022-09-26T09:30:39Z

Hi @yakutovicha, hi @csadorf , since this is not blocking the aiidalab-qe issue, I think it is ready to have another round of review.

csadorf · 2022-09-26T10:35:11Z

aiidalab_widgets_base/computational_resources.py

+        self.default_memory_per_machine = ipw.IntText(
+            value=16000000,
+            step=1000000,
+            description="#Memory(RAM) per node (kB):",


I don't think it's a good idea to ask the user to provide the value in kB since the number of machines providing RAM on the order of kB should at this point be restricted to edge and embedded device that are not typically known to run AiiDA or Jupyter. You can always internally convert the value if that is what is expected by the code plugin.

I also think MB or even GB is better

csadorf · 2022-09-26T10:40:42Z

aiidalab_widgets_base/computational_resources.py

@@ -888,11 +906,13 @@ def _reset(self):
        self.work_dir.value = ""
        self.mpirun_command.value = "mpirun -n {tot_num_mpiprocs}"
        self.mpiprocs_per_machine.value = 1
-        self.transport.value = "ssh"
+        self.default_memory_per_machine.value = 16000000


Where is this default value coming from?

It's should be set for AiiDA 2.x, I think @yakutovicha add it to the computer setup and made it mandatory.
It is hard to say if we should provide the default value for this field, maybe we shouldn't provide the default one.
But this comes with the problem that the code-registry-database doesn't have this field set yet, the "quick setup" of widget will fail since no value is provided.

I don't really think it matters where it comes from, but I think there should be some kind of justification for it and then it should be documented here in the form of a short comment.

I think that setting any default value is not good.

There is a way to have no default value for the memory when running verdi computer setup. It is !, which corresponds to the None as an internal value.

aiidalab_widgets_base/databases.py

aiidalab_widgets_base/process.py

.readthedocs.yml

yakutovicha

Thanks, @unkcpz. Not too many comments from my side.

yakutovicha · 2022-10-06T13:57:02Z

aiidalab_widgets_base/computational_resources.py

+        self.default_memory_per_machine = ipw.IntText(
+            value=16000000,
+            step=1000000,
+            description="#Memory(RAM) per node (kB):",


I also think MB or even GB is better

yakutovicha · 2022-10-06T13:58:33Z

aiidalab_widgets_base/computational_resources.py

@@ -888,11 +906,13 @@ def _reset(self):
        self.work_dir.value = ""
        self.mpirun_command.value = "mpirun -n {tot_num_mpiprocs}"
        self.mpiprocs_per_machine.value = 1
-        self.transport.value = "ssh"
+        self.default_memory_per_machine.value = 16000000


I think that setting any default value is not good.

There is a way to have no default value for the memory when running verdi computer setup. It is !, which corresponds to the None as an internal value.

unkcpz · 2022-11-01T11:07:57Z

Just find that since the CI tests are dummy tests with only checking some HTML classes, some functionalities like node viewer are not working properly after migration. I'll quickly check how to fix it. Will pinning you to review after it is ready. @danielhollas @yakutovicha

EDIT: The issue is subtlety related to the UUID traitslet issue #375, so that one should be merged in advance anyway.

danielhollas

@unkcpz thanks so much for the tests, that's awesome that you've been able to make it work. 🎉

Just a few minor comments / questions from me.

Since this is basically ready and has the tests, I would suggest to merge this one first, and build the uuid PR on top of it. I propose to have a temporary branch (aiida2) where we merge this and then we can thoroughly test on the uuid PR before merging to master. What do you think @yakutovicha

tests/docker-compose.yml

tests/test_notebooks.py

danielhollas · 2022-11-01T11:07:25Z

.github/workflows/ci.yml

        strategy:
            matrix:
-                tag: [stable]
-                browser: [chrome, firefox]
+                tag: [edge]


The tag is not used in this file anymore.

I keep it since going to use it as an ENV_VAR for using in the docker-compose. I'll change the docker-compose file.

.github/workflows/ci.yml

danielhollas · 2022-11-01T11:11:54Z

aiidalab_widgets_base/computational_resources.py

@@ -733,6 +767,11 @@ def __init__(self, **kwargs):
            style=STYLE,
        )

+        # Use double quotes to espace.
+        self.use_double_quotes = ipw.Checkbox(
+            value=False, description="Use double quotes"


Maybe add a tooltip with a more detailed explanation of what this is used for? Also the comment in the code should explain in more detail why the escaping is needed.

danielhollas · 2022-11-01T11:13:11Z

aiidalab_widgets_base/computational_resources.py

-                if hasattr(self, key):
+                if key == "default_memory_per_machine":
+                    self.default_memory_per_machine_widget.value = int(
+                        value / 1024**2


I don't understand why the 1024 value is squared?

The value in the database is in kB, but we agreed previously to keep GB for the widget. Just to answer why 1024 is squared.

I also noticed that this line is wrong, though. Thanks, @danielhollas, for pointing at it. The values shouldn't be an integer but a string.

Possible solutions are:

self.default_memory_per_machine_widget.value = str(decimal.Decimal(f"{value/1024**2:0.3f}").normalize()) + ' GB'

This requires importing decimal.

Another solution is simply to say that the imported value is in kB

self.default_memory_per_machine_widget.value = f"{value KB}

Any approach is good for me.

I am going to disappear, for some time, so I let @unkcpz and/or @danielhollas pick up an appropriate solution.

I prefer the second one without the decimal dependency.

tests/docker-compose.yml

yakutovicha · 2022-11-01T12:06:39Z

Since this is basically ready and has the tests, I would suggest to merge this one first, and build the uuid PR on top of it. I propose to have a temporary branch (aiida2) where we merge this and then we can thoroughly test on the uuid PR before merging to master. What do you think @yakutovicha

I would merge those things directly to master and make only bug fixes in the support/aiida-1.x branch. I would release 1.4.x versions from the support/aiida-1.x branch.

unkcpz · 2022-11-01T12:36:45Z

I would merge those things directly to master and make only bug fixes in the support/aiida-1.x branch. I would release 1.4.x versions from the support/aiida-1.x branch.

I agree!
The issue I mentioned for Node viewer is not dependent on the traitlets UUID change PR. I will fix the things mentioned in @danielhollas review and then should be good to go.

danielhollas

Thanks @unkcpz.

I am still not a fan of the current default_memory_per_machine widget. To me the fact that it mixes both number and text is super confusing, not to mention that we're introducing an extra dependency just to parse this string. When this field is not prefilled from the loaded computer, it is not clear what the units are, or even that one can specify the units.

Instead I would suggest to use two separate widgets, ipw.IntText (or ipw.BoundedIntText) for the numerical value itself, and ipw.Dropdown for specifying the units, perhaps backed by an Enum like this.

class MemoryUnits(IntEnum):
    KB = 1024
    MB = 1024**2
    GB = 1024**3

self.memory_units = ipw.Dropdown(
    options=[(unit.name, unit) for unit in MemoryUnits],
    value=MemoryUnits.MB,
    description=""
)

Nevertheless, I am approving so this can be done and iterated upon in a separate PR.

danielhollas · 2022-11-01T17:27:19Z

.github/workflows/ci.yml

+                  pip install -U -e .[tests]
+
+            - name: Run pytest
+              run: TAG=$(${{ matrix.tag }}) JUPYTER_TOKEN=$(openssl rand -hex 32) pytest --driver ${{ matrix.browser }}


I think this should work

Suggested change

run: TAG=$(${{ matrix.tag }}) JUPYTER_TOKEN=$(openssl rand -hex 32) pytest --driver ${{ matrix.browser }}

run: TAG=${{ matrix.tag }} JUPYTER_TOKEN=$(openssl rand -hex 32) pytest --driver ${{ matrix.browser }}

True, changed. Can you approve it again?

Nevermind, since this is a tiny fix if the tests passed. I can force merge it anyway.

.github/workflows/ci.yml

tests/docker-compose.yml

tests/test_notebooks.py

Done

yakutovicha · 2022-11-01T22:54:19Z

I am still not a fan of the current default_memory_per_machine widget. To me the fact that it mixes both number and text is super confusing, not to mention that we're introducing an extra dependency just to parse this string. When this field is not prefilled from the loaded computer, it is not clear what the units are, or even that one can specify the units.

Since I am responsible for this one, I try to defend my implementation.

From my perspective, the fact that the widget is human-readable is rather a plus: one can essentially put any value here and make sure that things will work. If one writes something that can’t be parsed, the cross will appear notifying the user about that.

if the field is not filled - it is not used. I added a placeholder that tries to explain that. Feel free to suggest a better one.

Also, concerning the new dependency: it is pretty well maintained, so I wouldn’t worry about that

yakutovicha

Thanks @unkcpz 👍

…iiDA 2.x - Use v2 code-registry database - Add use_double_quotes for code setup - use default user for computer configure

Since we need to set defalut memory for the computer setup now, the parameters need to provide to the widget. Here the human-readable input are implemented for the widget, so it can parse the inputs to the desired value.

The CI test was run by aiidalab-test-app-action which is outdated and not going to be maintained anymore. Since we have the new aiidalab-docker-stack image to launch the AiiDAlab instance, we use it here to run the test by using the pytest-docker and implement selenium tests directly and explicitly by checking the elements after the notebooks are opened and loaded. The new way of testing make the CI tests more robust and easy to write. It also support further tests on benchmark tests on widgets loading. - Remove all support of 3.7 since aiida-core not support it

…Workchain (aiidalab#348) fixes aiidalab#344 The register_viewer_widget decorator registers the viewer by class and the new viewer for QeAppWorkchain specifically also become the viewer of other WorkChain. It will raise the KeyError and raised which is not expected. Instead of raising the exception, nothing is returned. This is a workaround since ideally if there is no new specific viewer defined for a specific work chain, it needs to fall back to the native one of AWB. As tried in aiidalab#430

unkcpz mentioned this pull request Aug 29, 2022

Mig/2.1 aiidalab/aiidalab-qe#271

Merged

4 tasks

unkcpz force-pushed the mig/aiida-core-2.x branch from 784048d to c785192 Compare August 29, 2022 10:36

unkcpz requested review from csadorf and yakutovicha August 29, 2022 10:37

csadorf linked an issue Aug 30, 2022 that may be closed by this pull request

AWB: Support AiiDA 2.0 #346

Closed

unkcpz force-pushed the mig/aiida-core-2.x branch 2 times, most recently from 114ade6 to f14bf17 Compare August 31, 2022 21:25

danielhollas mentioned this pull request Sep 6, 2022

ComputationalResourceWidget: Hide comp_resources_database if it is empty #341

Merged

unkcpz mentioned this pull request Sep 7, 2022

Use UUID rather than the aiida node instance as traitlet #354

Closed

csadorf added the blocked This issue/PR is blocked by another issue/PR. label Sep 20, 2022

unkcpz force-pushed the mig/aiida-core-2.x branch 2 times, most recently from af4ed5a to 085e55a Compare September 26, 2022 09:29

csadorf previously requested changes Sep 26, 2022

View reviewed changes

unkcpz mentioned this pull request Sep 26, 2022

Refac: remove unsed hold_trait_notification for resource setup widget #357

Merged

unkcpz force-pushed the mig/aiida-core-2.x branch from 15bbf15 to 75507fb Compare September 26, 2022 12:18

yakutovicha reviewed Oct 6, 2022

View reviewed changes

unkcpz requested a review from danielhollas November 1, 2022 02:26

danielhollas removed the blocked This issue/PR is blocked by another issue/PR. label Nov 1, 2022

unkcpz requested review from csadorf and removed request for csadorf November 1, 2022 11:08

danielhollas requested changes Nov 1, 2022

View reviewed changes

unkcpz force-pushed the mig/aiida-core-2.x branch from 49b19f1 to 3886656 Compare November 1, 2022 12:01

unkcpz force-pushed the mig/aiida-core-2.x branch 3 times, most recently from 664562d to ac318df Compare November 1, 2022 16:18

unkcpz requested a review from danielhollas November 1, 2022 17:04

danielhollas previously approved these changes Nov 1, 2022

View reviewed changes

yakutovicha previously approved these changes Nov 1, 2022

View reviewed changes

unkcpz dismissed stale reviews from yakutovicha and danielhollas via f1b93be November 2, 2022 07:57

unkcpz and others added 3 commits November 2, 2022 12:35

Remove deprecated widgets and classes and migrate widget to support A…

71896c7

…iiDA 2.x - Use v2 code-registry database - Add use_double_quotes for code setup - use default user for computer configure

Add memory widget that accepts human-readable input

92fd98c

Since we need to set defalut memory for the computer setup now, the parameters need to provide to the widget. Here the human-readable input are implemented for the widget, so it can parse the inputs to the desired value.

unkcpz force-pushed the mig/aiida-core-2.x branch from f1b93be to b5b1c8a Compare November 2, 2022 11:39

unkcpz enabled auto-merge (rebase) November 2, 2022 11:40

unkcpz requested review from danielhollas and yakutovicha November 2, 2022 11:40

danielhollas approved these changes Nov 2, 2022

View reviewed changes

unkcpz merged commit a65e6ad into master Nov 2, 2022

unkcpz deleted the mig/aiida-core-2.x branch November 2, 2022 11:45

	run: TAG=$(${{ matrix.tag }}) JUPYTER_TOKEN=$(openssl rand -hex 32) pytest --driver ${{ matrix.browser }}
	run: TAG=${{ matrix.tag }} JUPYTER_TOKEN=$(openssl rand -hex 32) pytest --driver ${{ matrix.browser }}

Migrate widgets to 2.x #344

Migrate widgets to 2.x #344

Conversation

unkcpz commented Aug 29, 2022 • edited by yakutovicha Loading

csadorf commented Aug 29, 2022

yakutovicha commented Aug 29, 2022

unkcpz commented Aug 29, 2022 • edited Loading

unkcpz commented Aug 29, 2022

unkcpz commented Aug 31, 2022 • edited Loading

csadorf commented Sep 1, 2022

unkcpz commented Sep 1, 2022 • edited Loading

yakutovicha commented Sep 2, 2022

unkcpz commented Sep 2, 2022

csadorf commented Sep 2, 2022

danielhollas commented Sep 6, 2022

yakutovicha commented Sep 7, 2022

danielhollas commented Sep 7, 2022 • edited Loading

unkcpz commented Sep 26, 2022 • 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

yakutovicha Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

yakutovicha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

unkcpz commented Nov 1, 2022 • edited Loading

danielhollas 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

yakutovicha Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha commented Nov 1, 2022

unkcpz commented Nov 1, 2022

danielhollas 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

yakutovicha commented Nov 1, 2022 • edited Loading

yakutovicha left a comment

Choose a reason for hiding this comment

unkcpz commented Aug 29, 2022 •

edited by yakutovicha

Loading

unkcpz commented Aug 29, 2022 •

edited

Loading

unkcpz commented Aug 31, 2022 •

edited

Loading

unkcpz commented Sep 1, 2022 •

edited

Loading

danielhollas commented Sep 7, 2022 •

edited

Loading

unkcpz commented Sep 26, 2022 •

edited

Loading

yakutovicha Oct 6, 2022 •

edited

Loading

yakutovicha Oct 6, 2022 •

edited

Loading

unkcpz commented Nov 1, 2022 •

edited

Loading

yakutovicha Nov 1, 2022 •

edited

Loading

yakutovicha commented Nov 1, 2022 •

edited

Loading