-
Notifications
You must be signed in to change notification settings - Fork 317
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
Remove rpy2 #9886
Remove rpy2 #9886
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9886 +/- ##
==========================================
- Coverage 66.75% 66.73% -0.02%
==========================================
Files 454 454
Lines 71062 71054 -8
Branches 5680 5676 -4
==========================================
- Hits 47435 47420 -15
- Misses 23203 23210 +7
Partials 424 424 ☔ View full report in Codecov by Sentry. |
All images
|
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.
I did some basic tests with this image locally and it seems to work as expected.
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.
We have been withholding tzlocal because of a conflict with rpy2, but I just checked locally and we should be able to update it to 5.2. Do you want to do this in this PR or in our next round of Python updates?
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.
Let's wait til the next round of version upgrades.
# TODO: use standard OS Python installation? The only reason we switched to Conda | ||
# was to support R and `rpy2`, but now that we've removed those, we might not | ||
# get any benefit from Conda. | ||
echo "setting up conda..." | ||
cd / | ||
arch=`uname -m` | ||
curl -LO https://github.com/conda-forge/miniforge/releases/latest/download/Miniforge3-Linux-${arch}.sh | ||
bash Miniforge3-Linux-${arch}.sh -b -p /usr/local -f |
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.
I don't know enough to form a firm opinion on this, but it seems that conda is not used anywhere else, and the Python grader installs its own version of it, so it should be save to remove it.
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.
Discussion continued here: #9886 (comment)
We're going to leave this as-is for now.
@@ -45,45 +45,17 @@ echo "setting up postgres..." | |||
mkdir /var/postgres && chown postgres:postgres /var/postgres | |||
su postgres -c "initdb -D /var/postgres" | |||
|
|||
# TODO: use standard OS Python installation? The only reason we switched to Conda | |||
# was to support R and `rpy2`, but now that we've removed those, we might not | |||
# get any benefit from 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.
One potential advantage of using conda is that we can more easily control the timing of version upgrades, independent of upgrades to the underlying OS. I'm not sure how valuable this is in practice?
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.
AL2023 does support Python 3.11 as a separate package to install: https://docs.aws.amazon.com/linux/al2023/ug/python.html. Of course, I'm happy to stick with Conda for now; I certainly don't want to change that in this PR (since I want to minimize the number of changes here).
The last courses that were using
rpy2
have transitioned to plain Python, so we can finally remove R andrpy2
from our base image and remove the feature flag!