MDAnalysis / UserGuide

User Guide for MDAnalysis
https://userguide.mdanalysis.org
22 stars 33 forks source link

dev: comment and cleanup environment.yml #295

Closed jandom closed 1 year ago

jandom commented 1 year ago

Lots of discussion in https://github.com/MDAnalysis/UserGuide/pull/293 big thanks to @IAlibay and @lilyminium for helping me out.

This PR removes a few packages and adds a lot of comments. It's still possible that there are other unused packages.

The aim of this PR is to make it clearer which dep is needed where, it's still not super granular, and it's still possible that other packages are not needed.

lilyminium commented 1 year ago

I see @IAlibay is sick -- did you still want to re-visit this PR, Irfan? If not I can take over if you dismiss your review or approve changes.

IAlibay commented 1 year ago

As far as I am aware (edit: from looking at my last request changes) there are still four build or core dependencies of MDAnalysis left in the yaml file (matplotlib, networkx, packaging and threadpoolctl). If those are included intentionally then it would be great to document why, if not it would be good to remove them here.

If this PR needs a quick resolution then please go ahead and dismiss my review.

On Mon, Aug 21, 2023, 05:59 Lily Wang @.***> wrote:

I see @IAlibay https://github.com/IAlibay is sick -- did you still want to re-visit this PR, Irfan? If not I can take over if you dismiss your review or approve changes.

— Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/UserGuide/pull/295#issuecomment-1685643797, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7CAXITDXU3EFHY4OSWFSLXWLTMTANCNFSM6AAAAAA3INESSQ . You are receiving this because you were mentioned.Message ID: @.***>

jandom commented 1 year ago

@lilyminium this should be resolved now, re extensions. I still have it in me to try and remove all the other packages that are MDA deps

jandom commented 1 year ago

Sorry a history question... please ignore if it's a time waste... but i'm really curious how all these MDA deps made their way into here?

jandom commented 1 year ago

@IAlibay thanks for the confidence and just nuking those deps – this helped tremendously, I would have been removing them one-by-one, because I don't have the context

jandom commented 1 year ago

@lilyminium PTAL, this seems to be blocked on an approval from you – and i don't think it merits an override (it's nor urgent)

IAlibay commented 1 year ago

Sorry a history question... please ignore if it's a time waste... but i'm really curious how all these MDA deps made their way into here?

They've been around since before we started supporting PEP518, i.e. a world before you could just rely on pip to pick up the right build time dependencies & before we offered wheels. Back then it was safer to have your own deps before you attempted to pip install something.

jandom commented 1 year ago

Thank you for all you reviews 👏

jandom commented 1 year ago

Looking back at this month, we got a lot done – my original aim was to wrap up the snapshot tests and refactor PR, and I think we can do that too. @IAlibay and @lilyminium this wouldn't be possible without your engagement and encouragement, thank you!