Skip to content

Conversation

@namiroues
Copy link
Contributor

@namiroues namiroues commented Feb 23, 2025

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/

@namiroues namiroues changed the title Sdg23 update user guide Sdg23 update user guide installation instructions Feb 23, 2025
Copy link
Member

@lilyminium lilyminium left a 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.

@namiroues
Copy link
Contributor Author

namiroues commented Mar 4, 2025

Hi @lilyminium, thanks a lot for the comments, I addressed them and I also refined the instructions to have a clearer version.

@namiroues
Copy link
Contributor Author

namiroues commented Mar 4, 2025

Hi @micaela-matta could you please check the changes?

Copy link
Member

@orbeckst orbeckst left a 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.

orbeckst pushed a commit to MDAnalysis/mdanalysis that referenced this pull request Mar 14, 2025
* Fixes #4929
* Removed duplicate installation instructions from API Docs
* related to MDAnalysis/UserGuide#404
@namiroues
Copy link
Contributor Author

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 :).

Copy link
Member

@orbeckst orbeckst left a 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.

@namiroues
Copy link
Contributor Author

Thanks @orbeckst for your comments. I've addressed them.

Copy link
Member

@orbeckst orbeckst left a 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.)

@orbeckst
Copy link
Member

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.

Copy link
Member

@lilyminium lilyminium left a 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!

Copy link
Contributor

@micaela-matta micaela-matta left a 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

@orbeckst orbeckst linked an issue Mar 24, 2025 that may be closed by this pull request
Copy link
Member

@RMeli RMeli left a 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.

@namiroues namiroues force-pushed the sdg23-update-user-guide branch from 4fcc12e to 5ef1085 Compare March 26, 2025 17:16
@orbeckst
Copy link
Member

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.

@namiroues
Copy link
Contributor Author

Thanks @orbeckst and @RMeli for your comments. I addressed the comments and tested the installation instructions. They work for me with conda-forge (so I explicitly added this). Can someone try a fresh installation with mambaforge to double-test this?

@namiroues namiroues requested a review from orbeckst March 28, 2025 19:37
Copy link
Member

@orbeckst orbeckst left a 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.13

Based 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)

Copy link
Member

@orbeckst orbeckst left a 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.

@namiroues
Copy link
Contributor Author

Thanks @orbeckst. I fixed it and tested it myself.

@namiroues namiroues requested a review from orbeckst March 31, 2025 07:36
Copy link
Member

@orbeckst orbeckst left a 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.

@orbeckst orbeckst merged commit 55ca9a3 into MDAnalysis:develop Mar 31, 2025
4 checks passed
@orbeckst
Copy link
Member

Nice work, @namiroues !

Abdulrahman-PROG pushed a commit to Abdulrahman-PROG/mdanalysis that referenced this pull request Apr 13, 2025
…#4927)

* Fixes MDAnalysis#4929
* Removed duplicate installation instructions from API Docs
* related to MDAnalysis/UserGuide#404
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

installation instructions suggest incorrect mamba installation Installation instructions cleanup in User Guide

6 participants