-
Notifications
You must be signed in to change notification settings - Fork 5
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
⬆️ Update to aiida-core v1.4.2 #48
Changes from all commits
9950c1c
4a97fee
02b92da
9e3c1d9
a4d68f2
3c28f9e
d8e73a7
9344a4c
bd0463d
a175318
b4f5db3
e1c1f44
02e7f33
9262cf0
f8e7d97
85639d9
1b933d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
*.swp | ||
.DS_Store | ||
.galaxy_install_info | ||
.vscode/ | ||
.tox/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
#!/usr/bin/env python | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine to keep it this parser here for the moment - eventually this should probably go to into https://github.com/aiidateam/aiida-core/tree/develop/utils |
||
"""Script to convert aiida setup.json to pip constraints file. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not constraints but rather requirements. You'd usually use https://github.com/jazzband/pip-tools to generate the pinned env deps aka constraints. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, pip-tools can generate pins from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no that creates a lock file, with strict version pinning. we just want a constraints file, for when you run subsequent pip installs, i.e. these installs can change the version of packages if necessary, but only within the ranges specified by setup.json There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chrisjsewell true but the purpose of constraints file in pip is exactly that it should be a lock file. So that the usage would be |
||
|
||
Usage:: | ||
|
||
python aiida-core-constraints.py path/to/setup.json extras1 extras2 ... | ||
""" | ||
import json | ||
from pathlib import Path | ||
import sys | ||
|
||
def remove_extra(req): | ||
"""Extras are not allowed in constraints files, e.g. dep[extra]>0.1 -> dep>0.1 """ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, fair enough. Using pip-tools would fix that too. |
||
if "[" not in req or "]" not in req: | ||
return req | ||
start, end = req.split("[", 1) | ||
return start + end.split("]", 1)[1] | ||
|
||
|
||
def main(path, *extras): | ||
path = Path(path) | ||
data = json.loads(path.read_text("utf8")) | ||
|
||
output = ["# aiida-core requirements: v" + data["version"]] | ||
# we can't add this when using the file as a constraint file for actually installing aiida-core | ||
# output.append("aiida-core==" + data["version"]) | ||
output.extend([remove_extra(req) for req in data["install_requires"]]) | ||
|
||
for extra in extras: | ||
output.append("# " + extra) | ||
output.extend([remove_extra(req) for req in data["extras_require"][extra]]) | ||
|
||
print("\n".join(output)) | ||
|
||
if __name__ == "__main__": | ||
assert len(sys.argv) > 1, "JSON path required" | ||
main(sys.argv[1], *sys.argv[2:]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,16 @@ | |
- item.name in aiida_pps.stdout | ||
with_items: "{{ aiida_pseudopotentials }}" | ||
|
||
- name: print aiida-core version | ||
shell: "{{ aiida_venv }}/bin/pip show aiida-core" | ||
changed_when: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you don't even need this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought just in case a plugin tries to install a different version There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I meant the |
||
register: aiida_pip_show | ||
|
||
- name: check aiida-core version | ||
assert: | ||
that: | ||
- aiida_version in aiida_pip_show.stdout | ||
|
||
- name: check plugin versions | ||
include_tasks: tests/test_plugins.yml | ||
with_dict: "{{ aiida_plugin_versions }}" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
# for running | ||
ansible~=2.9 | ||
ansible~=2.10.0 | ||
# for testing | ||
docker | ||
molecule~=3.0 | ||
molecule[docker]~=3.1.0 | ||
docker~=4.2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,9 +75,9 @@ | |
pip: | ||
name: | ||
- pip~=20.0 | ||
- setuptools~=36.2 | ||
- setuptools | ||
- wheel | ||
- numpy==1.17.4 | ||
- numpy # required by pymatgen | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no pinning, are you sure? ;-) I'd feel safer with pinning it to some recent minor version There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is only needed for the pymatgen install, it will be updated, if necessary, by the pip install of aiida-core. If you pin it, then you have to ensure it is compatible with aiidalab and aiida-core (a current issue in the full quantum mobile build) |
||
virtualenv: "{{ aiida_venv }}" | ||
# According to: https://github.com/ansible/ansible/issues/52275 | ||
virtualenv_command: /usr/bin/python3 -m venv | ||
|
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.
Should this be removed?
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.
Ah, I see; we need to lock things down even further (?).
Should we be adding this constraint in the
setup.json
?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.
perhaps 🤷 (to the docs extra), I guess with the new
pip --use-feature=2020-resolver
it should resolve this and not allow such a broken environment to transpire. But without it, and with this particular set of extras it definitely creates a broken environmentThere 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.
@chrisjsewell that's why I advocate for having proper env requirements+pins managed by pip-tools instead of extras
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.
Yeh we do already have lock files for CI testing: https://github.com/aiidateam/aiida-core/tree/develop/requirements.
The problem is that we are not just trying to install aiida-core, we need to install a range of other plugin packages that might not be exactly compatible with these hard pinnings:
ansible-role-aiida/defaults/main.yml
Lines 60 to 71 in c709088
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.
Oh, so you need a combined lockfile, I guess... Just dump that to a file and use a few requirements sources to produce transitive pins? FWIW I think what confused me is the definition substitution (constraints vs requirements).
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.
Yeh exactly, I'm not completely sold on the current solution here, but at least now it is properly tested and produces a valid environment, which is a step forward 😬
So I am going to merge this as this, but anyway we have an open issue marvel-nccr/quantum-mobile#137 to consider this further, at which point I will definitely take into account your recommendation (thanks 🙏) and may well switch to some form of lock file
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.
FYI @webknjaz you might be interested to see I now set up a full ~2 hour vagrant+ansible build running in GH Actions: https://github.com/marvel-nccr/quantum-mobile/runs/1286287172?check_suite_focus=true 😄