Skip to content

Conversation

@mefuller
Copy link
Member

Motivation or Problem

Writing the documentation to only reference Anaconda is inflexible and bloated as compared with utilizing Miniconda or Conda available in system repositories. This is of particular relevance of storage-limited systems, such as shared high-performance resources where RMG is often installed (which also frequently run RHEL or another Enterpise Linux which includes conda in the repositories, easing administrative burden).

Description of Changes

  • Use repositories over Ananconda for proper locations, updates
  • switch to Miniconda to reduce download and installation size by 10x
  • replace deprecated source with conda in activating environments

Testing

Environments created and RMG-Py, RMG-database installed and tested on Fedora and EL systems

Copy link
Contributor

@sevyharris sevyharris left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think it should be ready to go after the two tabs are added so the HTML builds properly.

@JacksonBurns
Copy link
Contributor

Something like this came up in #2437 - the Docker image and CI use miniconda, which does not follow the developer installation instructions. For the same reasons listed in this PR, this should be changed. I am going to resolve the merge conflicts and get this ready to merge, since the same points made are still relevant. @sevyharris could you review after?

@JacksonBurns
Copy link
Contributor

Also adding that almost all of the content that was originally in this PR (like conda activate instead of source activate) has trickled in over time via other PRs.

@mefuller
Copy link
Member Author

So you want me to tidy it up?

@JacksonBurns
Copy link
Contributor

So you want me to tidy it up?

Nope, there was only one conlict, which I resolved. The rest of the changes were already present, so they did not raise conflicts when updating. Thanks for checking in though!

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #2246 (353d769) into main (770a8c8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2246   +/-   ##
=======================================
  Coverage   48.13%   48.13%           
=======================================
  Files         110      110           
  Lines       30626    30626           
  Branches     7988     7988           
=======================================
  Hits        14743    14743           
  Misses      14353    14353           
  Partials     1530     1530           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rwest
Copy link
Member

rwest commented May 24, 2023

I fixed the issues Sevy noted, and added some more edits.
Looks good to me, but as many of the changes are now mine, we should have someone else review it too.
(fwiw I did build the documentation locally, which for me required some hacks described here, and it rendered OK in HTML)

mefuller and others added 3 commits May 26, 2023 11:22
…nda for proper locations, updates; switch to Miniconda to reduce download and installation size by 10x; replace deprecated "source" with "conda"
Whitespace is crucial, a bit like in Python.
One needs to actually compile in sphinx and look at the output to be sure.
Make a sub-list for different package managers.
Tweak the MacOS instructions wording.
@rwest rwest enabled auto-merge May 26, 2023 19:00
@rwest rwest requested a review from JacksonBurns May 26, 2023 19:00
Copy link
Contributor

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

LGTM!

@rwest rwest merged commit 40b5558 into main May 26, 2023
@rwest rwest deleted the mef_install branch May 26, 2023 23:26
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.

5 participants