Computer setup: Fix core.local transport #502
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #502 +/- ##
==========================================
+ Coverage 79.46% 79.50% +0.04%
==========================================
Files 27 27
Lines 3759 3792 +33
==========================================
+ Hits 2987 3015 +28
- Misses 772 777 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think this is good for now.
However, later I am introducing the ssh_light which will revert things to Dropdown. Then, there will be firecrest. It would take some more work to adopt them, though.
Therefore, for now, it is good to keep it as HTML to signify that widget is not dealing with other cases.
I would only request to keep the self.transport.value = "core.ssh" line and set the original value to core.ssh too.
It was issued later on. Also, settings downloaded from the code database override the value. Herein, SSH as the value may cause trouble.
This reverts commit 94565d4.
Ah, that's good to know. I also found out that the transport value is indeed passed to the computer setup function, I did not notice that before since it is accessed via
Another good point. As a side note, I am not a fan of this design, Given all of the above, I've reverted the original change and instead fix the underlying bug and made the |
danielhollas
changed the title
| "safe_interval": self.safe_interval.value, | ||
| } | ||
| user = orm.User.collection.get_default() | ||
| computer.configure(user=user, **authparams) |
There was a problem hiding this comment.
Note that we need to catch any potential errors here. Since this was not done in the original function, I am keeping that for separate PR.
There was a problem hiding this comment.
Thanks, @danielhollas. As you've suggested, let's not allow creating a computer if its configuration is not working. The rest is good, I tested the widget locally and it works as expected 👍 .
Validate more inputs and catch more errors.
There was a problem hiding this comment.
@yakutovicha I've made some more improvements, can you take a look?
| @@ -902,16 +936,15 @@ def on_setup_computer(self, _=None): | |||
| ) | |||
| try: | |||
| computer = computer_builder.new() | |||
| self._configure_computer(computer) | |||
| self._configure_computer(computer, self.transport.value) | |||
| computer.store() | |||
There was a problem hiding this comment.
I've moved this call here since it can also fail.
Strangely, when a user provides empty workdir, the store command raises ValidationError, but the computer is still stored. That to me seems like aiida-core bug.
There was a problem hiding this comment.
Thanks, @danielhollas. I suggested a few minor changes. I also tested it, all good.
|
Great, thanks @yakutovicha! |
* Add a test for localhost setup * Raise if transport type is not recognized * Validate more inputs and catch more errors. Co-authored-by: Aliaksandr Yakutovich <[email protected]>
This PR adds an entry point for step 1, allowing the user to add a new structure importer and editor. In principle, all the structure importer and editor are pluggable. This PR generalizes this by adding entry point to it. This will be very useful: - add structure importer specific for particular structures, e.g. surface, adsorbate. - add a new editor to edit a structure for the plugin, e.g. edit tags, and cut surface.
It is not clear whether AiiDA Computer setup ever worked with any transport other than SSH,
but in any case the widget is clearly designed with SSH in mind so unless a clear need is found we can simply remove the 'Transport type" dropdown. Note the the value of this widget has not been used anywhere in the code, which is telling.
The localhost computer is setup in the AiiDAlab Docker image by default, and in the unlikely case that another one needs to be setup, such user probably knows how to set it up via verdi command line interface.
Fixes #417