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

Refactor installation routines and allow to run them from the CLI. #251

Conversation

csadorf
Copy link
Member

@csadorf csadorf commented Jul 5, 2022

No description provided.

@csadorf
Copy link
Member Author

csadorf commented Jul 5, 2022

Depends on aiidalab/aiidalab#295 .

@csadorf csadorf marked this pull request as ready for review July 7, 2022 12:31
@csadorf csadorf requested a review from unkcpz July 7, 2022 12:31
@csadorf csadorf linked an issue Jul 7, 2022 that may be closed by this pull request
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@csadorf thanks! The codes all look good. Just a minor request.
Since this change depend on the aiidalab, I think we need to also add aiidalab dependency in the setup.cfg file?

I still need to test this PR thoroughly both with CLI and install qe from setup widget. (Tested and all good from my side.)

@@ -281,6 +277,10 @@ def _update(self, change):

if self.error or self.installed:
self._progress_bar.value = 1.0
elif self.busy:
self._progress_bar.value = ProgressBar.AnimationRate(1.0)
Copy link
Member

Choose a reason for hiding this comment

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

What is the rate equal to 1.0 means? Can you add a comment in the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, would that be sufficient?

Suggested change
self._progress_bar.value = ProgressBar.AnimationRate(1.0)
# Start progress bar animation at constant rate (1.0).
self._progress_bar.value = ProgressBar.AnimationRate(1.0)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I still don't get the intuitive idea of what the 1.0 mean and in what unit?

for code_name in CODE_NAMES:
if not _code_is_setup(code_name):
yield f"Setting up AiiDA code ({code_name})..."
_setup_code(code_name)
Copy link
Member

Choose a reason for hiding this comment

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

After installing and setup of the code finished successfully, the FN_DO_NOT_SETUP need to be removed? Or it is already somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

The FN_DO_NOT_SETUP file prevents the installation. It is only created if the setup failed. That is to prevent the setup to make an attempt to perform the installation every time the app is opened.

Copy link
Member

Choose a reason for hiding this comment

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

I see, probably I set up code by hand long ago, I both have code successfully set up and FN_DO_NOT_SETUP file. I test with remove this file and creating a new profile to test and all works fine.

@csadorf
Copy link
Member Author

csadorf commented Jul 12, 2022

Since this change depend on the aiidalab, I think we need to also add aiidalab dependency in the setup.cfg file?

It does not, what made you think that?

@unkcpz
Copy link
Member

unkcpz commented Jul 13, 2022

Since this change depend on the aiidalab, I think we need to also add aiidalab dependency in the setup.cfg file?

It does not, what made you think that?

Because the post_install is added here which requires aiidalab/aiidalab#295 to be merged first.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@csadorf Thanks! It is all good from my side. Please go ahead.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Just come up with one more question not clear from my side.

aiidalab_qe/setup_codes.py Show resolved Hide resolved
@csadorf
Copy link
Member Author

csadorf commented Jul 13, 2022

Since this change depend on the aiidalab, I think we need to also add aiidalab dependency in the setup.cfg file?

It does not, what made you think that?

Because the post_install is added here which requires aiidalab/aiidalab#295 to be merged first.

Yes, but the installation is still triggered like before if post_install is ignored. But we could also require a higher aiidalab version if you prefer that.

@csadorf csadorf merged commit 97251d1 into master Jul 14, 2022
@csadorf csadorf deleted the feature/249-trigger-installation-of-qe-and-pseudo-installation-after-app-installation branch July 14, 2022 14:40
unkcpz added a commit that referenced this pull request Aug 8, 2022
)

After PR #251 there is an issue that `SSSPInstallWidget` can not update the state to installed even if the pseudo libraries are properly installed.
The reason is that SSSPInstallWidget once updates the installed traitlets in `_refresh_installed` but not in the new implementation.
The way to fix this is to set the `installed` state if function `install()` finished okay.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants