-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add support for conda environments #35
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 4 -1
Lines 107 109 +2
=========================================
+ Hits 107 109 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
61d3d76 to
94a7298
Compare
cca38df to
56ea6ff
Compare
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.
- Do we want to tie this to a specific version of acro ( could understand 'yes, for now')
- This seems to use python 12 (maybe becuase that is what is on condo?), and maybe it is how the file was shown to me, but R-CMD-check.yaml seemed ot refer only to 10 or 13
- there's an error message saying something like "miniconda is not installed". Should that use parenthesis in case people don't understand the relationship I.e. "...(mini) conda is not installed..."
- This does not give people the ability to provide a config file when they call init (line 92). I know it is a slightly different issues, but ...
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.
- It is pinned to Python ACRO v0.4.11 - we have always pinned ACRO-R to a specific Python version for stability; it does mean that the CRAN package needs updating each time the Python backend is updated, but it's safer and allows changes to occur upstream without breaking anything here.
- The Conda Python is set to Python 3.12 because it is probably the most stable version; and since Conda allows specifying a specific version it reduces the number of moving parts. The CI uses Python 3.12 so we only need to test it - hence fewer CI runners needed. If Conda is not used then the virtual environment will use whatever Python is available on the system (>=3.10) as before.
- Changed "miniconda" to just "conda".
- Done.
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.
should we have a test for install_conda ?
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.
It's time consuming to create a single runner that will test both Conda and venv just so the code coverage can be contained. The CI has dedicated runners for Conda now which execute the tests in that environment (except for one specific test that tests installation in a venv that I kept because it was originally there --- I wonder if it's even necessary since if that fails then none of the tests would pass.)
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.
sounds good.
Appreciate your adding the unrelated feature about handling config files
should we add a test to check the custom yaml has been loaded or do that elsewhere?
Add a Boolean
use_condaoption toacro_init()to enable support for conda environments.If no value for
use_condais passed, it checks for an environment variableACRO_USE_CONDA(defaults false if unset) - this is primarily for CI usage, but could also be configured by a sysadmin so that the end user doesn't have to set this variable.Note: this setup assumes pre-installation of Conda (which seems most suited to restricted environments) - reticulate does have an option to
install_miniconda()but that would require Internet access so that has been omitted here.This needs more people to test before merging, but it seems to run locally (and on CI).
Steps:
Manually install miniconda
Accept the miniconda terms
use_conda=TRUEinacro_init()Obviously if this is pushed to CRAN, cloning, use of devtools and local package installation is not needed.