-
Notifications
You must be signed in to change notification settings - Fork 17
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
Migrate widgets to 2.x #344
Conversation
Yes, if this change requires aiida-core >= 2.0.0 then this needs to be reflected in the declared dependencies. |
if those changes are enough, I am very happy :) |
We are going to deprecate some widgets such as EDIT: I am wrong about it, there are some tests running and failed, I'll take a look and fix all. |
784048d
to
c785192
Compare
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. |
114ade6
to
f14bf17
Compare
Tests are all passed. But the computer/code setup configs from https://github.com/aiidateam/aiida-code-registry not working anymore. |
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". |
From aiida 1.x to 2.x the transport and scheduler are changed from an endpoint, for example,
This is for sure a good option but will increase the complexity of the |
Ok, let me tell you my perspective on this.
|
No issue with adding new code/computer. I misunderstood that @csadorf ask to have for same configs files have two versions of aiida supported.
In this case, we need to think about which version to keep in @csadorf Sorry that I didn't read Sasha's comment carefully and did not make it very clear in the materials cloud meeting. |
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 ( 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. |
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? |
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! |
af4ed5a
to
085e55a
Compare
Hi @yakutovicha, hi @csadorf , since this is not blocking the aiidalab-qe issue, I think it is ready to have another round of review. |
self.default_memory_per_machine = ipw.IntText( | ||
value=16000000, | ||
step=1000000, | ||
description="#Memory(RAM) per node (kB):", |
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 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.
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 also think MB or even GB is better
@@ -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 |
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.
Where is this default value coming from?
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.
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.
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 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.
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 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.
15bbf15
to
75507fb
Compare
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. Not too many comments from my side.
self.default_memory_per_machine = ipw.IntText( | ||
value=16000000, | ||
step=1000000, | ||
description="#Memory(RAM) per node (kB):", |
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 also think MB or even GB is better
@@ -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 |
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 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.
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. |
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.
@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
.github/workflows/ci.yml
Outdated
strategy: | ||
matrix: | ||
tag: [stable] | ||
browser: [chrome, firefox] | ||
tag: [edge] |
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.
The tag is not used in this file anymore.
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 keep it since going to use it as an ENV_VAR for using in the docker-compose. I'll change the docker-compose file.
@@ -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" |
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.
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.
if hasattr(self, key): | ||
if key == "default_memory_per_machine": | ||
self.default_memory_per_machine_widget.value = int( | ||
value / 1024**2 |
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 don't understand why the 1024 value is squared?
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.
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.
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 prefer the second one without the decimal
dependency.
49b19f1
to
3886656
Compare
I would merge those things directly to |
I agree! |
664562d
to
ac318df
Compare
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.
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.
.github/workflows/ci.yml
Outdated
pip install -U -e .[tests] | ||
|
||
- name: Run pytest | ||
run: TAG=$(${{ matrix.tag }}) JUPYTER_TOKEN=$(openssl rand -hex 32) pytest --driver ${{ matrix.browser }} |
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 think this should work
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 }} |
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.
True, changed. Can you approve it again?
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.
Nevermind, since this is a tiny fix if the tests passed. I can force merge it anyway.
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 |
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 👍
…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
f1b93be
to
b5b1c8a
Compare
…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
computer_resource
widgetbump AWB version to2.0.0
aiida-code-registry
for both 1.x and 2.x core datatype.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?