feat: python notebooks template repo #1195

loferris · 2021-08-25T23:14:34Z

Adding a template repo for Python notebooks, updating common.py

ivanmkc · 2021-08-25T23:19:38Z

synthtool/gcp/templates/python_notebooks/.cloud-build/check_python_version.py

+import sys
+
+MINIMUM_MAJOR_VERSION = 3
+MINIMUM_MINOR_VERSION = 3


Should be 5?

This is current with AI platform (updated 12 days ago), but will do!

ivanmkc · 2021-08-25T23:20:03Z

synthtool/gcp/templates/python_notebooks/.cloud-build/check_python_version.py

+    sys.version_info.major < MINIMUM_MAJOR_VERSION
+    and sys.version_info.minor < MINIMUM_MINOR_VERSION
+):
+    print("Error: Python version less than 3.5")


Maybe dynamically generate the string to reflect the constants?

Ah I understand that this might be old code that was inherited from several people.

I'm fixing it in next commit!

ivanmkc · 2021-08-25T23:23:44Z

synthtool/gcp/templates/python_notebooks/.cloud-build/notebook-execution-test-cloudbuild.yaml

@@ -0,0 +1,40 @@
+steps:


Can you make sure you have the most up-to-date version of this folder? I see this file is using an older version.

Compare with: https://github.com/GoogleCloudPlatform/ai-platform-samples/blob/master/.cloud-build/notebook-execution-test-cloudbuild.yaml

ivanmkc · 2021-08-25T23:24:10Z

synthtool/gcp/templates/python_notebooks/.cloud-build/requirements.txt

+nbconvert>=6.0
+papermill
+numpy
+pandas


Please update file

ivanmkc · 2021-08-25T23:28:09Z

Question, I see you are including synthtool/gcp/templates/python_notebooks/.cloud-build/test_folders.txt, which is an empty file.

Does that mean this will overwrite any changes in the downstream repos?
Perhaps better to leave this file out.

Regarding non-code files, such as MD, READMES, .gitignore:
I prefer to leave out all of these files and scope this synthtool integration to only the .cloud-build folder.

I imagine different teams can pick and choose what to integrate from synthtool. They might want their own README's but just want the notebook testing pipeline in .cloud-build.

loferris · 2021-08-26T00:26:24Z

shall we include just .github and .cloud-build in this PR? @ivanmkc

…htool into python-notebooks-templates updating pr

tswast · 2021-08-26T14:16:06Z

synthtool/gcp/templates/python_notebooks/.cloud-build/check_python_version.py

+
+if (
+    sys.version_info.major < MINIMUM_MAJOR_VERSION
+    and sys.version_info.minor < MINIMUM_MINOR_VERSION


This might need to be or? Wouldn't 2.7 result in the else statement?

Alternatively, you could flip the if/else and check that major and minor are >= expected value. (Probably worth checking for == 3 for the major since who knows what Python 4 will be if that ever happens.

Done in latest commit!

tswast · 2021-08-26T14:17:54Z

synthtool/gcp/templates/python_notebooks/.cloud-build/cleanup/cleanup-cloudbuild.yaml

@@ -0,0 +1,8 @@
+steps:  


Probably not 100% necessary, but YAML supports comments, so we really should have a license header here.

tswast · 2021-08-26T14:20:09Z

synthtool/gcp/templates/python_notebooks/README.md

@@ -0,0 +1,20 @@
+# Google Cloud [PRODUCT] Notebooks


Probably best to omit this file for now. Or perhaps template it (see other files with .j2 extension)

Removed with recent commit!

… yaml

ivanmkc

LGTM except that I flagged some files to remove.

ivanmkc · 2021-09-01T22:33:50Z

synthtool/gcp/templates/python_notebooks_testing_pipeline/.github/ISSUE_TEMPLATE/bug_report.md

@@ -0,0 +1,31 @@
+---


This seems unrelated to notebook testing so perhaps remove it?

ivanmkc · 2021-09-01T22:34:36Z

...templates/python_notebooks_testing_pipeline/.github/ISSUE_TEMPLATE/sample_feature_request.md

@@ -0,0 +1,20 @@
+---


This seems unrelated to notebook testing so perhaps remove it?

ivanmkc · 2021-09-01T22:34:40Z

synthtool/gcp/templates/python_notebooks_testing_pipeline/.github/pull-request-template.md

@@ -0,0 +1,12 @@
+Fixes #<issue_number_goes_here>


This seems unrelated to notebook testing so perhaps remove it?

sgtm! removed in latest commit

leahecole · 2021-09-16T22:27:00Z

synthtool/gcp/templates/python_notebooks_testing_pipeline/.cloud-build/cleanup/cleanup.py

+            else:
+                manager.delete(resource)
+
+        print("")


Does this print statement need to be here?

It was probably to add a newline for formatting purposes. Feel free to remove.

removing, but depending we can add back formatting if it makes things confusing.

leahecole · 2021-09-16T22:27:46Z

synthtool/gcp/templates/python_notebooks_testing_pipeline/.cloud-build/cleanup/cleanup.py

+        print("")
+
+
+is_dry_run = False


Will this setting of the dry run override any time it's set to true?

This is hard-coded, but could be changed to accept an arg. There was no need at the time for an arg, so it was left hard-coded.

@ivanmkc I've rewritten the cleanup file and updated the source files for the repo in the last two commits if you want to take a look!

leahecole · 2021-09-16T22:29:10Z

...ol/gcp/templates/python_notebooks_testing_pipeline/.cloud-build/execute_changed_notebooks.py

+        for notebook in notebooks:
+            print(f"Running notebook: {notebook}")
+
+            # TODO: Handle cases where multiple notebooks have the same name


Is there an issue open tracking this todo somewhere?

Not at the moment, could track in the synthtool issue tracker.

agreed, removing and let's track via issues.

leahecole · 2021-09-16T22:32:25Z

synthtool/gcp/templates/python_notebooks_testing_pipeline/.cloud-build/execute_notebook.py

+            stderr_file=sys.stderr,
+        )
+
+    except Exception:


I'm a little skittish about catching ALL exceptions, but at least we're raising them so I think I feel better about that. Do other @googleapis/python-samples-owners have opinions here?

If an exception isn't caught, then Cloud Build will fail on that step, basically failing all the notebooks (even ones that would have otherwise passed.)

Also, the cleanup and debug steps at the end of the Cloud Build pipeline will never run, thus the user wouldn't be able to debug the error.

Actually, this whole file seems out of date, it should be matching this file: https://github.com/GoogleCloudPlatform/ai-platform-samples/blob/master/.cloud-build/ExecuteNotebook.py

leahecole · 2021-09-16T22:32:48Z

...cp/templates/python_notebooks_testing_pipeline/.cloud-build/cleanup/cleanup-requirements.txt

@@ -0,0 +1 @@
+google-cloud-aiplatform


For most other requirements files, we prefer to pin to specific versions and have them updated by renovate bot. (You can add this non-standard filename to the renovate.json). Is there a reason we're not doing that here?

Ooh, I think we just weren't aware of the other standards for synthtool. I'll look into changing this in the PR.

leahecole · 2021-09-16T22:33:19Z

...gcp/templates/python_notebooks_testing_pipeline/.cloud-build/cleanup/cleanup-cloudbuild.yaml

+    args:
+    - -c
+    - 'python3 -m pip install -U -r .cloud-build/cleanup/cleanup-requirements.txt && python3 .cloud-build/cleanup/cleanup.py'
+timeout: 86400s


Is there a reason why we have such a high timeout?

leaving as is!

leahecole · 2021-09-16T22:34:15Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+# limitations under the License.
+steps:
+  # Show the gcloud info and check if gcloud exists
+  - name: ${_PYTHON_IMAGE}


Are the cloud build variables needed documented somewhere for our internal reference? I'm not sure if we do this well in general or not - @dinagraves opinions welcome here 😁

These would generally be in trigger definition, which doesn't have a lot of visibility outside of the project itself -- unless you're using terraform. However, I would suggest adding defaults in the cloudbuild.yaml - these can be overwritten by anyone when they run it, but it'll provide more context and a good default.

Yup, adding defaults would work fine @loferris
Also, +1 on documentation.

FYI $_BASE_BRANCH is injected by Cloud Build (see https://cloud.google.com/build/docs/configuring-builds/substitute-variable-values#using_default_substitutions)

$_FORCE_BASE_BRANCH is used because sometimes that variable is not populated (may be a bug), when you rerun a build.

leahecole · 2021-09-16T22:34:26Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+    args: 
+    - -c
+    - 'echo "Download executed notebooks with this command: \"mkdir -p artifacts && gsutil rsync -r gs://${_GCS_ARTIFACTS_BUCKET}/test-artifacts/PR_${_PR_NUMBER}/BUILD_${BUILD_ID} artifacts/\"" && if [ "$(ls -A /workspace/${BUILD_ID}/failure | wc -l)" -ne 0 ]; then exit 1; else exit 0; fi' 
+timeout: 86400s


flagging another very high timeout

It's 24 hours, VertexAI notebooks can take many hours to run.

leaving as is!

leahecole · 2021-09-16T22:35:12Z

synthtool/gcp/templates/python_notebooks_testing_pipeline/.cloud-build/requirements.txt

@@ -0,0 +1,8 @@
+ipython>=7.0


similar to above comment, can we pin to exact versions and use renovate to keep up with it? or is that less of an issue with a cloud build script compared to with samples maintenance?

I think that's possible, although I've never used renovate

@leahecole I think my main question here is if this template will be incorporated into repos downstream, wouldn't that mean that the template itself should contain its own renovate bot so its usable downstream? And if I do that, will that interfere with the renovate set up for this repo? I can add the requirements to this repo's requirements.txt at project root but I'm not sure how that would be passed to all the downstream repos using this template.

Good question. Will it be incorporated into samples libraries with owlbot, which already have renovatebot enabled? If so, we would want to modify this template. I believe that adding a new renovate.json template would make our 🤖 friend and the folks who maintain it extremely confused.

Or, are these notebook templates going to be used ad-hoc in random repos? If it's going to be used in random repos under the GoogleCloudPlatform and googleapis orgs, we do have renovatebot enabled at the org level and the way to bring it to life would be to create a new renovate.json template (like that other one I linked).

Other than python-docs-samples I'm not sure any of the other repos already have owlbot enabled, though that would be our next step! However, both of the main repos currently are under GCP not googleapis.

Would the suggestion be to add a renovate.json to the root of this template, make sure the right file extensions are flagged, and then leave blank (?) requirements files?

They all probably have it, but similar to renovate, it can magically be turned on with the addition of its config file (in owlbots case, owlbot.py)

Given that it sounds like the main repos are in the GCP org, I think that we actually might be covered - renovate will match any requirements.txt file now that I think about it, so as long as it's in a repo that has a renovate.json we are good!

awesome, so I've gone ahead and made sure all dependencies have specific versions and made sure the file names are easy for renovatebot to parse. see latest two commits!

ivanmkc

Blocking on execute_notebooks.

It should match: https://github.com/GoogleCloudPlatform/ai-platform-samples/blob/master/.cloud-build/ExecuteNotebook.py

Can you double-check that all the other files match https://github.com/GoogleCloudPlatform/ai-platform-samples/blob/master/.cloud-build/**, unless there are intentional changes?

ivanmkc · 2021-09-16T23:25:46Z

...ol/gcp/templates/python_notebooks_testing_pipeline/.cloud-build/execute_changed_notebooks.py

@@ -0,0 +1,160 @@
+#!/usr/bin/env python


This file also looks out-of-date, it should match https://github.com/GoogleCloudPlatform/ai-platform-samples/blob/master/.cloud-build/ExecuteChangedNotebooks.py

Will do - I think it's just out of date because the PR has been in flight for a minute.

ivanmkc

execute_changed_notebooks.py should match
https://github.com/GoogleCloudPlatform/ai-platform-samples/blob/master/.cloud-build/ExecuteChangedNotebooks.py

ivanmkc · 2021-09-16T23:28:51Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+    entrypoint: /bin/sh
+    args:
+    - -c
+    - 'python3 -m pip freeze && python3 .cloud-build/ExecuteChangedNotebooks.py --test_paths_file "${_TEST_PATHS_FILE}" --base_branch "${_FORCED_BASE_BRANCH}" --output_folder ${BUILD_ID} --variable_project_id ${PROJECT_ID} --variable_region ${_GCP_REGION}'


ExecuteChangedNotebooks.py -> execute_changed_notebooks.py

ivanmkc

Not sure how to add it in this repo, but can we add CI tests to test that all of this works?

dinagraves · 2021-09-16T23:30:05Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+# limitations under the License.
+steps:
+  # Show the gcloud info and check if gcloud exists
+  - name: ${_PYTHON_IMAGE}


These would generally be in trigger definition, which doesn't have a lot of visibility outside of the project itself -- unless you're using terraform. However, I would suggest adding defaults in the cloudbuild.yaml - these can be overwritten by anyone when they run it, but it'll provide more context and a good default.

dinagraves · 2021-09-16T23:31:07Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+    entrypoint: /bin/sh
+    args:
+    - -c
+    - 'gcloud config list'


Is this going to work? I guess I don't know what your python image is. Usually I would use the gcloud SDK image for this. If you're using something custom, I would add some comments about it.

It works. The image has the following requirements: https://docs.google.com/document/d/1MjbMDrm4GvfLJfqnPmdTUS8dueKdRAUsn5bmm2aCoZo/edit?resourcekey=0-LE_XfwNqTclOKG8hbFnxrQ#bookmark=id.412ps3ifvobk

leaving as is for now

dinagraves · 2021-09-16T23:32:02Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+    args:
+    - -c
+    - 'gcloud config list'
+  # Check the Python version


Do all of these need to be separate steps? Since you're using the shell entrypoint, you could combine some of them, unless you want them to be distinct failures.

@ivanmkc what was your thinking on combining these?

They are kept separate for readability.

dinagraves · 2021-09-16T23:33:53Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+    entrypoint: /bin/sh
+    args:
+    - -c
+    - 'python3 -m pip freeze && python3 .cloud-build/ExecuteChangedNotebooks.py --test_paths_file "${_TEST_PATHS_FILE}" --base_branch "${_FORCED_BASE_BRANCH}" --output_folder ${BUILD_ID} --variable_project_id ${PROJECT_ID} --variable_region ${_GCP_REGION}'


I would recommend splitting this up on separate lines since it's quite long. See example here: https://github.com/dinagraves/cloud-build-empathy-solution/blob/main/cloudbuild.yaml#L36

changed in last commit - I followed that pattern for the cited example as I didn't see more multi-line commands.

dinagraves · 2021-09-16T23:37:10Z

...lates/python_notebooks_testing_pipeline/.cloud-build/notebook-execution-test-cloudbuild.yaml

+    - 'python3 -m pip freeze && python3 .cloud-build/ExecuteChangedNotebooks.py --test_paths_file "${_TEST_PATHS_FILE}" --base_branch "${_FORCED_BASE_BRANCH}" --output_folder ${BUILD_ID} --variable_project_id ${PROJECT_ID} --variable_region ${_GCP_REGION}'
+    env:
+    - 'IS_TESTING=1'
+  # Manually copy artifacts to GCS


You can use the built-in artifacts feature: https://github.com/GoogleCloudPlatform/cloud-build-samples/blob/main/python-example-flask/cloudbuild.yaml#L54

changed in latest commit!

Hi Dinah - I had to revert this change as it wasn't working for our particular setup!

loferris · 2021-09-22T22:10:16Z

Not sure how to add it in this repo, but can we add CI tests to test that all of this works?

Agreed, let's add it as an issue for a followup PR and I'll investigate with Synthtool folks.

loferris · 2021-10-15T22:27:45Z

@dinagraves the current commit reflects some adjustments based on your comments and @ivanmkc's input! would love any additional feedback before I ping folks on approvals!

chingor13

Is this used by multiple repositories? If not, we can just manage the files in the one python-notebooks repository rather than indirectly managing the files here.

loferris · 2021-10-18T16:08:36Z

@chingor13 It's being used in multiple repositories and hopefully many more.

I made the changes requested.

bcoe

I'm very much in support of codifying how notebooks are created.

A few non blocking questions:

it looks like unlike other templates this will use Cloud Build for testing, rather than kokoro?
where will the environment variables be set, such as project name, for running the notebook tests.
what project will the notebook tests run against?

loferris · 2021-10-19T22:09:59Z

Yes, we're using Cloud Build rather than Kokoro at the moment and are in active convo about whether that may become a variant standard for Python sample testing.
I think right now we're leaning into hardcoding defaults, per @dinagraves suggestion of cloud-build.yaml best practices. The current PR is sort of splitting the difference, and the environmental defaults at the moment are set in Cloud Build I believe.
Our current expectation is that participating repos would decide where they wanted their cloud-build testing pipeline to live in their projects, and we'll be creating documentation for that side of things. (At the moment the current users of this template are all directly involved in the notebooks working group.)

I'm going to make one more commit to fix a little bug I found on my testing branch before merging!

leahecole · 2021-10-20T18:41:26Z

I just came here to catch up on the comments and am thrilled this is merged!! Great work @loferris !!! 😁

leahecole · 2021-10-20T18:41:55Z

(and great work many many reviewers!! 😀 )

Lo Ferris added 3 commits August 24, 2021 09:15

adding python_notebooks template to synthtool

b2e0505

adding python notebook temlate repo

f22fe84

adding python notebooks template

d7c0713

loferris requested review from bcoe, chingor13 and SurferJeffAtGoogle as code owners August 25, 2021 23:14

loferris requested a review from a team August 25, 2021 23:14

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 25, 2021

loferris requested review from ivanmkc and tswast August 25, 2021 23:15

Merge branch 'master' into python-notebooks-templates

f2e4ea0

loferris changed the title ~~Python notebooks templates~~ feat: python notebooks template repo Aug 25, 2021

ivanmkc reviewed Aug 25, 2021

View reviewed changes

Lo Ferris added 3 commits August 25, 2021 17:35

updating .cloud-build to recent versions

790c298

Merge branch 'python-notebooks-templates' of github.com:loferris/synt…

db73c44

…htool into python-notebooks-templates updating pr

addressing kokoro fail

7aa53a7

tswast reviewed Aug 26, 2021

View reviewed changes

Lo Ferris and others added 3 commits August 27, 2021 12:46

streamlining purpose of PR to cover only our shared testing pipeline

373043e

updating check for Python version and adding a licence to cloud-build…

7e9b33e

… yaml

Merge branch 'master' into python-notebooks-templates

567d727

ivanmkc reviewed Sep 1, 2021

View reviewed changes

Lo Ferris and others added 2 commits September 1, 2021 16:07

removing PR templates

a2d7581

Merge branch 'master' into python-notebooks-templates

bf74f9d

Merge branch 'master' into python-notebooks-templates

e187dea

leahecole reviewed Sep 16, 2021

View reviewed changes

ivanmkc suggested changes Sep 16, 2021

View reviewed changes

ivanmkc reviewed Sep 16, 2021

View reviewed changes

ivanmkc suggested changes Sep 16, 2021

View reviewed changes

ivanmkc reviewed Sep 16, 2021

View reviewed changes

ivanmkc previously requested changes Sep 16, 2021

View reviewed changes

dinagraves reviewed Sep 16, 2021

View reviewed changes

updating versions of cloud-build suite

ff9c271

loferris and others added 8 commits September 22, 2021 15:13

Merge branch 'master' into python-notebooks-templates

bb613bd

addressed comments on cleanup setup

9906270

make requirements more compatible with renovatebot

f03fb89

pinning versions

00cdb68

used default artifacts pattern instead of manual copying of artifacts

c3ee69a

Merge branch 'master' into python-notebooks-templates

721511c

reformatting commands

f86f6ac

adding defaults

3c2b225

chingor13 reviewed Oct 16, 2021

View reviewed changes

Merge branch 'master' into python-notebooks-templates

e8068cb

bcoe approved these changes Oct 19, 2021

View reviewed changes

reverting to previous cloud-build.yaml file

cbc4220

loferris merged commit 02cd0ee into googleapis:master Oct 19, 2021

loferris deleted the python-notebooks-templates branch October 19, 2021 23:04

		@@ -0,0 +1 @@
		google-cloud-aiplatform

feat: python notebooks template repo #1195

feat: python notebooks template repo #1195

Conversation

loferris commented Aug 25, 2021

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

ivanmkc commented Aug 25, 2021

loferris commented Aug 26, 2021

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

ivanmkc 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

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

ivanmkc Sep 16, 2021 • edited Loading

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

ivanmkc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanmkc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanmkc 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

loferris commented Sep 22, 2021

loferris commented Oct 15, 2021

chingor13 left a comment

ivanmkc Sep 16, 2021 •

edited

Loading