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

providing a subsection in developer guide docs, about automatic code installation #576

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mikibonacci
Copy link
Member

No description provided.

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0cc35b9) 80.73% compared to head (1d571ae) 80.73%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #576   +/-   ##
=======================================
  Coverage   80.73%   80.73%           
=======================================
  Files          49       49           
  Lines        3415     3415           
=======================================
  Hits         2757     2757           
  Misses        658      658           
Flag Coverage Δ
python-3.10 80.73% <ø> (ø)
python-3.8 80.77% <ø> (ø)
python-3.9 80.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@unkcpz
Copy link
Member

unkcpz commented Dec 5, 2023

Cool thanks @mikibonacci
It looks good to me, but maybe we can wait for #565, @superstar54 did some changes on using AiiDA python API instead of calling verdi to speed it up. The performance is not a problem for phonopy installation, but I think it is better to have a consistent method to use for new code setups.

@mikibonacci
Copy link
Member Author

yep, fine for me to wait :)

thanks

@mikibonacci
Copy link
Member Author

mikibonacci commented Dec 7, 2023

What I should add (from aiidalab meeting discussions):

  • actually, a entry point in aiidalab_qe.console_scripts, ore something similar. Anyway, a standardized way for all plugins (and document it of course);
  • the script should be something that it can be triggered also from the GUI (functionality not yet implemented in the GUI). So, forward-compatibility;
  • In the script we need also some conda or git clone of the needed codes. In principle, python packages will be downloaded as requirements of the plugin (or used aiida plugin), but we need something general I think.

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.

After a second look at #565, I think it requires more discussion on it. Therefore I personally don't want to block this PR and propose to go with calling verdi command to set the code which we test over many releases and should be robust.
The performance issue is caused from verdi command and will be improved a lot after aiida-core v2.5.0 which will soon release.

@mikibonacci thanks for adding this, I added some minor requests, if you can give it a look I'll do a local test with the method you write here.

Comment on lines +357 to +359
Thus, the user needs to set up the codes manually. However, the developer can provide console
scripts to make user life easier. Please read to following section for some instructions on how
to do it.
Copy link
Member

Choose a reason for hiding this comment

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

We try to break lines for a full sentence to make it easy to review and for future change. Do need to worry about a long line (if it is super long, then probably means it can be cut to short sentences for readability).

Comment on lines +364 to +365
It is possible to provide console scripts in order to make new code installation almost automatic,
with only running an instruction from the command line.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It is possible to provide console scripts in order to make new code installation almost automatic,
with only running an instruction from the command line.
It is possible to provide console scripts to make new code installation almost automatic, by only running an instruction from the command line.

Comment on lines +379 to +380
this will invoke the function `install_phonopy`, contained in the phonopy_install.py file in the
root directory of the package.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this will invoke the function `install_phonopy`, contained in the phonopy_install.py file in the
root directory of the package.
this will invoke the function `install_phonopy`, contained in the `phonopy_install.py` file in the root directory of the package.

this will invoke the function `install_phonopy`, contained in the phonopy_install.py file in the
root directory of the package.

or in the `setup.cfg` file (if you prefer):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
or in the `setup.cfg` file (if you prefer):
Another option is add following lines in the `setup.cfg` file:

Comment on lines +390 to +391
this will invoke the function `InstallCodes`, contained in the post_install.py file in the
{root-directory-of-the-package}/aiidalab_qe_muon/scripts/ directory.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this will invoke the function `InstallCodes`, contained in the post_install.py file in the
{root-directory-of-the-package}/aiidalab_qe_muon/scripts/ directory.
This will invoke the function `InstallCodes`, contained in the `post_install.py` file in the `<root-directory-of-the-package>/aiidalab_qe_muon/scripts/` directory.

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