-
Notifications
You must be signed in to change notification settings - Fork 193
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
Docs: revamp Intro/Install #4060
Docs: revamp Intro/Install #4060
Conversation
Ok this is coming along @csadorf, if you want a quick look!
|
One thing I've just realised, is that there is no top-level headers for any of the sections (tutorial, how-tos, topics, etc), and so nothing specific to link to on the front page! |
@chrisjsewell Just a thing I noticed just now: the sixth panel is called "Development" and describes that this is for guidelines for contributors, but we decided that this would be hosted on the Github wiki. Instead we have the "Internal Architecture" section. This will provide detailed information on the design and architecture and the reason behind it, including the decision process. |
@sphuber does this look ok: https://69-77234579-gh.circle-artifacts.com/0/html/index.html |
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.
Overall good, just a few minor suggestions.
AiiDA has been tested on the following platforms: | ||
|
||
* Ubuntu 14.04, 16.04, 18.04 | ||
* Mac OS X |
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.
@sphuber Can we qualify this further?
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.
Not sure, to be honest. Depends on how how strict we want to have "tested" mean. If that should only include with what we have tested through CI, we should technically only list 18.04. If we want to be a bit more lenient, we could add 16.04 and Mac OS X, as we know people how have run successfully on it or are currently running on it. However, this might be a slippery slope, because for what versions of AiiDA does this hold? When do we remove a platform if it has not been tested in a long time. Do we add WSL, since we have some people that have run on it? This for example I don't think we should officially mention.
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.
Well we are mentioning WSL pretty extensively as a "supported installation method". I think we should have a brief team discussion about this at the next meeting and then we can make sure to consistently qualify what is considered a "supported system".
docs/source/intro/installation.rst
Outdated
.. _PyPI: https://pypi.python.org/pypi/aiida-core | ||
.. _github repository: https://github.com/aiidateam/aiida-core | ||
|
||
AiiDA can be installed either from the python package index `PyPI`_, `Conda`_ (both good for general use) or directly from the aiida-core `github repository`_ (good for developers). |
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.
I think I'd prefer to split this into "Install with pip (and venv)" and "Install with conda" instead of mixing the two things like this.
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.
Fixed in 9323e43.
I haven't split into sections, because that doesn't make sense IMO, since you can also install with pip in a Conda environment. Virtual environments are covered in the section above, I've just clarified that it's installing from anaconda cloud and added the console line command
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.
There are limitations to using conda and pip in combination. Especially using conda with venv
is not possible afaik or at least unwise in so far I think that the current structure is sub-optimal. So, while I disagree with you on this matter, I'm ok with leaving this for now, I don't think that this discussion should block this PR.
As a side note: I'd prefer if you could not resolve conversations that are not obviously resolved. Otherwise it is very difficult for me to follow open points for discussion, because I need to manually unhide all resolved conversations to check the status of the discussion.
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.
Well it was resolved IMO 😉, but yeh I understand.
Especially using conda with venv is not possible afaik
pip has nothing to do with venv though:
- you use venv, pipenv, or conda to create your virtual environment, then
- within that virtual environment you use either pip, anaconda cloud (i.e.
conda install
), or git directly to install your python packages.
It would make absolutely no sense to create a venv inside conda, but using pip is absolutely fine (although using anaconda cloud is generally better for handling version dependencies)
Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>
Hi @chrisjsewell , just a quick reminder that we all get emails for each commit made to this PR. So far I have received more than a 100 emails for this PR alone. If possible, it would be nice to try and limit the amount of commits that are pushed |
Sorry @sphuber 😳 I'm basically done on this now! (you know that you can unsubscribe from the PR though 😬 ) |
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.
Just three minor suggestions, but otherwise I think this is ready!
Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>
@sphuber I think you should adjust your notification settings. I've been working with https://github.com/notifications which works very well in staying on top of mentions, PR requests, etc. Receiving a notification (and especially an email) with every commit is unreasonable IMO. @chrisjsewell However, I think we should still try to bundle our commits (pushes) a little bit to reduce the strain on our CI resources. |
Yep absolutely 👍 |
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.
Ready to ship! 👍 Thank's a lot for this! 🎉
Revision of all sections within the first chapter. Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>
closes #4026
closes #4056