feat: Add DRIFT region tag parser to CI by ace-n · Pull Request #4506 · GoogleCloudPlatform/python-docs-samples

ace-n · 2020-08-15T01:42:52Z

Cleaned-up version of #4402

tmatsuo · 2020-08-15T16:27:29Z

python-3.7 build has a failure, and the new script is throwing an error:

Command pytest --junitxml=sponge_log.xml failed with exit code -6
Session py-3.7 failed.
cp: cannot stat '/workspace/speech/cloud-client/sponge_log.xml': No such file or directory
cat: /workspace/speech/cloud-client/drift_tmp.xml: No such file or directory
Traceback (most recent call last):
  File "/region-tag-parser/wizard-py/cli.py", line 200, in <module>
    inject_snippet_mapping(args.root_dir, sys.stdin.readlines())
  File "/region-tag-parser/wizard-py/cli.py", line 89, in inject_snippet_mapping
    xunit_tree = etree.fromstring("".join(stdin_lines))
  File "/usr/local/lib/python3.8/xml/etree/ElementTree.py", line 1321, in XML
    return parser.close()
xml.etree.ElementTree.ParseError: no element found: line 1, column 0
Testing failed: Nox returned a non-zero exit code.

Seems like the script needs to be little bit more robust.

Also, since you have changes in the Dockerfile, all the CI builds are full build.
I strongly recommend that you develop the script further locally.

You can emulate the Kokoro full build with the following command:

$ cd python-docs-samples
$ TRAMPOLINE_BUILD_FILE=.kokoro/tests/run_tests.sh .kokoro/trampoline_v2.sh

Full build takes long time, so you may prefer:

$ cd python-docs-samples
$ TRAMPOLINE_BUILD_FILE=.kokoro/tests/run_tests_diff_master.sh .kokoro/trampoline_v2.sh

This command only runs tests for directories that has changes from master. You can change some tests so that run_tests_diff_master.sh will pick up the directory.
Update: Unfortunately, you have a change in Dockerfile, this command will be also full build for you.

You can commit your Dockerfile changes into your local master, then you can deceive the test script.

$ cd python-docs-samples
$ git checkout master
$ git rebase drift-parser-2 # let master have the Dockerfile change
$ git checkout drift-parser-2
# Edit some test files
$ TRAMPOLINE_BUILD_FILE=.kokoro/tests/run_tests_diff_master.sh .kokoro/trampoline_v2.sh

Anyways, full builds on Kokoro consume resources and interfere other PRs, so please be considerable.

tmatsuo · 2020-08-15T16:57:15Z


+# Inject region tag data into the test log
+XUNIT_PATH="$PWD/sponge_log.xml"
+XUNIT_TMP_PATH="$PWD/drift_tmp.xml"


Can you use temp directory for XUNIT_TMP_PATH?

Done for XUNIT_TMP_PATH, though I think XUNIT_PATH needs to stay in $PWD.

tmatsuo · 2020-08-18T07:25:11Z


+# install PyYaml (used by the DRIFT region tag parsing system)
+pip install --user -q pyyaml
+


If we introduce an envvar, can we only install pyyaml if the envvar is set to true? Also I think it is ok to install the parser alongside of pyyaml.

Or even, how about to have requirements.txt in the parser repo and run pip install --user -r ${PARSER_DIR}/requirements.txt or sth?

I got this working by installing pyyaml in a specific directory and adding that to the PYTHONPATH. (I think there are some virtualenv issues here that prevent the usual "pip install and go" approach, and this was the easiest workaround to implement.)

tmatsuo

You need to add INJECT_REGION_TAGS to pass_down_envvars in .trampolinerc

…m/python-docs-samples into drift-parser-2

tmatsuo

Code looks good. Maybe we should run py-3.7 periodic build manually against the drift-parser-2 branch before merge. I'll start the build once the presubmit builds finish.

tmatsuo · 2020-08-19T04:49:22Z

I run py37 periodic build against your branch:
https://source.cloud.google.com/results/invocations/34c925d1-5261-466a-826b-6e8bc00c30ad/targets

I think we have little more to fix.

tmatsuo

It's close, but we need little bit more tweak.

Also, can you try this locally and check if the resulted xml files have expected contents?

tmatsuo · 2020-08-19T04:56:58Z

+# Setup DRIFT region tag parser
+# (only run on *some* builds)
+if [ "${INJECT_REGION_TAGS:-}" == "true" ]]; then
+    # install PyYaml (used by the DRIFT region tag parsing system)


Ditto for missing bracket.

It's not obvious from the log, so can you add info log?

echo "Downloading region tag parser"

or something?

Also, do you prefer always installing it from HEAD?

If you install with a SHA1 hash or release tag, can you also log the hash or the release tag?

Fixed missing bracket

Added debug log statements

Yes - installing from HEAD is the intent here (esp. since we don't need to use a commit hash to bust Docker caches!)

Ok, this raises another question. How is the parser script tested?

The Python script itself is fetched from this repo; there are tests in that folder.

(There aren't any [new] tests for the .sh testing scripts.)

Do you have CI builds setup?

Not yet - though we plan to add them in the coming months.

(If you'd prefer to do that immediately after merging this PR though, that can be done.)

Well, then the script might be not tested. We'll need extra caution because the script might fail.

Also, the script might empty the xml file. If that happens, we'll lost all the reporting capabilities including build cop bot issues.

I'd like you to setup CI on your repo before merging this.

Also, I don't think we should use redirect (>) here. Because if the script exit 0, but produces no output, then we'll loose all the test logs.

ace-n · 2020-08-19T07:22:12Z

Reran the periodic build here

tmatsuo · 2020-08-19T18:24:52Z

+# Setup DRIFT region tag parser
+# (only run on *some* builds)
+if [ "${INJECT_REGION_TAGS:-}" == "true" ]]; then
+    # install PyYaml (used by the DRIFT region tag parsing system)


Ok, this raises another question. How is the parser script tested?

tmatsuo · 2020-08-21T20:18:37Z

+    if [[ -f "$XUNIT_PATH" ]]; then
+        echo "=== Injecting region tags into XUnit output ==="
+        echo "Processing XUnit output file: $XUNIT_PATH"
+        cat "$XUNIT_PATH" | python3.7 "$PARSER_PATH" inject-snippet-mapping "$PWD" > "$XUNIT_TMP_PATH"


What happens when we remove python 3.7 from the docker image?

Would it be better to use python3 here?

tmatsuo · 2020-08-21T20:25:14Z

+# Setup DRIFT region tag parser
+# (only run on *some* builds)
+if [ "${INJECT_REGION_TAGS:-}" == "true" ]]; then
+    # install PyYaml (used by the DRIFT region tag parsing system)


Also, the script might empty the xml file. If that happens, we'll lost all the reporting capabilities including build cop bot issues.

I'd like you to setup CI on your repo before merging this.

Also, I don't think we should use redirect (>) here. Because if the script exit 0, but produces no output, then we'll loose all the test logs.

…m/python-docs-samples into drift-parser-2

ace-n · 2020-08-24T19:52:31Z

New periodic build here.

tmatsuo

Also let me know once CI build is set up in the upstream repo.

tmatsuo · 2020-08-24T22:24:39Z

+
+        cat "$XUNIT_PATH" | python3.7 "$PARSER_PATH" inject-snippet-mapping --output_file "$XUNIT_TMP_PATH" "$PWD"
+        if [[ $? -eq 0 ]]; then
+            mv $XUNIT_TMP_PATH $XUNIT_PATH


Are you 100% sure that there is a file when the script exit with 0?
If not, please check the file existance.

Also, does it make sense to log something here as well?

If the file at $XUNIT_TMP_PATH doesn't exist, then the mv command should fail.

IMO, logging something here would be overkill. 🙂

So if mv fails, the test is marked as failed even if just the injection here failed?
I don't like it.

Well, actually, there's no -e flag set in this file. So it may keep running.
I still like the explicit check, but I think I can live with the current code.

tmatsuo · 2020-08-24T22:25:43Z

+
+    export REGION_TAG_PARSER_DIR="/tmp/region-tag-parser"
+    export PARSER_PATH="${REGION_TAG_PARSER_DIR}/wizard-py/cli.py"
+    export PIP_PATH=/tmp/pyyaml


Why did you need PIP_PATH again?

Did you try:

pip install --user pyyaml -q

?

IIRC, I did (and it didn't work).

What's the error message?

If memory serves, it couldn't find the package (so probably something like cannot find module 'yaml')

I think --user flag should work.
Can you try this locally and paste the error log?

If I use that line and remove the PYTHONPATH modification:

Processing XUnit output file: /workspace/functions/env_vars/sponge_log.xml (saving output to /tmp/tmp.I0Hx5IhTZh) Traceback (most recent call last): File "/tmp/region-tag-parser/wizard-py/cli.py", line 17, in <module> import analyze File "/tmp/region-tag-parser/wizard-py/analyze.py", line 19, in <module> import yaml_utils File "/tmp/region-tag-parser/wizard-py/yaml_utils.py", line 19, in <module> import yaml ModuleNotFoundError: No module named 'yaml'

If we're going to use --user instead of specifying a target directory, we'll probably need to tell Python what directory --user corresponds to (via PYTHONPATH).

Hmm maybe the package is being installed to a pip associated with a different Python version. Could you try something like python3.7 -m pip install ...?

tmatsuo · 2020-08-25T00:05:25Z

+
+        cat "$XUNIT_PATH" | python3.7 "$PARSER_PATH" inject-snippet-mapping --output_file "$XUNIT_TMP_PATH" "$PWD"
+        if [[ $? -eq 0 ]]; then
+            mv $XUNIT_TMP_PATH $XUNIT_PATH


So if mv fails, the test is marked as failed even if just the injection here failed?
I don't like it.

ace-n · 2020-08-25T00:08:24Z

So if mv fails, the test is marked as failed even if just the injection here failed?
I don't like it.

Maybe the cleaner answer here (instead of including a bunch of checks) is to wrap this section like this?

set +e
...
set -e

…m/python-docs-samples into drift-parser-2

tmatsuo · 2020-08-25T00:38:30Z

@ace-n
Question: is there any reason you don't want to check the file existence?

ace-n · 2020-08-25T00:55:31Z

@ace-n
Question: is there any reason you don't want to check the file existence?

I'm not entirely against it, but explicitly checking for every edge case makes this more complex (and in my view, harder to maintain). Personally, I think the set e approach is a (syntactically) cleaner solution.

tmatsuo · 2020-08-25T01:18:35Z

@ace-n
Actually this file does not -e flag set, so it will keep running even if the mv fails.

However, I think being explicit and verbose logging will help us debug things if something wrong happens. I still recommend my approach.

…m/python-docs-samples into drift-parser-2

ace-n · 2020-08-25T04:11:37Z

@tmatsuo I went ahead and added a file existence check.

I also added set -e to trap any errors we might not think to explicitly check for.

busunkim96

LGTM if @tmatsuo approves.

(Stamping to make this auto-mergeable once Takashi approves the changes)

ace-n · 2020-08-25T23:13:15Z

FYI @tmatsuo build failures are unrelated to this PR.

tmatsuo · 2020-08-25T23:21:49Z

+if [[ "${INJECT_REGION_TAGS:-}" == "true" ]]; then
+    export REGION_TAG_PARSER_DIR="/tmp/region-tag-parser"
+    export PARSER_PATH="${REGION_TAG_PARSER_DIR}/wizard-py/cli.py"
+    export PIP_PATH=/tmp/pyyaml


Is PIP_PATH used?

tmatsuo · 2020-08-26T05:47:22Z

Test failures are known ones, let's merge this.

Add DRIFT region tag parser to CI

b3fd0d5

ace-n requested review from busunkim96 and tmatsuo August 15, 2020 01:42

ace-n requested a review from a team as a code owner August 15, 2020 01:42

blunderbuss-gcf Bot assigned tmatsuo Aug 15, 2020

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 15, 2020

ace-n mentioned this pull request Aug 15, 2020

[ABANDONED] draft of DRIFT region tag tracking #4402

Closed

Merge branch 'master' into drift-parser-2

a23d664

tmatsuo reviewed Aug 15, 2020

View reviewed changes

Ace Nassri added 2 commits August 17, 2020 17:25

Merge branch 'master' into drift-parser-2

27a1f1c

Address comments

6fbf850

tmatsuo reviewed Aug 18, 2020

View reviewed changes

ace-n added 3 commits August 18, 2020 16:50

Address comments, take 2

d699285

Address comments, take 2.1 (yaml)

65599ff

Merge branch 'master' into drift-parser-2

01f28d8

tmatsuo suggested changes Aug 19, 2020

View reviewed changes

ace-n added 2 commits August 18, 2020 18:11

Address feedback, take 3

b92b20e

Merge branch 'drift-parser-2' of http://github.com/GoogleCloudPlatfor…

c0d2629

…m/python-docs-samples into drift-parser-2

tmatsuo reviewed Aug 19, 2020

View reviewed changes

tmatsuo suggested changes Aug 19, 2020

View reviewed changes

Address comments, take 4

55281f3

Fix missing bracket

231a7e4

tmatsuo suggested changes Aug 19, 2020

View reviewed changes

Address comments, take 5

9b4d428

ace-n requested a review from tmatsuo August 20, 2020 01:21

tmatsuo reviewed Aug 20, 2020

View reviewed changes

Comment thread .kokoro/tests/run_single_test.sh Outdated

Address comments, take 6

a6b821b

tmatsuo suggested changes Aug 21, 2020

View reviewed changes

ace-n mentioned this pull request Aug 21, 2020

Add missing test datafiles + Python "write to file" option GoogleCloudPlatform/repo-automation-playground#32

Merged

ace-n added 5 commits August 21, 2020 16:08

Use uuid in temp file + python write instead of stdout

611044e

DBG foo

802d8b7

Address comments + get things working locally

18dc556

Merge branch 'drift-parser-2' of http://github.com/GoogleCloudPlatfor…

09e19ee

…m/python-docs-samples into drift-parser-2

Revert "DBG foo"

487712d

tmatsuo suggested changes Aug 24, 2020

View reviewed changes

tmatsuo suggested changes Aug 25, 2020

View reviewed changes

ace-n added 3 commits August 24, 2020 17:22

uuid -> mktemp

01d2896

Merge branch 'drift-parser-2' of http://github.com/GoogleCloudPlatfor…

2c3d822

…m/python-docs-samples into drift-parser-2

Merge branch 'master' into drift-parser-2

d1cf339

ace-n added 2 commits August 24, 2020 21:08

Use set e for error handling + check file existence

085ada5

Merge branch 'drift-parser-2' of http://github.com/GoogleCloudPlatfor…

00a2d3b

…m/python-docs-samples into drift-parser-2

busunkim96 approved these changes Aug 25, 2020

View reviewed changes

Merge branch 'master' into drift-parser-2

a723d33

tmatsuo reviewed Aug 25, 2020

View reviewed changes

Takashi Matsuo added 2 commits August 26, 2020 01:09

use python3 -m pip install

d653e3e

remove env vars from run_single_test.sh

18358aa

tmatsuo approved these changes Aug 26, 2020

View reviewed changes

tmatsuo merged commit d1dfc72 into master Aug 26, 2020

tmatsuo deleted the drift-parser-2 branch August 26, 2020 05:49


		# install PyYaml (used by the DRIFT region tag parsing system)
		pip install --user -q pyyaml

Conversation

ace-n commented Aug 15, 2020

Uh oh!

tmatsuo commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

tmatsuo commented Aug 19, 2020

Uh oh!

tmatsuo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmatsuo Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n commented Aug 24, 2020

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmatsuo commented Aug 15, 2020 •

edited

Loading

ace-n Aug 20, 2020 •

edited

Loading

tmatsuo left a comment •

edited

Loading

tmatsuo Aug 19, 2020 •

edited

Loading

ace-n Aug 20, 2020 •

edited

Loading

tmatsuo Aug 21, 2020 •

edited

Loading

ace-n commented Aug 19, 2020 •

edited

Loading

tmatsuo Aug 21, 2020 •

edited

Loading

tmatsuo Aug 25, 2020 •

edited

Loading

ace-n Aug 25, 2020 •

edited

Loading

ace-n commented Aug 25, 2020 •

edited

Loading

ace-n commented Aug 25, 2020 •

edited

Loading