-
Notifications
You must be signed in to change notification settings - Fork 45
Sdg23 update user guide installation instructions #404
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
Sdg23 update user guide installation instructions #404
Conversation
lilyminium
left a comment
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.
Awesome work @namiroues! I've just left some non-blocking recommendations to separate links or maybe use refs to streamline the text a bit.
|
Hi @lilyminium, thanks a lot for the comments, I addressed them and I also refined the instructions to have a clearer version. |
|
Hi @micaela-matta could you please check the changes? |
orbeckst
left a comment
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.
Please see comments inline, sorry to be brief.
* Fixes #4929 * Removed duplicate installation instructions from API Docs * related to MDAnalysis/UserGuide#404
|
Thanks @orbeckst for your comments. I've updated the installation instructions. I also updated other parts that were outdated. Please have a look. I am sure that you will have some more comments for me :). |
orbeckst
left a comment
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.
Great rewrite.
I found a few things that occurred to me (and which are generally not that obvious). Thank you for updating this page, it's been high time for an update.
|
Thanks @orbeckst for your comments. I've addressed them. |
orbeckst
left a comment
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.
Last check before approving: Have you followed the installation instructions and can you confirm that they work?
(I had minor changes but I added them myself from suggestions.)
|
The pre-commit hook check is failing for me. I am just editing on GitHub so I can't easily see where the trailing whitespace is. Can you please run it locally through your pre-commit hook to fix it, @namiroues ? Thanks. |
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 haven't tested the installation instructions myself, but otherwise LGTM! Thanks for all your work @namiroues, looks excellent!
micaela-matta
left a comment
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 second @orbeckst's request to double check installation instructions, but other that that, LGTM! thanks @namiroues
RMeli
left a comment
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.
Thanks @namiroues, looks very good! I left some additional comments.
4fcc12e to
5ef1085
Compare
|
The RTD build failed because a package had the wrong checksum. I assume that this is an intermittent error (which is out of our control) and it will work the next time you push to this PR. |
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.
Minor changes inline, the instructions themselves seem good, but not everything works. I kept notes for future follow-up.
Notes from testing on macOS x86_64
❌ mamba Python 3.13.2
I tried in an existing conda/mamba installation. By default it created a Python 3.13.2 environment. MDAnalysis installed fine:
>>> python -c "import MDAnalysis; print(MDAnalysis.__version__)"
2.9.0
However, MDAnalysisTests has issues for me:
mamba install -c conda-forge MDAnalysisTests
gives
Getting conda-forge osx-64
Getting conda-forge noarch
Looking for: ['mdanalysistests', 'python 3.13.2']
509175 packages in https://conda.anaconda.org/conda-forge/osx-64
259522 packages in https://conda.anaconda.org/conda-forge/noarch
Encountered problems while solving.
Problem: nothing provides mdanalysis 0.15.0 needed by mdanalysistests-0.15.0-py27_0
I have no idea why it wants to install 0.15.
Then I try pinning
mamba install -c conda-forge "MDAnalysisTests=2.9.0"
and that fails with
Looking for: ['mdanalysistests 2.9.0.*', 'python 3.13.2']
509175 packages in https://conda.anaconda.org/conda-forge/osx-64
259522 packages in https://conda.anaconda.org/conda-forge/noarch
Encountered problems while solving.
Problem: package mdanalysistests-2.9.0-pyhd8ed1ab_0 requires python >=3.9,<=3.13, but none of the providers can be installed
The mdanalysistests condaforge recipe contains https://github.com/conda-forge/mdanalysistests-feedstock/blob/c9c9a496f8172b2bc9a5c7571092e2ab2f1f8c2f/recipe/meta.yaml#L25
- python >=3.9,<=3.13Based on the package match specification this seems to be the issue because it will not match Python 3.13.2.
I raised conda-forge/mdanalysistests-feedstock#51, which has already been fixed. (So need to test with conda -forge packages ...)
✅ mamba Python 3.12
mamba create -n mdanalysis -c conda-forge mdanalysis python=3.12
mamba activate mdanalysis
mamba install -c conda-forge MDAnalysisTests
Tests in parallel
pytest --disable-pytest-warnings -n 2 -v --pyargs MDAnalysisTests
were a bit wonky for me. Deadlocked once around 87%, second time it happened I started killing python3.12 processes and then suddenly tests continued to run (although possibly only in one runner). Tried again
pytest --disable-pytest-warnings -n 4 -v --pyargs MDAnalysisTests
succeeded (installs 2.9.0 and tests pass).
============ 19714 passed, 363 skipped, 7 xfailed, 2 xpassed, 107840 warnings in 749.94s (0:12:29) ============
✅ , ❌ pip 3.13
Create mamba env with python 3.13
mamba create -n mdapip313 python=3.13 pip
✅ standard
and installed with
pip install --upgrade MDAnalysis
pip install --upgrade MDAnalysisTests
Tests
pytest --disable-pytest-warnings -n 2 -v --pyargs MDAnalysisTests
passed
============ 19014 passed, 781 skipped, 6 xfailed, 2 xpassed, 89945 warnings in 962.55s (0:16:02) =============
❌ Installing extras
pip install --upgrade MDAnalysis[analysis,extra_formats,parallel]
failed because pytng did not build
...
pytng/pytng.c:2398:22: error: implicit declaration of function '_PyList_Extend' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
PyObject* none = _PyList_Extend((PyListObject*)L, v);
^
...
pytng/pytng.c:2436:39: error: implicit declaration of function '_PyInterpreterState_GetConfig' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
__pyx_assertions_enabled_flag = ! _PyInterpreterState_GetConfig(__Pyx_PyThreadState_Current->interp)->optimization_level;
^
pytng/pytng.c:2436:107: error: member reference type 'int' is not a pointer
__pyx_assertions_enabled_flag = ! _PyInterpreterState_GetConfig(__Pyx_PyThreadState_Current->interp)->optimization_level;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
...
pytng/pytng.c:34856:13: error: implicit declaration of function '_PyUnicode_FastCopyCharacters' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
_PyUnicode_FastCopyCharacters(result_uval, char_pos, uval, 0, ulength);
^
...
pytng/pytng.c:38679:70: error: too few arguments to function call, expected 6, have 5
is_little, !is_unsigned);
^
...
pytng/pytng.c:34856:13: error: implicit declaration of function '_PyUnicode_FastCopyCharacters' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
_PyUnicode_FastCopyCharacters(result_uval, char_pos, uval, 0, ulength);
^
...
pytng/pytng.c:38679:70: error: too few arguments to function call, expected 6, have 5
is_little, !is_unsigned);
^
...
#define Py_EXPORTED_SYMBOL __attribute__ ((visibility ("default")))
^
pytng/pytng.c:39895:13: error: implicit declaration of function '_PyGen_SetStopIterationValue' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
_PyGen_SetStopIterationValue(result);
^
...
22 warnings and 9 errors generated.
error: command '/usr/bin/clang' failed with exit code 1
[end of output]
...
Failed to build pytng
ERROR: Failed to build installable wheels for some pyproject.toml based projects (pytng)
orbeckst
left a comment
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.
Source installation instructions do not work as written.
Please fix/clarify.
|
Thanks @orbeckst. I fixed it and tested it myself. |
orbeckst
left a comment
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.
Good rewrite, all perfect from my end.
|
Nice work, @namiroues ! |
…#4927) * Fixes MDAnalysis#4929 * Removed duplicate installation instructions from API Docs * related to MDAnalysis/UserGuide#404
This PR updates the installation instructions in the User Guide as part of the SDG documentation cleanup.
This PR is linked to MDAnalysis/mdanalysis#4927
This PR fixes #402 and #405
📚 Documentation preview 📚: https://mdanalysisuserguide--404.org.readthedocs.build/en/404/