Jenkins: make it possible to run notebooks from external repos programmatically #138
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| #!/bin/sh | ||
| # This file can be used both as executable script or library to be sourced. | ||
| # To use as library to be sourced, set DOWNLOADREPOS_AS_LIB=1 env var. | ||
|
|
||
| downloadrepos() { | ||
| github_repo="$1"; shift | ||
|
|
@@ -13,25 +15,98 @@ downloadgithubrepos() { | |
| repo_owner="`echo "$owner_and_repo_name" | sed "s@/.*\\$@@g"`" | ||
| repo_name="`echo "$owner_and_repo_name" | sed "s@^.*/@@g"`" | ||
| repo_branch="$1"; shift | ||
| # clean up other previously downloaded branches of the same repo as well | ||
| rm -rf ${repo_name}-* | ||
| ls | grep $repo_name | ||
| downloadrepos https://github.com/$repo_owner/$repo_name "$repo_branch" | ||
| ls | grep $repo_name | ||
| } | ||
|
|
||
| # USAGE: VAR_TO_LOWER="$(lowercase "$VAR_TO_LOWER")" | ||
| lowercase() { | ||
| echo "$1" | tr '[:upper:]' '[:lower:]' | ||
| } | ||
|
|
||
| lowercase_boolean_build_params() { | ||
| TEST_MAGPIE_AUTH="$(lowercase "$TEST_MAGPIE_AUTH")" | ||
| TEST_PAVICS_SDI_REPO="$(lowercase "$TEST_PAVICS_SDI_REPO")" | ||
| TEST_PAVICS_SDI_WEAVER="$(lowercase "$TEST_PAVICS_SDI_WEAVER")" | ||
| TEST_FINCH_REPO="$(lowercase "$TEST_FINCH_REPO")" | ||
| TEST_PAVICS_LANDING_REPO="$(lowercase "$TEST_PAVICS_LANDING_REPO")" | ||
| TEST_RAVEN_REPO="$(lowercase "$TEST_RAVEN_REPO")" | ||
| TEST_RAVENPY_REPO="$(lowercase "$TEST_RAVENPY_REPO")" | ||
| TEST_ESGF_COMPUTE_API_REPO="$(lowercase "$TEST_ESGF_COMPUTE_API_REPO")" | ||
| TEST_LOCAL_NOTEBOOKS="$(lowercase "$TEST_LOCAL_NOTEBOOKS")" | ||
| } | ||
|
|
||
| # Replace all slash (/) by dash (-) because (/) is illegal in folder name | ||
| # for branch name of the format "feature/my_wizbang-feature". | ||
| # Github does the same when downloading repo archive by downloadrepos above. | ||
| # USAGE: export BRANCH_NAME="$(sanitize_branch_name "$BRANCH_NAME")" | ||
| sanitize_branch_name() { | ||
| echo "$1" | sed "s@/@-@g" | ||
| } | ||
|
|
||
| # Ex: extract 'pavics-sdi' from 'Ouranosinc/pavics-sdi'. | ||
| # USAGE: REPO_NAME_ONLY="$(extract_repo_name "$REPO_NAME")" | ||
| extract_repo_name() { | ||
| echo "$1" | sed "s@^.*/@@g" | ||
| } | ||
|
|
||
| # Branches that have allowed characters such as '+' other than alphanum, '-', '_' and '.' are converted to '-' in archives. | ||
| # USAGE: FOLDER_NAME="$(sanitize_extracted_folder_name "$FOLDER_NAME")" | ||
| sanitize_extracted_folder_name() { | ||
| echo "$1" | sed "s@[^a-zA-Z0-9_\-\.]@-@g" | ||
| } | ||
|
|
||
| # Presence of setup.cfg, tox.ini, pyproject.toml files confuse py.test execution rootdir discovery. | ||
| # USAGE: delete_files_confusing_pytest "$CHECKOUT_DIR" | ||
| delete_files_confusing_pytest() { | ||
| for afile in setup.cfg tox.ini pyproject.toml; do | ||
| if [ -f "$1/$afile" ]; then | ||
| rm -v "$1/$afile" | ||
| fi | ||
| done | ||
| } | ||
|
There was a problem hiding this comment. If the repository defined pytest options, such as necessary plugins or marker definitions, this will break their execution. There was a problem hiding this comment. I agree this is weird. I have no other hypothesis other can because I run multiple notebooks from multiple repos at the same time, if each repo has their config, it breaks the full test run. There was a problem hiding this comment. In that case we should be running a separate I agree with @fmigneault that there could be crucial definitions in the There was a problem hiding this comment. Is it possible those pytest definitions are for the unit test but here we are running notebook tests so they do not apply? Because for the current list of repos hardcoded in this Jenkins config, missing those pytest definitions are absolutely fine since forever. Keeping them are actually causing trouble. The step to remove those files delete_files_confusing_pytest.
|
||
|
|
||
| downloadrepos_main() { | ||
| . ./default_build_params | ||
|
|
||
| lowercase_boolean_build_params | ||
|
|
||
| if [ -z "$DOWNLOAD_ALL_DEFAULT_REPOS" ]; then | ||
| # Back-compat with old default behavior, used in binder/reorg-notebook | ||
| # and other external scripts that autodeploy tutorial notebooks (see | ||
| # https://github.com/bird-house/birdhouse-deploy/blob/444a7c35a31aa8ad351e47f659383ba5c2919705/birdhouse/deployment/trigger-deploy-notebook#L64-L75) | ||
| DOWNLOAD_ALL_DEFAULT_REPOS=true | ||
| fi | ||
|
Comment on lines
+67
to
+72
There was a problem hiding this comment. I think I would rather have the other script set There was a problem hiding this comment. No, that's the point. If the other scripts have to set this new I want no changes to other scripts outside this repo. There was a problem hiding this comment. Instead, every CI needs to inject the new variable to take advantage of more intelligent download rather than getting it for free. Since birdhouse-deploy does There was a problem hiding this comment.
Huh, the only one is this Jenkins or more precisely the
Using exact commit hash is like pinning everything in your |
||
|
|
||
| if [ -z "$1" ]; then | ||
| if [ x"$DOWNLOAD_ALL_DEFAULT_REPOS" = xtrue ] || [ x"$TEST_PAVICS_SDI_REPO" = xtrue ]; then | ||
| downloadgithubrepos $PAVICS_SDI_REPO $PAVICS_SDI_BRANCH | ||
| fi | ||
| if [ x"$DOWNLOAD_ALL_DEFAULT_REPOS" = xtrue ] || [ x"$TEST_FINCH_REPO" = xtrue ]; then | ||
| downloadgithubrepos $FINCH_REPO $FINCH_BRANCH | ||
| fi | ||
| if [ x"$DOWNLOAD_ALL_DEFAULT_REPOS" = xtrue ] || [ x"$TEST_PAVICS_LANDING_REPO" = xtrue ]; then | ||
| downloadgithubrepos $PAVICS_LANDING_REPO $PAVICS_LANDING_BRANCH | ||
| fi | ||
| if [ x"$DOWNLOAD_ALL_DEFAULT_REPOS" = xtrue ] || [ x"$TEST_RAVEN_REPO" = xtrue ]; then | ||
| downloadgithubrepos $RAVEN_REPO $RAVEN_BRANCH | ||
| fi | ||
| if [ x"$DOWNLOAD_ALL_DEFAULT_REPOS" = xtrue ] || [ x"$TEST_RAVENPY_REPO" = xtrue ]; then | ||
| downloadgithubrepos $RAVENPY_REPO $RAVENPY_BRANCH | ||
| fi | ||
| if [ x"$DOWNLOAD_ALL_DEFAULT_REPOS" = xtrue ] || [ x"$TEST_ESGF_COMPUTE_API_REPO" = xtrue ]; then | ||
| downloadgithubrepos $ESGF_COMPUTE_API_REPO $ESGF_COMPUTE_API_BRANCH | ||
| fi | ||
| else | ||
| set -x | ||
| downloadrepos "$@" | ||
| fi | ||
| } | ||
|
|
||
| if [ -z "$DOWNLOADREPOS_AS_LIB" ]; then | ||
| # Script mode, not library mode. | ||
| downloadrepos_main "$@" | ||
| fi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| #!/bin/sh | ||
| # | ||
| # Sample Jenkins params override script to demonstrate running new notebooks | ||
| # from an external repo and on-the-fly CONFIG_OVERRIDE_SCRIPT_URL file creation. | ||
| # | ||
| # This script is intended for param CONFIG_PARAMETERS_SCRIPT_URL. | ||
|
|
||
| # Scenario: we want to run notebooks from an external repo, unknown to current Jenkins config. | ||
| # https://github.com/roocs/rook/tree/master/notebooks/*.ipynb | ||
|
|
||
| # Disable all existing default repos to avoid downloading them and running them. | ||
| TEST_PAVICS_SDI_REPO="false" | ||
| TEST_FINCH_REPO="false" | ||
| TEST_PAVICS_LANDING_REPO="false" | ||
| TEST_LOCAL_NOTEBOOKS="false" | ||
|
|
||
| # Set new external repo vars. Need 'export' so CONFIG_OVERRIDE_SCRIPT_URL can see them. | ||
| export ROOK_REPO="roocs/rook" | ||
| export ROOK_BRANCH="master" | ||
|
|
||
| # Not checking for expected output, just checking whether the code can run without errors. | ||
| PYTEST_EXTRA_OPTS="$PYTEST_EXTRA_OPTS --nbval-lax" | ||
|
|
||
| # Create CONFIG_OVERRIDE_SCRIPT_URL file on-the-fly to run the notebooks from | ||
| # our external repo. | ||
|
|
||
| CONFIG_OVERRIDE_SCRIPT_URL="/tmp/custom-repos.include.sh" | ||
|
|
||
| # Populate the content of our CONFIG_OVERRIDE_SCRIPT_URL. | ||
| echo ' | ||
| #!/bin/sh | ||
| # Sample config override script to run new notebooks from new external repo. | ||
|
|
||
| # Replicate processing steps in 'testall' script. | ||
|
|
||
| # Download the external repo. | ||
| downloadgithubrepos $ROOK_REPO $ROOK_BRANCH | ||
|
|
||
| # Prep vars for including new nbs in nb list to test. | ||
| ROOK_REPO_NAME="$(extract_repo_name "$ROOK_REPO")" | ||
| ROOK_DIR="$(sanitize_extracted_folder_name "${ROOK_REPO_NAME}-${ROOK_BRANCH}")" | ||
|
|
||
| delete_files_confusing_pytest "$ROOK_DIR" | ||
|
|
||
| # Set new nbs as nb list to test. | ||
| NOTEBOOKS="$ROOK_DIR/notebooks/*.ipynb" | ||
| ' > "$CONFIG_OVERRIDE_SCRIPT_URL" |
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.
Why not use a
buildout/**/*.ipynband leave the directory structure as originally defined by each repository? That would avoid all the directory renaming manipulations and potential side effects of removing pytest configs.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 previously thought that too. But actually real life usage, a flat directory is much easier to use, meaning to download the notebooks I want. In a nested layout, if I want multiple notebooks under multiple different sub-folders, I have to click a lot more. It's tiring.
Now with the option to only archive the notebooks enabled for the run, the chance to have name clash is even lower.
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.
Not sure what you mean. I do not perform any directory renaming manipulations. The notebooks are run from the original location of the checkout because some will use relative path to get their test data in the same checkout.
I only copy the notebooks only, not the test data, to the
buildout/folder so they are archived by Jenkins, along side their matching "output" notebook so we can download both of them and diff them manually if the diff presented by Jenkins console output is impossible to understand.As for "potential side effects of removing pytest configs", I also do not understand how that relate to Jenkins archiving config change.
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.
All the notebooks are copied as a flat list, which could lead to conflicts between repositories. I was under the impression that the repository name was used as prefix to the archived notebook names to avoid this, but it is not the case. With the results archived this way, it is very hard to trace back the original source of the notebook.
It does not affect the archiving, but it affects how the notebooks are executed by
pytestin order to generate their outputs. Therefore, if anypytestconfigurations options are applied to obtain certain effects needed by the tests, this could have bad side effects.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 never had trouble knowing which nb is from which repo but I agree not everyone know this. Just so it is clear, it is already like this before (flat list of
*.output.ipynb), I am just adding the original along-side the correspondingoutput.ipynb.However, I'll add the repo name as prefix to the nb filename to help differentiate and avoid name clash.
By the way, I did preserve the already existing name clash prevention:
PAVICS-e2e-workflow-tests/runtest
Lines 88 to 89 in d6fd500
But you had your comment in this Jenkins archiving section so that's why I was mixed up.
Like I said here #138 (comment), it weird but I had to do it, otherwise the test run do not work at all. The only hypothesis I have is mentioned in that comment.
By the way, deleting the extra files is an existing behavior. I did not just add it.
This PR is maximally backward-compatible. I did not add any new processing steps other than making these processing steps available to external repos.
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.
No, I open the notebook directly from the artifacts page and look at its contents from the browser.
If I went through the artifact to look for a notebook, it's because I already know which test case I'm investigating to debug.
This is exactly why I would rather have the notebooks with the generated outputs located under the same locations as the current artifacts using the same structure as displayed in the console outputs.
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.
@fmigneault
Okay, how about I make all the files (original and output) keep the same folder structure as prefix. Then you have your structure to easy search for your file, I have my flat list to easily switch between multiples repos.
Something like
buildout/pavics-sdi-master--docs--source--notebooks--WCS_example{,-output]}.ipynb. Good enough compromise so we can merge this?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 think it is a matter of compromise. The artifact structure should simply reflect the source repository. There is no need for an alternate naming convention and there is no need to define more code to handle it. The outputs should simply be saved where the notebooks are naturally collected in artifacts.
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.
Agreed, no compromise for usability.
Let me repeat comment #138 (comment) again if not clear enough: if "notebooks are under different nested folders ! You'll have to click to get to the first older, back out, then again to the 2nd folder ! The more folders, the more back and forth it is !!!"
Just a reminder, this artifact archiving just for immediate troubleshooting, it has no long term persistance value. I'd rather focus on usability and productivity than following some standard for something that is ephemeral and not important. Once you manage a lot of notebook repos, a flat list if much faster to deal with.
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.
Even then, I do not agree. If you know the path of the notebook you are interested in after getting an error, you have no reason to go back-and-forth between folders.
I see the addition of code, special character handling and deletion of repo-specific configs as a sign of bigger maintenance burden and potential side effects.