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

Custom tutorial notebooks #3

Merged
merged 13 commits into from
May 11, 2021
Merged

Custom tutorial notebooks #3

merged 13 commits into from
May 11, 2021

Conversation

ChaamC
Copy link
Collaborator

@ChaamC ChaamC commented Apr 20, 2021

New PR for the custom tutorial notebooks solution.
Adds jq/yq installation which is required to run the deploy-data-specific-image script from birdhouse-deploy repo.
Please refer to the other PR for more details

New solution that tries to solve what was tried to be done on this earlier PR.

Copy link

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ChaamC ChaamC requested review from matprov and dbyrns April 23, 2021 19:16
@ChaamC
Copy link
Collaborator Author

ChaamC commented Apr 23, 2021

Updated : deploy script and related env file added to the repo, and removed from birdhouse-deploy

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really close here. Only simplication is needed. All the pieces seem to be here.

#############

DEPLOY_DATA_JOB_SCHEDULE="${AUTODEPLOY_NOTEBOOK_FREQUENCY}"
DEPLOY_DATA_JOB_JOB_NAME="notebookdeploy-${DEPLOY_DATA_JOB_JUPYTERHUB_IMAGE_NAME}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest, to make it simple, merge your birdhouse/components/scheduler/deploy_data_specific_image_job.env with this one because you are rolling a custom cronjob generation code to match deploy_data_specific_image anyways.

I have a two-part deploy_data_job.env generic part and an specific override because it works for many jobs. Yours work only for 1 specific job, might as well make it simple and have everything in this file, including the looping part so the activation in env.local is very simple for users.

@ChaamC
Copy link
Collaborator Author

ChaamC commented May 4, 2021

Updated PR :

  • moved the env file from birdhouse-deploy to this repo
  • added a dest_sub_dir parameter to specify a subdirectory destination where to download the data

@ChaamC ChaamC requested review from tlvu and matprov May 4, 2021 17:12
Copy link

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also perfect here!

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for me as well, except the part about the notebook dest dir.

Also, you've manually enabled this on a real PAVICS deployment and see it works properly?

scheduler-jobs/deploy_data_pavics_jupyter.env Outdated Show resolved Hide resolved
@@ -9,6 +9,9 @@
# - repo_url: path to the github repo (ex.: https://github.com/crim-ca/pavics-jupyter-images)
# branch: name of the branch containing the required version of the data
# source_path: path of the desired file or folder in the source repo
# dest_sub_dir: Data is sent to a tutorial-notebooks directory with the same name as the image running the script.
# Use this parameter to specify a subdirectory path (without a prefix '/') in this directory where the data will be sent.
# If the parameter is absent or left empty, data is sent directly to the image's tutorial-notebooks folder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So before this dest_sub_dir fix, given crim-eo image, the notebooks would have been deployed at the root /data/jupyterhub_user_data/tutorial-notebooks/ instead of /data/jupyterhub_user_data/tutorial-notebooks/crim-eo?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No the host path mount inside the image is /data/jupyterhub_user_data/tutorial-notebooks/<image_name> so for your example /data/jupyterhub_user_data/tutorial-notebooks/crim-eo/
The sub_dir only allows to checkout multiple repository, for exemple :
common notebooks with a dest_sub_dir "common" will be checkout to /data/jupyterhub_user_data/tutorial-notebooks/crim-eo/common
eo notebooks with a dest_sub_dir "eo" will be checkout to /data/jupyterhub_user_data/tutorial-notebooks/crim-eo/eo
If dest_sub_dir stay empty they will be checkout to /data/jupyterhub_user_data/tutorial-notebooks/crim-eo/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, thanks.

scheduler-jobs/deploy_data_pavics_jupyter.env Outdated Show resolved Hide resolved
@ChaamC
Copy link
Collaborator Author

ChaamC commented May 4, 2021

@tlvu

Also, you've manually enabled this on a real PAVICS deployment and see it works properly?

It worked on a local setup, but I will try to do a test with the iac too eventually.

@matprov
Copy link
Collaborator

matprov commented May 4, 2021

..but I will try to do a test with the iac too eventually.

@ChaamC Eventually like before merging this PR? :)

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good for me with the last code update.

@ChaamC ChaamC merged commit 8c9cd23 into master May 11, 2021
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.

None yet

5 participants