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

Make GSD optional #4174

Merged
merged 8 commits into from Jun 23, 2023
Merged

Make GSD optional #4174

merged 8 commits into from Jun 23, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jun 17, 2023

Fixes #3819

Changes made in this Pull Request:

  • Make GSD an optional requirement

PR Checklist

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

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

@IAlibay
Copy link
Member Author

IAlibay commented Jun 17, 2023

Opening this early so I can get some feedback from @MDAnalysis/coredevs - this is probably quite an abrupt change, do we need to have some kind of deprecation window?

@github-actions
Copy link

github-actions bot commented Jun 17, 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/5304029149/jobs/9600062116


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

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (efc51af) 93.60% compared to head (ecabb78) 93.61%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4174   +/-   ##
========================================
  Coverage    93.60%   93.61%           
========================================
  Files          193      193           
  Lines        25147    25168   +21     
  Branches      4056     4058    +2     
========================================
+ Hits         23539    23560   +21     
  Misses        1092     1092           
  Partials       516      516           
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/GSD.py 91.89% <100.00%> (+1.56%) ⬆️
package/MDAnalysis/topology/GSDParser.py 92.85% <100.00%> (+1.36%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@RMeli
Copy link
Member

RMeli commented Jun 17, 2023

I don't think so IMO. It might surprise some users, but the error message is very clear and the solution a pip install away.

@IAlibay IAlibay changed the title [WIP] Make GSD optional Make GSD optional Jun 18, 2023
@IAlibay
Copy link
Member Author

IAlibay commented Jun 18, 2023

Ok this should be good to go I think (unless I forgot to fix some test somewhere). Since it's a core dependency change, I'm going to leave this here for a week for other @MDAnalysis/coredevs to chime in, and if there are no objections it'll get merged.

Specifically pinging @orbeckst here, since we've spent a lot of time pondering in the past on how best to do these types of changes.

Meaningless messages in meaningless commits. Can you make computers
survive aquatic conditions?
I think my keyboard's `t` key is broken, but it's such a nice colour :(
Where do you stand on the Luftballons vs Red Balloons debate?
If it's not the former then you are wrong.
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

LGTM thanks @IAlibay

@orbeckst
Copy link
Member

For reference: conda-forge feedstock was already updated conda-forge/mdanalysis-feedstock#57

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.

I agree.

Eventually it would be good to also allow gsd > 3.0 but we have to be able to test it. In the short term, the restriction is sensible.

@orbeckst
Copy link
Member

@IAlibay I'll let you do the merging (unless you want someone else to do it, then change the assignee). Thanks for working out the problem and the quick-fix.

@joaander
Copy link

I agree.

Eventually it would be good to also allow gsd > 3.0 but we have to be able to test it. In the short term, the restriction is sensible.

Please test GSD 3.0.1.

@orbeckst
Copy link
Member

orbeckst commented Jun 23, 2023

@joaander thanks, we'll check that #4172 is solved with gsd 3.0.1. Will just take a few days (people on holidays etc).

If it works we could blacklist 3.0.0 and otherwise allow gsd!=3.0.0. We'll probably still make it optional as we have been trying to slim down dependencies.

@joaander
Copy link

@orbeckst understood. If 3.0.1 passes your tests, I expect 3.0.0 will work for your users. I find that most users follow PEP8 guidelines and issue imports at the top of the module, not inside child threads.

@IAlibay IAlibay merged commit 07c9d1f into develop Jun 23, 2023
25 checks passed
@IAlibay IAlibay deleted the noncore-gsd branch June 23, 2023 17:44
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.

Make GSD an optional requirement
6 participants