Update for external mdahole2 mdakit #4464

IAlibay · 2024-02-23T06:14:08Z

Fixes #4414

Changes made in this Pull Request:

Switches all imports in the hole2 analysis module to use the mdakit mdahole2.

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

📚 Documentation preview 📚: https://mdanalysis--4464.org.readthedocs.build/en/4464/

github-actions · 2024-02-23T06:16:37Z

Linter Bot Results:

Hi @IAlibay! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/8043270736/job/21965086091

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2024-02-24T06:51:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.66%. Comparing base (3a5339c) to head (af1b08d).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4464      +/-   ##
===========================================
+ Coverage    93.38%   93.66%   +0.28%     
===========================================
  Files          171      180       +9     
  Lines        21744    22270     +526     
  Branches      4014     3902     -112     
===========================================
+ Hits         20305    20859     +554     
- Misses         952      959       +7     
+ Partials       487      452      -35

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

RMeli

LGTM. The only (non-blocking) question I have is: it is really worth it to add an additional external dependency for something that is deprecated?

package/MDAnalysis/analysis/hole2/__init__.py

RMeli · 2024-02-24T20:55:32Z

package/doc/sphinx/source/documentation_pages/analysis/hole2.rst

+  This module is deprecated in favour of the mdakit
+  `mdahole2 <https://www.mdanalysis.org/mdahole2/>`_ and
+  will be removed in MDAnalysis 3.0.0.


Suggested change

This module is deprecated in favour of the mdakit

`mdahole2 <https://www.mdanalysis.org/mdahole2/>`_ and

will be removed in MDAnalysis 3.0.0.

This module is deprecated in favour of the mdakit

`mdahole2 <https://www.mdanalysis.org/mdahole2/>`_ and

will be removed in MDAnalysis 3.0.0.

Is this indentation correct?

I think it doesn't matter all that too much with sphinx, but I've added it, thanks!

RMeli · 2024-02-24T20:56:37Z

package/pyproject.toml

@@ -44,6 +44,7 @@ dependencies = [
    'mda-xdrlib',
    'waterdynamics',
    'pathsimanalysis',
+    'mdahole2',


This is going to be annoying to package upstream, but it's a "me" problem.

(EDIT: This comment refers to @RMeli 's comment of downstream pain in packaging)

Because you need to make hole2 available in spack?

I thought so, but looking at mdahole2's dependencies it does not look like hole2 is provided, is it? So packaging mdahole2 would be easier, although I don't think it would make much sense to provide a downstream Spack package without hole2 TBH.

We have a hole2 cf package https://anaconda.org/conda-forge/hole2 for Linux and osx — just not for Windows.

We didn't include it as a dependency because ... well, because we only got the conda-forge package recently and used the reasoning for the pip package (see also MDAnalysis/mdahole2#25 ) that people should be able to use their own hole2 installation (as it worked previously).

See MDAnalysis/mdahole2#55

Just to clarify the current situation here:

PyPi mdahole2 / pure pip install - has no hole2 dependency. This is because we can't distribute hole2 over pypi at the moment.

conda-forge mdahole2-base: has no hole2 dependency - this exists for all the os & architecture types where hole2 can't be used. The combination of MDAnalysis & mdahole2-base effectively replicates the current status of the MDA core library where we ask folks to install hole2 themselves.

conda-forge mdahole2: for x86_64 linux and macos this is mdahole2-base + hole2

Thanks for explaining. I won't resolve the comment, just so that it remains visible.

IAlibay · 2024-02-25T02:26:33Z

Thanks @RMeli

Re: extra dependencies - unfortunately this is a temporary pain to avoid another one. The reasons for importing things directly rather than keeping the old code around are 1) to avoid having to back port a bunch of stuff (happened a few times already for hole2), 2) to get folks used to the idea of the mdakit - i.e. "you've been using them all along", is better than "ok starting from now you have to trust this instead".

From a packaging perspective, I have only been thinking about pypi and conda-forge, sorry :( How much work is it elsewhere? I would have hoped that folks were just pulling from pypi for pure python projects.

IAlibay · 2024-02-25T02:30:17Z

I'm going to cc @orbeckst @fiona-naughton @lilyminium into this discussion.

My opinion here is very likely biased since I've spent a decent chunk of time on the mdakit migrations. However, we should make sure that everyone is happy with the plan to add in these temporary core dependencies.

Co-authored-by: Rocco Meli <r.meli@bluemail.ch>

RMeli · 2024-02-25T09:22:08Z

If the module is deprecated we don't need to back port stuff IMO, we can just point to the MDAKit if users have problems (which the code itself already does).

Packaging upstream is not too hard, but you still need to write the package file etc. I should do it anyways for the MDAKits we own.

However, my concern here is adding an external dependency when we know we will remove it. It looks like the worst option, the other two being (1) deprecate but leave the old code until removal (without backporting fixes and improvements) or (2) leave the MDAKit as an external dependency, i.e. use the MDAKit code but don't deprecate the module.

IAlibay · 2024-02-25T12:58:19Z

If the module is deprecated we don't need to back port stuff IMO, we can just point to the MDAKit if users have problems (which the code itself already does).

I disagree, it's deprecated but we still need to ensure that folks get the right outcomes, i.e. it's deprecated for us not for them. The code here pointing to the new kit is actually a mistake - when we originally created mdahole2 we hadn't packaged it, so we included a deprecation notice but didn't replace the imports.

Packaging upstream is not too hard, but you still need to write the package file etc. I should do it anyways for the MDAKits we own.

That would be useful if you want to do it, but it could also fall under the scope of someone working in EOSS4 if it's time you'd rather not spend on this task.

However, my concern here is adding an external dependency when we know we will remove it. It looks like the worst option, the other two being (1) deprecate but leave the old code until removal (without backporting fixes and improvements) or (2) leave the MDAKit as an external dependency, i.e. use the MDAKit code but don't deprecate the module.

I think the issue here is that you end up with a mess of fixes that are done mostly here and need to get moved to mdahole2. Then we keep facing having to tell folks "no actually do this change over there", to which folks will go "but I'm not using that thing I'm using the core library" or if it's ourselves "but not fixing this here means that we don't get to support Y in the core library".

A good example of this is doing numpy 2.0 fixes. MDA 3.0 will likely come out after numpy 2.0. If we want to support it then we have to make relevant fixes here and then also in the kit. It's not excessively problematic, but it does have a time cost.

w.r.t. not deprecating - that's in the territory of roadmap 3.0 stuff and I'm not sure I have a strong enough opinion.

orbeckst

Thanks for doing all the work @IAlibay .

I agree with #4464 (comment) that we need to bite the bullet and increase dependencies while we're still shipping a thin version of mdahole2 — it's on us to make it work while we're going through deprecation, not on our users. Sorry @RMeli — I understand the desire to make clean breaks but that seems too user-unfriendly.

package/CHANGELOG

maintainer/conda/environment.yml

orbeckst · 2024-02-25T17:42:02Z

package/pyproject.toml

@@ -44,6 +44,7 @@ dependencies = [
    'mda-xdrlib',
    'waterdynamics',
    'pathsimanalysis',
+    'mdahole2',


(EDIT: This comment refers to @RMeli 's comment of downstream pain in packaging)

Because you need to make hole2 available in spack?

RMeli · 2024-02-25T20:49:34Z

deprecate but leave the old code until removal (without backporting fixes and improvements)

To clarify my comment, which was poorly phrased, I was not advocating to leave the code go bad. I was thinking of just not backporting fixes or enhancement from the MDAKit to change the behaviour (i.e. if there was a bug pre-deprecation, it would stay during deprecation). As a user I'd expect deprecated code to work as at time of deprecation (bugs included).

However, I was not aware of big changes that would mean a lot of work, which would incur in high maintenance costs (and are therefore not viable/acceptable).

w.r.t. not deprecating - that's in the territory of roadmap 3.0 stuff and I'm not sure I have a strong enough opinion.

Yeah, I'm just wondering if it's really worth removing a thin wrapper to an MDAKit once it's already in core.

…ole2

IAlibay · 2024-02-26T03:31:38Z

However, I was not aware of big changes that would mean a lot of work, which would incur in high maintenance costs (and are therefore not viable/acceptable).

I wouldn't say "high maintenance cost" - porting fixes across the core library & kits isn't a big thing in isolation (couple of lines here and there), but it becomes an extra thing to think about / do when you already have a tiny number of maintainers. A couple half hours here and there and the cost starts really adding up.

IAlibay · 2024-02-26T03:32:31Z

I'll leave this PR up for a little while longer so that folks get to discuss a bit more - we can merge when everyone is happy.

update for external mdahole2 mdakit

8c69fc0

github-actions bot added the Component-Analysis label Feb 23, 2024

put deprecation notice at top of docs

4ca51f1

github-actions bot added the Continuous Integration label Feb 23, 2024

update stuff

6b87c45

IAlibay added 2 commits February 24, 2024 06:57

remove hole duecredit test entries

99df88f

update changelog

046d353

IAlibay marked this pull request as ready for review February 24, 2024 06:59

Merge branch 'develop' into mdahole2

c4cc736

RMeli approved these changes Feb 24, 2024

View reviewed changes

Update package/MDAnalysis/analysis/hole2/__init__.py

68d2b3b

Co-authored-by: Rocco Meli <r.meli@bluemail.ch>

orbeckst approved these changes Feb 25, 2024

View reviewed changes

IAlibay added 2 commits February 26, 2024 03:26

address review comments

28170c3

Merge branch 'mdahole2' of github.com:MDAnalysis/mdanalysis into mdah…

af1b08d

…ole2

RMeli merged commit d366921 into develop Mar 4, 2024
24 checks passed

RMeli deleted the mdahole2 branch March 4, 2024 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for external mdahole2 mdakit #4464

Update for external mdahole2 mdakit #4464

IAlibay commented Feb 23, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 24, 2024 •

edited

Loading

RMeli left a comment

RMeli Feb 24, 2024

IAlibay Feb 26, 2024

RMeli Feb 24, 2024

orbeckst Feb 25, 2024 •

edited

Loading

RMeli Feb 25, 2024

orbeckst Feb 25, 2024

orbeckst Feb 25, 2024

orbeckst Feb 25, 2024

IAlibay Feb 26, 2024

orbeckst Feb 26, 2024

IAlibay commented Feb 25, 2024

IAlibay commented Feb 25, 2024

RMeli commented Feb 25, 2024

IAlibay commented Feb 25, 2024

orbeckst left a comment

orbeckst Feb 25, 2024 •

edited

Loading

RMeli commented Feb 25, 2024

IAlibay commented Feb 26, 2024

IAlibay commented Feb 26, 2024

Update for external mdahole2 mdakit #4464

Update for external mdahole2 mdakit #4464

Conversation

IAlibay commented Feb 23, 2024 • edited Loading

PR Checklist

Developers certificate of origin

github-actions bot commented Feb 23, 2024 • edited Loading

Linter Bot Results:

codecov bot commented Feb 24, 2024 • edited Loading

Codecov Report

RMeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Feb 25, 2024

IAlibay commented Feb 25, 2024

RMeli commented Feb 25, 2024

IAlibay commented Feb 25, 2024

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

RMeli commented Feb 25, 2024

IAlibay commented Feb 26, 2024

IAlibay commented Feb 26, 2024

IAlibay commented Feb 23, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 24, 2024 •

edited

Loading

orbeckst Feb 25, 2024 •

edited

Loading

orbeckst Feb 25, 2024 •

edited

Loading