Skip to content
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

Update for automatically update ml, ml-verse #192

Merged
merged 17 commits into from
Jul 27, 2021

Conversation

eitsupi
Copy link
Member

@eitsupi eitsupi commented Jun 29, 2021

Make the same changes to ml and ml-verse (cuda10.1) that we made to core images in #164 and #169.

ml is running the same script as core in a different Dockerfile, so I added a new script sync-template-vars.R to copy core's ENVs to ml.
I also moved the scripts for generating Dockerfiles etc. to the new build directory.

ToDo

  • template
  • script
  • GitHubAction definition

@eitsupi eitsupi mentioned this pull request Jun 29, 2021
36 tasks
@eitsupi
Copy link
Member Author

eitsupi commented Jun 29, 2021

@cboettig I need to discuss a few things with you.

  1. When I updated stacks/ml-cuda10.1-devel.json as a template, I didn't know which file to use as a base because the contents of cuda10.1 stack file changed greatly in each version...
    I copied the contents of 4.0.0 to 4.0.4 into devel because they were consistent, but removed the install_tensorflow.sh run judging from tensorflow unusable after install via install_tensorflow.sh #124 (comment).
    If there is a problem with the stacks/ml-cuda10.1-devel.json in e23e556, please let me know. Also, can I remove the execution of install_tensorflow.sh from 4.0.0 to 4.0.4 as well?
$ diff ./stacks/ml-cuda10.1-devel.json ./stacks/ml-cuda10.1-4.0.4.json
4c4
<   "TAG": "devel-cuda10.1",
---
>   "TAG": "4.0.4-cuda10.1",
9c9
<       "FROM": "rocker/r-ver:devel",
---
>       "FROM": "rocker/r-ver:4.0.4",
33c33
<       "FROM": "rocker/r-ver:devel-cuda10.1",
---
>       "FROM": "rocker/r-ver:4.0.4-cuda10.1",
35c35
<         "S6_VERSION": "v2.1.0.2",
---
>         "S6_VERSION": "v1.21.7.0",
45c45,46
<         "/rocker_scripts/install_tidyverse.sh"
---
>         "/rocker_scripts/install_tidyverse.sh",
>         "/rocker_scripts/install_tensorflow.sh"
52c53
<       "FROM": "rocker/ml:devel-cuda10.1",
---
>       "FROM": "rocker/ml:4.0.4-cuda10.1",
60,61c61,62
<       "FROM": "rocker/ml:devel-cuda10.1",
<       "TAG": "devel"
---
>       "FROM": "rocker/ml:4.0.4-cuda10.1",
>       "TAG": "4.0.4"
65,66c66,67
<       "FROM": "rocker/ml-verse:devel-cuda10.1",
<       "TAG": "devel"
---
>       "FROM": "rocker/ml-verse:4.0.4-cuda10.1",
>       "TAG": "4.0.4"
  1. In ml and ml-verse, RSTUDIO_VERSION and CTAN_REPO are currently not fixed, unlike core. Is it OK to change them to be fixed?
  2. Since there are more script files to generate Dockerfile, etc., can I move these and .github/workflows/buildmatrix to the "build" directory (or some other name)?
    It would be easier to understand the automated build system if we put README.md in the directory and write a description about the scripts.

@cboettig
Copy link
Member

cboettig commented Jul 1, 2021

Great work here, thanks.

On (1): yeah, install_tensorflow.sh was dropped as per the issue going forward but maintained in previously 'frozen' images for historical consistency. I'm not sure that's the right choice; since I'm not sure we got the python version-locking entirely in place in that script before it was retired.

In general the need to support tensorflow 1.x for some applications and tensorflow 2.x for others isn't fully resolved, especially when CUDA support is required. (tensorflow 1.x with CUDA requires CUDA 10.0 toolbox libs, which can be installed ontop the cuda10.1 images with this script: https://github.com/rocker-org/rocker-versioned2/blob/tensorflow-1.x/scripts/install_tf1_cuda_10_0.sh. tensorflow 1.x also requires python < 3.8, we added pyenv+pipenv mechanism to switch between python versions: https://github.com/rocker-org/ml/#python-versions-and-virtualenvs, but I may be the only one who knows/uses that currently!)

re 2: yes, good catch, they should be fixed, mimicking core. (Again, ideally we wouldn't have redundant code in the first place, though the automation goes some distance to fixing that...)

re 3: yes, 💯! Would be wonderful to package up and properly document the current build system and associated scripts. (and cc @noamross )

@eitsupi
Copy link
Member Author

eitsupi commented Jul 9, 2021

Sorry for the late reply.

re 2: yes, good catch, they should be fixed, mimicking core. (Again, ideally we wouldn't have redundant code in the first place, though the automation goes some distance to fixing that...)

I agree with you.
I'll try this scripted automation as I couldn't think of a better way, but it would be great to achieve something more sophisticated.

@eitsupi eitsupi force-pushed the update-for-autoupdate-ml branch from a92d0c1 to c99da7c Compare July 9, 2021 15:20
@eitsupi
Copy link
Member Author

eitsupi commented Jul 9, 2021

Since markdownlint's list item prefix was set to allow only 1. by default, I disabled this rule MD029.
https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md#md029---ordered-list-item-prefix

Perhaps it would be better to change the check tool to markdownlint-cli to make it easier to write the configuration file...

@eitsupi eitsupi marked this pull request as ready for review July 10, 2021 15:33
@eitsupi
Copy link
Member Author

eitsupi commented Jul 10, 2021

@cboettig I've finished editing and would appreciate your review.
The following files in particular have been rewritten quite a bit, but I have not used these images and do not understand their contents, so please check them carefully.

  • dockerfiles/Dockerfile_r-ver_4.0.5-cuda10.1
  • dockerfiles/Dockerfile_r-ver_4.1.0-cuda10.1
  • dockerfiles/Dockerfile_r-ver_devel-cuda10.1

The newly added build/README.md contains only the minimum description for now.
I'll open the other PR later to enrich the contents. (And, it would be a good idea to put README.md in each folder compose, dockerfiles, stacks, to make it easier to understand which files are automatically generated and not edited manually.)

@cboettig
Copy link
Member

okay, finally had a chance to run through this, looks good to me. here we go!

@cboettig cboettig merged commit b6b6430 into rocker-org:master Jul 27, 2021
@eitsupi eitsupi deleted the update-for-autoupdate-ml branch July 28, 2021 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants