dev: comment and cleanup environment.yml #295
Conversation
There was a problem hiding this comment.
Thanks @jandom for reorganising this file! I had some comments on the conceptual grouping. I'm not sure organising by notebook file is easiest to understand and maintain, especially since there's nothing stopping us from using e.g. scikit-image in another notebook and forgetting to update this. Instead, I've suggested ways to document by overall purpose.
environment.yml
Outdated
| - moviepy | ||
| - ipyvtklink | ||
| # used in conf.py |
There was a problem hiding this comment.
I mostly think of these as sphinx "extensions" -- we use the ipython directive in some docs, nbsphinx to render notebooks, etc. If they're imported/used in conf.py it would generally be in their role as extensions.
There was a problem hiding this comment.
Thanks, yeah, easy to change
There was a problem hiding this comment.
Thanks @jandom -- could you please move these up to the #extensions then?
There was a problem hiding this comment.
Just this to go -- nbsphinx, ipywidgets and the like are used to render notebooks, and IIRC ipython to use the ipython sphinx directive. If they're imported in conf.py it's to configure settings but, otherwise they fall into the same category as other extensions, e.g. sphinx bibtex.
environment.yml
Outdated
| - widgetsnbextension | ||
| - ipycytoscape>=1.3.0 | ||
| - python-graphviz | ||
| # used in multiple notebooks |
There was a problem hiding this comment.
I would label this section generally extensions instead. nbconvert especially I would assume is not called by any notebook explicitly but to download notebooks, or in nbsphinx.
There was a problem hiding this comment.
Thank you these changes were easy to apply
environment.yml
Outdated
| # used in pairwise_rmsd.ipynb | ||
| - plotly |
There was a problem hiding this comment.
As we edit or update notebooks, I can see the notebook-specific labels getting outdated quite easily. Instead, could we maybe organise these by purpose? e.g. plotly, ipygany, pyvista, scikit-image, seaborn are for visualising/plotting; moviepy and nglview are for visualising/making movies of molecules; dask and distributed you could switch the heading to # parallelization.
There was a problem hiding this comment.
Changed the grouping here as well, easy to apply
There was a problem hiding this comment.
Hi @jandom, thank you for making the changes! Just a couple things would be nice to re-arrange and then I think we're good to go.
environment.yml
Outdated
| # used by custom_parallel_analysis.ipynb | ||
| - graphviz | ||
| - tabulate | ||
| - widgetsnbextension | ||
| - ipycytoscape>=1.3.0 | ||
| - python-graphviz |
There was a problem hiding this comment.
Could you move this down to #visualisation?
environment.yml
Outdated
| - moviepy | ||
| - ipyvtklink | ||
| # used in conf.py |
There was a problem hiding this comment.
Thanks @jandom -- could you please move these up to the #extensions then?
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
environment.yml
Outdated
| - moviepy | ||
| - ipyvtklink | ||
| # used in conf.py |
There was a problem hiding this comment.
Just this to go -- nbsphinx, ipywidgets and the like are used to render notebooks, and IIRC ipython to use the ipython sphinx directive. If they're imported in conf.py it's to configure settings but, otherwise they fall into the same category as other extensions, e.g. sphinx bibtex.
|
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. |
|
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
<#295 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC7CAXITDXU3EFHY4OSWFSLXWLTMTANCNFSM6AAAAAA3INESSQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@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 |
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
|
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? |
|
@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 |
|
@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) |
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. |
There was a problem hiding this comment.
Sorry for the wait -- thanks for the changes @jandom and for all your patience!
|
Thank you for all you reviews 👏 |
|
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! |
Lots of discussion in #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.