BLD, MAINT: hoist packaging import by tylerjereddy · Pull Request #3527 · MDAnalysis/mdanalysis

tylerjereddy · 2022-02-13T00:42:12Z

hoist the import of packaging outside of a Cython-focused
try/except block to avoid a deceptive error message when
packaging is missing, per discussion at:
BLD: comply with PEP518 #3398 (comment)
we may also want to consider removing the external dependency
completely and vendoring the pertinent code since it is so small, but
maybe not this close to a release; an example:
https://github.com/scipy/scipy/blob/main/scipy/_lib/_pep440.py

PR Checklist

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

* hoist the import of `packaging` outside of a Cython-focused `try`/`except` block to avoid a deceptive error message when `packaging` is missing, per discussion at: MDAnalysis#3398 (comment) * we may also want to consider removing the external dependency completely and vendoring the pertinent code since it is so small, but maybe not this close to a release; an example: https://github.com/scipy/scipy/blob/main/scipy/_lib/_pep440.py

codecov · 2022-02-13T00:58:19Z

Codecov Report

Merging #3527 (1a31cfe) into develop (3b7590e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3527   +/-   ##
========================================
  Coverage    94.10%   94.10%           
========================================
  Files          189      189           
  Lines        24596    24596           
  Branches      3305     3305           
========================================
  Hits         23146    23146           
  Misses        1404     1404           
  Partials        46       46

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b7590e...1a31cfe. Read the comment docs.

IAlibay · 2022-02-13T01:28:29Z

package/setup.py

    import Cython
    from Cython.Build import cythonize
    cython_found = True
-    from packaging.version import Version


So thinking about it a bit more, this explicitly makes packaging a core requirement for user builds right?

I guess the question is - where do we stand on eventually making Cython a core dependency (rather than us shipping the C/C++ files)? If we want to keep things the way we usually do, then I think we should possibly try/except around the packaging import and just set it to either say that either cython or packaging isn't found?

So thinking about it a bit more, this explicitly makes packaging a core requirement for user builds right?

What? I just moved the import so the error message would be better when packaging is absent--it was the MDA team who added this dependency months ago.

IAlibay · 2022-02-13T13:56:41Z

@MDAnalysis/coredevs please review - this will technically add a dependency. I don't personally think packaging is that big that it will cause issues (it gets pulled in by one or more of our core dependencies anyways).

jbarnoud · 2022-02-13T16:25:06Z

When is packaging installed?

IAlibay · 2022-02-13T16:29:17Z

When is packaging installed?

So my understanding is that if you pip install it'll pull it in as a build dependency (based on the project.toml), otherwise on a python setup.py call you'll need to have it installed (i.e. it'll need to be under requires in setup.py).

I'm not sure which, but one of our core dependencies already pulls it in so we had added it in without ever really caring too much until now.

IAlibay · 2022-02-13T19:10:23Z

Poking around a little bit, seems like we've already bought into using packaging internally as a core dependency.

For example:

mdanalysis/package/MDAnalysis/coordinates/chemfiles.py

Line 84 in 3b7590e

from packaging.version import Version

I think we should just move ahead with putting packaging down as both a setup_requires and a install_requires entry?

jbarnoud · 2022-02-13T19:29:27Z

If we imply it anyway, we should have it explicit.

tylerjereddy · 2022-02-13T23:11:01Z

please review - this will technically add a dependency.

Huh? MDAnalysis added the packaging dependency months ago, I'm not adding a dependency here, I'm just making the error message a bit less useless when packaging is absent.

IAlibay · 2022-02-13T23:21:45Z

please review - this will technically add a dependency.

Huh? MDAnalysis added the packaging dependency months ago, I'm not adding a dependency here, I'm just making the error message a bit less useless when packaging is absent.

@tylerjereddy - yeah you're right, but the problem we face is that packaging was "added" implicitly rather than explicitly in #3499 (if that makes seinse - I only realised this in #3527 (comment)).

Sorry about the confusing/confused messages, essentially my final point is that we probably should reflect this dependency in setup.py?

tylerjereddy · 2022-02-14T00:26:55Z

Sorry about the confusing/confused messages, essentially my final point is that we probably should reflect this dependency in setup.py?

Honestly, it looks like the team accidentally added a runtime dependency and not just a build time dependency, which is a bit more serious, even for the average consumer of a pre-built binary for example.

Given the increased gravity of a runtime dependency addition, and one that wasn't really discussed with the team IIUC, I'd probably vote for vendoring the module I mention above, which may also have the added benefit of avoiding chicken-and-egg problems with needing an external package to make (versioning) decisions about other external packages in the setup code.

I'm not sure that any of the possible decisions really make a difference to this PR though--regardless of where the machinery comes from, and what else you tell setup.py to do, the import for version handling still seems appropriately placed at the top-level, where the default ImportError would probably be clear enough without the Cython error message confusion.

It looks like there may have been an intention to make packaging a build time dependency only when building a development branch, but not for the sdist official releases placed on PyPI. This is rather subtle though--you can only get away with that as long as you only need version checking on Cython and nothing else and always have pre-built Cython in the sdist. I suppose you could still make packaging an optional build-time dependency for git checkouts only, and a mandatory run-time dependency, which is the strongest argument I can see against this PR, but I doubt you're gaining much if it is already a mandatory runtime dependency anyway.

So, it still seems to me like this is one of the cases where vendoring a small chunk of external code may actually be the right choice to avoid some head-spinning dependency issues, and then a top-level import should always be safe anyway.

tylerjereddy · 2022-02-14T01:09:57Z

One other point is that I'm not sure why we have such a low minimum version of Cython in setup.py--I'd say we should feel free to bump that up quite a bit since it is a build time dependency only and even at the very base of the ecosystem in NumPy the minimum version is way higher: https://github.com/numpy/numpy/blob/main/pyproject.toml#L8

Plus I've already set Cython>=0.28 in pyproject.toml. Do we support the 3.x series already? These may be more appropriate in separate issues...

jbarnoud · 2022-02-14T07:39:42Z

I agree that the vendoring of version is out of the scope of that PR. I opened #3532 to track that specific issue.

The cython version issue is likely also a separate issue. I opened #3533 about it.

jbarnoud

I approve the PR but wait for @IAlibay to give a green light as well before merging. Let's try to limit the scope of each PR.

IAlibay · 2022-02-22T18:01:00Z

Honestly, it looks like the team accidentally added a runtime dependency and not just a build time dependency, which is a bit more serious, even for the average consumer of a pre-built binary for example.

Yeah I think this is on me as the approver of that PR (I forgot packaging wasn't a built-in package).

Here's what I propose we do:

1 - In this PR add packaging as a listed setup.py + pyproject.toml setup_requires and install_requires
2 - Raise an issue to vendor packaging in the future.

I realise this is an issue that happened before this PR, but if we can fix it now we can just push out 2.1.0.

Does that sound sensible @tylerjereddy ?

IAlibay

putting a temporary halt on merge whilst we discuss next steps

IAlibay · 2022-03-02T21:58:15Z

Honestly, it looks like the team accidentally added a runtime dependency and not just a build time dependency, which is a bit more serious, even for the average consumer of a pre-built binary for example.

Yeah I think this is on me as the approver of that PR (I forgot packaging wasn't a built-in package).

Here's what I propose we do:

1 - In this PR add packaging as a listed setup.py + pyproject.toml setup_requires and install_requires 2 - Raise an issue to vendor packaging in the future.

I realise this is an issue that happened before this PR, but if we can fix it now we can just push out 2.1.0.

Does that sound sensible @tylerjereddy ?

We need to get this moving so I'll merge this and follow up with a PR that explicitly adds packaging as a dependency.

* Related to #3527

tylerjereddy added maintainability installation labels Feb 13, 2022

IAlibay reviewed Feb 13, 2022

View reviewed changes

This was referenced Feb 14, 2022

BLD, MAINT: Add packaging.version to the repo #3532

Open

Update the minimum cython version in setup.py #3533

Open

jbarnoud approved these changes Feb 14, 2022

View reviewed changes

IAlibay mentioned this pull request Feb 22, 2022

Release 2.1.0 #3446

Closed

5 tasks

IAlibay requested changes Feb 22, 2022

View reviewed changes

IAlibay approved these changes Mar 2, 2022

View reviewed changes

IAlibay merged commit acd929f into MDAnalysis:develop Mar 2, 2022

IAlibay mentioned this pull request Mar 2, 2022

Add packaging as a core dependency of MDAnalysis #3549

Merged

2 tasks

AnirG mentioned this pull request Mar 3, 2022

Accept AtomGroup alongwith Universe in the DistanceMatrix #3541

Merged

4 tasks

lilyminium pushed a commit that referenced this pull request Mar 3, 2022

Add packaging as a core dependency of MDAnalysis (#3549)

4509856

* Related to #3527

Conversation

tylerjereddy commented Feb 13, 2022

PR Checklist

Uh oh!

codecov bot commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IAlibay Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

tylerjereddy Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

IAlibay commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbarnoud commented Feb 13, 2022

Uh oh!

IAlibay commented Feb 13, 2022

Uh oh!

IAlibay commented Feb 13, 2022

Uh oh!

jbarnoud commented Feb 13, 2022

Uh oh!

tylerjereddy commented Feb 13, 2022

Uh oh!

IAlibay commented Feb 13, 2022

Uh oh!

tylerjereddy commented Feb 14, 2022

Uh oh!

tylerjereddy commented Feb 14, 2022

Uh oh!

jbarnoud commented Feb 14, 2022

Uh oh!

jbarnoud left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay commented Feb 22, 2022

Uh oh!

IAlibay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IAlibay commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 13, 2022 •

edited

Loading

IAlibay commented Feb 13, 2022 •

edited

Loading

IAlibay left a comment •

edited

Loading