-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add Workbench for Google Cloud Workstations image #564
Conversation
9091a54
to
01f2c68
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.
Most of my comments are geared toward attempting to reduce the size of the image layers where possible.
I may have missed some places where we can do this, especially if they were buried in a script.
Also, is there an R package cache we can remove after packages are installed like I suggested for pip
? I don't know as much about managing packages there.
Note: I did not do a thorough review of the configuration files
Setting as blocked by #583. The issue has been escalated to Google and we are awaiting their response. |
Use RSW version for test tag
Co-authored-by: Benjamin R. J. Schwedler <[email protected]>
- Purge pip caches after install steps - Purge apt caches where missing - Consolidate COPY steps
c6fe98f
to
0400594
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.
I think this is fine. I left some comments about things to talk about, but I don't think any immediate change is needed.
RSW_NAME=rstudio-workbench | ||
PYTHON_VERSION=3.10.12 | ||
PYTHON_VERSION_ALT=3.9.17 | ||
PYTHON_VERSION_JUPYTER=3.10.12 |
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.
This confused me, as I thought that it was a third installation of python. Why are we explicitly setting this? Why isn't it just always presumed to be installed at PYTHON_VERSION?
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.
This matches the pattern of the other Workbench products. I personally have no problem defaulting to one version or the other, but we should probably make it a global change for the repo. It would make more sense to default the Jupyter install to the primary or alt version. I feel like this only causes problems for users, though I doubt many people will build directly from this project.
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.
@@ -0,0 +1,2 @@ | |||
CRAN=https://packagemanager.posit.co/cran/__linux__/focal/latest |
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 seems like we should be leaving this as the real CRAN URL, but making RSPM take priority. This is not especially a question about this PR, though, so perhaps it is best discussed elsewhere.
&& rm -rf /var/lib/apt/lists/* | ||
|
||
### Install R versions ### | ||
RUN curl -O https://cdn.rstudio.com/r/ubuntu-2004/pkgs/r-${R_VERSION}_1_amd64.deb \ |
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 wonder if it might be nicer to write a proper bash file here, and then use a loop for these duplicated lines. I'm ambivalent. It would be extra work. It would make the Dockerfile easier to read and update. It might make it easier to run/debug scripts outside of docker builds.
Up to you!
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.
Thanks Mike. I think this could be useful, and I agree with your later comment about making this a follow-up issue so this can be addressed across multiple 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.
&& rm -f /usr/lib/rstudio-server/bin/license-manager | ||
|
||
### Install Jupyter and extensions ### | ||
RUN /opt/python/"${PYTHON_VERSION_JUPYTER}"/bin/python -m venv /opt/python/jupyter \ |
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.
A lot of this stuff has me feeling like we should have centralized scripts for this stuff. We must be duplicating a ton of this stuff between this and the other workstations image. I don't think it's important to consolidate it all here in this PR, but perhaps it is worth filing a tech debt ticket for.
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.
&& rm -rf /var/lib/apt/lists/* | ||
|
||
### Install R versions ### | ||
RUN curl -O https://cdn.rstudio.com/r/ubuntu-2004/pkgs/r-${R_VERSION}_1_amd64.deb \ |
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.
Thanks Mike. I think this could be useful, and I agree with your later comment about making this a follow-up issue so this can be addressed across multiple images.
LANGUAGE=en_US:en | ||
LC_ALL=en_US.UTF-8 | ||
JobType: any | ||
Environment: PATH=/opt/python/3.9.16/bin:$PATH |
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 still need this? This caused some issues on the DataOps platform, especially because it was pre-pended to the path.
If we do, we need to update this to the appropriate patch version.
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 have no clue! I noticed that these files tend to get wildly out of date in ever project that uses them. If we should keep it and update it, I'm happy to. If we can remove it, I would also be happy.
Adds the image definition for the initial release of Workbench for Google Cloud Workstations