Skip to content
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

Feature/select process #23

Merged
merged 6 commits into from
Nov 27, 2020
Merged

Feature/select process #23

merged 6 commits into from
Nov 27, 2020

Conversation

csadorf
Copy link
Member

@csadorf csadorf commented Nov 16, 2020

Enable the switch between previously submitted processes and the submission of new processes.

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @csadorf, it is a very good feature. I tried different scenarios of running the app. Based on my interaction experience I have a few questions/suggestions:

  • Is it possible to call the reset, when the structure is changed? I believe you can observe the structure trait for that.
  • Related to the previous point. If, after changing the structure, I click on the refresh button - the structure remains unchanged.
  • If I open the "Select structure" accordion, the reset button gets deactivate. Is it intended?
  • If, after selecting a finished calculation, I go back to the New calculation.. and click on Refresh button - the structure remains unchanged. If I change the structure, the Confirm button remains inactive. Only after pressing RESET the confirm button gets activated.
  • Selecting the running calculation seems not to be very responsive.

Comment on lines +56 to +59
assert dict(load_default_parameters()) \
== dict(builder.scf.pw['parameters']) \
== dict(builder.bands.pw['parameters'])
assert builder.scf.kpoints_distance == Float(0.8)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure why do you want those parameters to always be the same. What does this condition bring?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is used to ensure that the parameters of the process we load is the same that we would use to submit. Otherwise a very basic user assumption would be broken, namely that resubmitting a previous process is actually submitting with the same parameters. It would probably be even better to only show processes in the list with those parameters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't entirely understand the motivation behind but to me, a slight modification of the input parameters would be normal. For instance, for the metals, you want to include smearing, while for non-metals you don't. I think the "clever" work chains actually do those changes. So I am still not fully convinced this check is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you load a process and then just resubmit it to a different host. Wouldn't you expect it to run the exact same calculation? Or maybe even on the same host? Would you not be very surprised if the calculation executed ends up actually being a different one? I would be. 😕

codes.py Show resolved Hide resolved
process.py Outdated Show resolved Hide resolved
@csadorf
Copy link
Member Author

csadorf commented Nov 18, 2020

Thanks, @csadorf, it is a very good feature.

Thx!

I tried different scenarios of running the app. Based on my interaction experience I have a few questions/suggestions:

* Is it possible to call the reset, when the structure is changed? I believe you can observe the `structure` trait for that.

Ideally it should not even be possible to select a different structure without resetting, but I'll look into that.

* Related to the previous point. If, after changing the structure, I click on the refresh button - the structure remains unchanged.

The "refresh" button refreshes the list of work chains, not the selected work chain. That is probably ambiguous. Since the list of work chains is auto-refreshed every 10 seconds or so anyways, should we maybe just remove the "refresh" button?

* If I open the "Select structure" accordion, the `reset` button gets deactivate. Is it intended?

Yes, you can only reset the latest locked step, in that case it is step 2.

* If, after selecting a finished calculation, I go back to the `New calculation..` and click on `Refresh` button - the structure remains unchanged. If I change the structure, the `Confirm` button remains inactive. Only after pressing `RESET` the `confirm` button gets activated.

This seems like a bug in the structure selection step. Like I said before, it should not be possible to change the structure once it is confirmed. I'll look into it.

* Selecting the running calculation seems not to be very responsive.

What exactly is not responsive? I've tried to streamline the implementation as much as possible and am not sure how to make it more efficient, but maybe you can describe to me what exactly you would expect to happen faster and I can double check for possible issues.

For example, resetting step 1, also resets step 2 and all other
following steps, while resetting from step 2 only resets step 2 and
following steps.

That means resetting all steps can be achieved from the first step.
@csadorf
Copy link
Member Author

csadorf commented Nov 20, 2020

@yakutovicha I believe I've addressed all of your comments. I've changed the app behavior in the way that you suggested. So clicking reset on step 1 will actually reset all steps and furthermore, selecting a new structure automatically resets the app. Let me know what you think!

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @csadorf.

I tested the newest changes and I have three comments:

  1. Looks like there is a loop that presses the Refresh button with a certain interval. I don't know whether this is only my case but after a certain number of auto-refreshes, the latest output gets frozen.
  2. Another thing is related to the code selection widget. I realized that if the code, which was used for the finished calculation, is not available - the app fails. This is entirely the problem of the CodeDropdown widget and has been fixed in the previous release. This makes it important to update the image before running the tutorial.
  3. I would probably remove the structure after selecting the "New calculation" in the process selection widget, but this is up to you in the end.

@csadorf
Copy link
Member Author

csadorf commented Nov 27, 2020

1. Looks like there is a loop that presses the `Refresh` button with a certain interval. I don't know whether this is only my case but after a certain number of auto-refreshes, the latest output gets frozen.

No, I have not experienced that issue. Does the widget simply not update anymore or does it remain disabled?

2. Another thing is related to the code selection widget. I realized that if the code, which was used for the finished calculation, is not available - the app fails. This is entirely the problem of the `CodeDropdown` widget and has been fixed in the previous release. This makes it important to update the image before running the tutorial.

👍

3. I would probably remove the structure after selecting the "New calculation" in the process selection widget, but this is up to you in the end.

I'll consider it.

@csadorf
Copy link
Member Author

csadorf commented Nov 27, 2020

@yakutovicha I've played around with resetting the structure as well and found that it leads to inconsistencies to what the "New calculation..." state implies. So for the time being, I'd prefer to leave it as is.

I have not been able to identify any issues with the process selection auto-refresh.

@csadorf
Copy link
Member Author

csadorf commented Nov 27, 2020

@yakutovicha Issue should be fixed now.

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, @csadorf .

There is one little remaining issue: if one runs a calculation, then switches to a completed one, and then switches back to the running calculation - the on-the-fly output gets frozen.

But I think it is fine to fix it later. This is not a critical issue.

@csadorf csadorf changed the base branch from master to develop November 27, 2020 16:36
@csadorf csadorf merged commit a5f90f2 into develop Nov 27, 2020
@csadorf csadorf deleted the feature/select-process branch November 27, 2020 16:37
csadorf added a commit to csadorf/aiidalab-qe that referenced this pull request Jan 21, 2021
* Add header to retrieve previously submitted processes.

* Make ProcessOutputFollower more resilient against errors during fetch.

For example when the node from which output is fetched was deleted.

* Enable app steps reset from arbitrary step.

For example, resetting step 1, also resets step 2 and all other
following steps, while resetting from step 2 only resets step 2 and
following steps.

That means resetting all steps can be achieved from the first step.

* Automatically reset steps when selecting a new and different structure.

* Fix issue causing the latest output to hang upon process list refresh.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants