-
Notifications
You must be signed in to change notification settings - Fork 646
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
base: develop
Are you sure you want to change the base?
Conversation
Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-11-14 11:01:19 UTC |
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.
Please have a look at the Please note: The |
Codecov ReportAttention:
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. |
@IAlibay please ping me when you need a review. |
@orbeckst first impressions would be good - is this too complicated, too unpythonic, etc? |
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.
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: |
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.
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( |
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.
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.
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. |
Removed the |
@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) |
Ah. Fair point. For organisational purposes, I would be in favour of enabling all CI on drafts PR, to encourage their use. |
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. |
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). |
This might be what I'm remembering 😅https://github.com/MicrosoftDocs/azure-devops-docs/issues/13175 |
Yep, sorry about that. I was trying to avoid #4361 from repeating. |
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:
TODO:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4345.org.readthedocs.build/en/4345/