Skip to content
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

Towards a better optional package system #4345

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Nov 14, 2023

This was brought up a while ago by @orbeckst - how do we deal with all these optional dependencies when we need version ranges.

This aim of this PR is eventually to clean up the optional dependency approach by centralising version checks in one location.

Changes made in this Pull Request:

  • Adds an optional_import method that either returns None or the package
  • Adds an optional "check the version range"

TODO:

  • Switch everything over to using this new method

PR Checklist

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

Developers certificate of origin


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

@pep8speaks
Copy link

pep8speaks commented Nov 14, 2023

Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 2607:9: E129 visually indented line with same indent as next logical line

Comment last updated at 2023-11-14 11:01:19 UTC

Copy link

github-actions bot commented Nov 14, 2023

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/6862668366/job/18660873330


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

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f4005db) 93.36% compared to head (970a897) 93.35%.

Files Patch % Lines
package/MDAnalysis/lib/util.py 77.77% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4345      +/-   ##
===========================================
- Coverage    93.36%   93.35%   -0.02%     
===========================================
  Files          170      184      +14     
  Lines        22325    23448    +1123     
  Branches      4079     4082       +3     
===========================================
+ Hits         20844    21890    +1046     
- Misses         963     1039      +76     
- Partials       518      519       +1     

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

@orbeckst
Copy link
Member

@IAlibay please ping me when you need a review.

@IAlibay
Copy link
Member Author

IAlibay commented Nov 15, 2023

@orbeckst first impressions would be good - is this too complicated, too unpythonic, etc?

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.

Really good start, see inline comments.

@@ -1070,7 +1069,7 @@ def sequence_alignment(mobile, reference, match_score=2, mismatch_penalty=-1,
Biopython is now an optional dependency which this method requires.

"""
if not HAS_BIOPYTHON:
if biopython is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics of if not HAS_BIOPYTHON are clearer.

The following is extra code

def has_optional_dependency(mod):
    return mod is not None

but may be more readable:

from MDAnalysis.lib.util import optional_import, has_optional_dependency

if not has_optional_dependency(biopython):
   # do the thing when we don't have biopython

@@ -2552,3 +2556,59 @@ def wrapper(self, *args, **kwargs):
self._kwargs[key] = arg
return func(self, *args, **kwargs)
return wrapper


def optional_import(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a separate lib.optional module? It's a well-characterized problem so I'd prefer than throwing even more into util. We can then also have better/more focused docs to go along with it.

@orbeckst
Copy link
Member

As long as this is in "WIP" I won't be assigning anyone to the PR. Let me know if you need another set of eyes.

@RMeli RMeli changed the title [WIP] Towards a better optional package system Towards a better optional package system Dec 16, 2023
@RMeli RMeli marked this pull request as draft December 16, 2023 17:10
@RMeli
Copy link
Member

RMeli commented Dec 16, 2023

As long as this is in "WIP" I won't be assigning anyone to the PR.

Removed the WIP label and converted to draft, to avoid confusion.

@IAlibay
Copy link
Member Author

IAlibay commented Dec 16, 2023

@RMeli iirc not all of CI is triggered in draft PRs, need to double check but enforcing draft PR might be a bit of an issue (especially here where I need to ensure we get triggers across all OS types)

@RMeli
Copy link
Member

RMeli commented Dec 16, 2023

Ah. Fair point. For organisational purposes, I would be in favour of enabling all CI on drafts PR, to encourage their use.

@IAlibay
Copy link
Member Author

IAlibay commented Dec 16, 2023

Iirc the issue is with the azure trigger, or maybe it was cirrus - definitely something that was out of our control at the time. Generally if gh actions runs that ok for drafts, but here + py3.12 that's a problem.

@IAlibay
Copy link
Member Author

IAlibay commented Dec 16, 2023

I can have a proper check on what's up after I'm done with other internal MDA tasks - maybe it's not a thing anymore (especially if it's cirrus), but for now I would suggest not being too stringent unless we really have to (especially if I end up having to turn this back to WIP).

@IAlibay
Copy link
Member Author

IAlibay commented Dec 16, 2023

This might be what I'm remembering 😅https://github.com/MicrosoftDocs/azure-devops-docs/issues/13175

@RMeli
Copy link
Member

RMeli commented Dec 17, 2023

Yep, sorry about that. I was trying to avoid #4361 from repeating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants