custom notebooks are added to the image #3

ChaamC · 2021-03-30T21:38:19Z

This PR adds the feature for the customization of included tutorial-notebooks.

The notebooks for the specific images are included when building the images. All notebooks are saved directly in this repo, in the folder related to their image.

This PR relates to the PR in pavics-jupyter-images : bird-house/pavics-jupyter-base#2

dbyrns

LGTM, but we need to take care of bird-house/pavics-jupyter-base#2 before merging this one.

tlvu

I am surprised but this is CRIM image. I leave it to you guys.

tlvu · 2021-04-02T04:46:09Z

eo/Dockerfile

+
+# Add a copy of the eo notebooks to the docker image
+# All tutorial-notebooks are set with jenkins as owner, so they can be manipulated by jenkins in the docker entrypoint
+COPY --chown=jenkins notebooks/ /notebook_dir/tutorial-notebooks


I am surprised notebooks are copied directly into the image. The the other associated PR bird-house/pavics-jupyter-base#2 you just wrote "avoid having to rebuild the image just to update a notebook".

I thought I would see this child image leverage the new feature in the base image.

Adding to what I said here : bird-house/pavics-jupyter-base#2 (comment), we looked for an easy way to add notebooks to the ones provide in the base image. We agree that having a custom image with static notebooks is the bare minimum, but it fits our needs right now. We do not expect a lot of modifications in a near future and until we build new images weekly, having another repository just for some notebooks, seems overkill. Adding a wget in this image is in the plan given that the notebooks start to be updated more frequently.

ChaamC · 2021-04-19T19:49:53Z

PR has been updated for a new solution. Please refer to the other PR for more details.

@matprov @dbyrns @tlvu

dbyrns

Some requests based on the main PR comments.

dbyrns · 2021-04-20T14:22:37Z

eo/Dockerfile

@@ -15,5 +15,7 @@ RUN umask 0000 && conda env update -f /environment.yml
 # (https://github.com/conda-forge/gdal-feedstock/issues/83#issue-162911573)
 ENV CPL_ZIP_ENCODING=UTF-8

+COPY notebook_config.yml.template /notebook_config.yml.template


Ok like I said, I can now really see that we use the template as is, so it's not a template. One cannot expect some variables to be replaced like in the birdhouse-deploy config folder.

dbyrns · 2021-04-20T14:24:32Z

eo/notebook_config.yml.template

+
+- repo_url: https://github.com/crim-ca/pavics-jupyter-images
+  branch: master
+  source_path: eo/notebooks


That's very neat. I hadn't thought to reuse the same repo both for the image and notebooks.

dbyrns · 2021-04-20T14:25:57Z

eo/notebook_config.yml.template

+- repo_url: https://github.com/crim-ca/pavics-jupyter-images
+  branch: master
+  source_path: eo/notebooks
+  dest_dir: ${JUPYTERHUB_USER_DATA_DIR}/tutorial-notebooks/eo-crim


You see here, what could prevent an image maintainer to cause trouble and set the dest_dir to ${JUPYTERHUB_USER_DATA_DIR}? (See : bird-house/birdhouse-deploy#150 (comment))

dbyrns · 2021-04-20T14:26:11Z

nlp/Dockerfile

@@ -9,5 +9,8 @@ COPY environment.yml /environment.yml
 # use umask 0000 so that package files for the updated environment are usable by the user for the jupyter-conda-extension
 RUN umask 0000 && conda env update -f /environment.yml

+COPY notebook_config.yml.template /notebook_config.yml.template


dbyrns · 2021-04-20T14:26:21Z

nlp/notebook_config.yml.template

+- repo_url: https://github.com/crim-ca/pavics-jupyter-images
+  branch: master
+  source_path: nlp/notebooks
+  dest_dir: ${JUPYTERHUB_USER_DATA_DIR}/tutorial-notebooks/nlp-crim


ChaamC · 2021-04-23T19:16:06Z

Updated repo : removed dest_dir parameter from the script config

tlvu

+1 for me here, just minor comment.

eo/notebook_config.yml

ChaamC · 2021-05-04T17:13:35Z

Updated PR :

added a dest_sub_dir parameter to specify a subdirectory destination where to download the data. Parameter is optional

dbyrns

Last little thing : I would like to see the common repository included here for both eo/nlp and a confirmation that both repos are keep synchronized for each images.

tlvu

LGTM. A remark about dest_sub_dir but I am not sure.

tlvu · 2021-05-04T17:43:33Z

eo/CHANGELOG.md

@@ -12,6 +12,18 @@ Fixes:
 ------
 - ...

+0.2.0 (2021-03-30)


finalize the date I guess.

tlvu · 2021-05-04T17:44:20Z

eo/notebook_config.yml

+- repo_url: https://github.com/crim-ca/pavics-jupyter-images
+  branch: master
+  source_path: eo/notebooks
+  dest_sub_dir:


I would expect dest_sub_dir: crim-eo? But I am not sure.

@tlvu see my comment here : bird-house/pavics-jupyter-base#3 (comment)
Like this, the notebooks would be checkout to : /data/jupyterhub_user_data/tutorial-notebooks/crim-eo
What I asked to @ChaamC would make it clearer to show how to achieve the following strucutre:
/data/jupyterhub_user_data/tutorial-notebooks/crim-eo/eo
/data/jupyterhub_user_data/tutorial-notebooks/crim-eo/common

I can add the suggested structure.
But what would be the repo_url/branch/source_path for the common folder?
Should I try to find all the notebooks that are found in the tutorial-notebooks folder (when spawning the pavics-workflow image for example)?
Or maybe I can just add this folder for now?

I thought that the current tutorial-notebooks folder was filled using only that folder. But looking at the diff, this is enough for our common folder. Maybe we could call it "core"?

Yes, there are more in the current tutorial-notebooks folder, so more sources are necessary if we want them all. But I don't think all those notebooks will work anyway. The eo image for example has only the base packages and the specific packages for eo. The packages necessary for the current tutorial-notebooks folder are not all on the eo image.

Even with the linked folder in my previous comment, I am not sure which notebooks will work or not from that list.

So, do I just take the linked folder for now, and store it in a 'core' folder, for both eo/nlp?

Yes go for it. At least for people using those images, they will see what PAVICS has to offer and the mecanism will be demonstrated. If some libraries are missing we will fix that later.

FYI info if you guys want to replicate exactly what in the current tutorial-notebooks folder into your common folder https://github.com/bird-house/birdhouse-deploy/blob/20d440fd293b01d4cf3e1dccf2933877a651d571/birdhouse/deployment/trigger-deploy-notebook#L60-L71

However, keep in mind that bringing in notebook that your image can not run (missing packages) is no cool ! I'd suggest keeping only the notebooks relevant to your image runtime capabilities.

With that in mind, pavics-sdi is a good start (probably most notebooks will run without missing packages). Finch and Raven notebooks will requires more packages to be installed.

However, keep in mind that bringing in notebook that your image can not run (missing packages) is no cool !

I agree, but it's more as a place holder or demonstration capabilities right now. I think that once this setup is ready we will be able to better partitionize the notebooks for all our (Ouranos and CRIM) images.

Common folder has been added to configs :)

tlvu · 2021-05-05T14:24:53Z

nlp/notebook_config.yml

+  source_dir: nlp/notebooks
+  dest_sub_dir: nlp
+
+- repo_url: https://github.com/Ouranosinc/pavics-sdi


Ouch double download of the pavics-sdi notebooks. At least the config is clearer.

We could have one config.yml that will handle all the notebooks repos (eo, nlp, common) but then each image won't be "independent" anymore.

Don't have a quick solution at the moment.

@tlvu, at least it's one copy per image and not per user. Hopefully, the common notebooks folder will stay small.

We could have one config.yml that will handle all the notebooks repos (eo, nlp, common) but then each image won't be "independent" anymore.

That's the whole point of this PR! Having a single config.yml is about the same as what we have right now.

The download script could be optimized (not for this PR) and keep a cache of each repo independently of the current image. So that while looping over the images, if a repo has already been updated, it uses the cache.

But again I'm note sure of your concern, is it the download or the disk space? In both case it looks like trying to save pennies.

Opps sorry, sending wrong impression again. Wasn't saying this is a deal breaker, else I would change the approval status.

Probably the layout on disk could be

/data/jupyterhub_user_data/tutorial-notebooks/common /data/jupyterhub_user_data/tutorial-notebooks/crim-eo /data/jupyterhub_user_data/tutorial-notebooks/crim-nlp

Instead of

/data/jupyterhub_user_data/tutorial-notebooks/crim-eo/eo /data/jupyterhub_user_data/tutorial-notebooks/crim-eo/common /data/jupyterhub_user_data/tutorial-notebooks/crim-nlp/nlp /data/jupyterhub_user_data/tutorial-notebooks/crim-nlp/common

But that also assume the same definition of common for those images, which might make sense only if on the same deployment node. Like you said, it's pennies at this point so let's do this once we get there.

Don't worry I'm not offended and sorry if this is the impression I leave.

Just to be clear, I like all trade-offs/limitations to be clearly documented in a PR. Doesn't mean we need to address all of them immediately. I much prefer iterative approach where we improve gradually than keep adding feature to grow the PR and never release it!

custom notebooks are added to the image

c982034

ChaamC requested review from matprov and dbyrns March 30, 2021 21:38

ChaamC mentioned this pull request Mar 30, 2021

Custom tutorial notebooks bird-house/pavics-jupyter-base#2

Closed

ChaamC requested a review from tlvu March 31, 2021 12:16

matprov approved these changes Mar 31, 2021

View reviewed changes

dbyrns approved these changes Apr 1, 2021

View reviewed changes

tlvu reviewed Apr 2, 2021

View reviewed changes

cwcummings added 3 commits April 12, 2021 16:48

added new config file for notebook deployment

8fb6d48

add config to nlp image

3553e9f

update doc

f5d8561

ChaamC mentioned this pull request Apr 19, 2021

Custom notebooks bird-house/birdhouse-deploy#150

Merged

ChaamC requested review from dbyrns, matprov and tlvu April 19, 2021 19:50

dbyrns suggested changes Apr 20, 2021

View reviewed changes

cwcummings added 4 commits April 21, 2021 15:01

remove dest_dir config parameter and remove template suffix

0e68050

update readme

9efeda7

fix README

9aaccb2

test notebook update

2cd4f38

ChaamC requested a review from dbyrns April 23, 2021 19:16

tlvu approved these changes Apr 26, 2021

View reviewed changes

eo/notebook_config.yml Show resolved Hide resolved

add dest_sub_dir parameter in config

fa208b3

ChaamC requested a review from tlvu May 4, 2021 17:13

dbyrns approved these changes May 4, 2021

View reviewed changes

tlvu approved these changes May 4, 2021

View reviewed changes

add common folder to configs

a3aa1b4

tlvu reviewed May 5, 2021

View reviewed changes

cwcummings added 4 commits May 11, 2021 12:32

bump version dates

12de0f9

fix to avoid dockerhub warning problem

b8c3b08

format example notebooks

25efbba

use new base version

55f4d55

ChaamC merged commit f865380 into master May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom notebooks are added to the image #3

custom notebooks are added to the image #3

ChaamC commented Mar 30, 2021

dbyrns left a comment

tlvu left a comment

tlvu Apr 2, 2021

dbyrns Apr 6, 2021

ChaamC commented Apr 19, 2021 •

edited

Loading

dbyrns left a comment

dbyrns Apr 20, 2021

dbyrns Apr 20, 2021

dbyrns Apr 20, 2021

dbyrns Apr 20, 2021

dbyrns Apr 20, 2021

ChaamC commented Apr 23, 2021

tlvu left a comment

ChaamC commented May 4, 2021

dbyrns left a comment

tlvu left a comment

tlvu May 4, 2021

tlvu May 4, 2021

dbyrns May 4, 2021

ChaamC May 4, 2021

dbyrns May 4, 2021

ChaamC May 4, 2021

dbyrns May 4, 2021

tlvu May 4, 2021

dbyrns May 4, 2021

ChaamC May 5, 2021

tlvu May 5, 2021

dbyrns May 5, 2021

tlvu May 5, 2021

dbyrns May 5, 2021

tlvu May 5, 2021

custom notebooks are added to the image #3

custom notebooks are added to the image #3

Conversation

ChaamC commented Mar 30, 2021

dbyrns left a comment

Choose a reason for hiding this comment

tlvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC commented Apr 19, 2021 • edited Loading

dbyrns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC commented Apr 23, 2021

tlvu left a comment

Choose a reason for hiding this comment

ChaamC commented May 4, 2021

dbyrns left a comment

Choose a reason for hiding this comment

tlvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC commented Apr 19, 2021 •

edited

Loading