Add extra marks to VertexAI IT tests #28534

jrmccluskey · 2023-09-19T15:59:49Z

Mark the Vertex AI IT tests as using tensorflow and add an extra check to ensure the dependency has been installed before running the test suite.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

jrmccluskey · 2023-09-19T16:01:38Z

Run Python 3.8 PostCommit

codecov · 2023-09-19T16:39:59Z

Codecov Report

Merging #28534 (3bea45a) into master (8f60924) will increase coverage by 0.01%.
Report is 77 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #28534      +/-   ##
==========================================
+ Coverage   72.33%   72.34%   +0.01%     
==========================================
  Files         683      683              
  Lines      100709   100814     +105     
==========================================
+ Hits        72843    72938      +95     
- Misses      26288    26298      +10     
  Partials     1578     1578

Flag	Coverage Δ
python	`82.85% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jrmccluskey · 2023-09-19T16:58:54Z

Run Python 3.8 PostCommit

github-actions · 2023-09-19T17:05:52Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damccorm for label python.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

damccorm

LGTM, lets make sure that the postcommit passes before merging though

tvalentyn · 2023-09-19T18:07:30Z

sdks/python/apache_beam/ml/inference/vertex_ai_inference_it_test.py

@@ -45,6 +47,10 @@


 class VertexAIInference(unittest.TestCase):
+  @unittest.skipIf(
+      tf is None, 'Missing dependencies. '


Given that we already have @pytest.mark.uses_vertex_ai , I would't do this skip. We might accidentally silently disable the test and won't know it's not running.

do we need to remove @pytest.mark.it_postcommit if this test is accidentally included into a suite where it is not supposed to run?

actually, this is almost the same as statements in line 32. i guess i have a general question, how do we make sure this test is running in a suite where it is supposed to run in. It sounds like this test was passing before, and now it is failing, b/c TF no longer installed in beam containers, and the spirit of this change is to skip the test.

sounds like something might be misconfigured.

If it was included in a suite that picks up all it_postcommit, not vertexai-specific - that's not expected - is it because we incorrectly included it_postcommit annotation? then we should remove that.

If the test was included in a special vertex AI suite, which we run as part of postcommits, and now starts to fail - i'd expect seeing changes pertaining to adding tensorflow in a requirements file, to make the test pass again.

looks like we have

beam/sdks/python/apache_beam/ml/inference/vertex_ai_tests_requirements.txt

Line 19 in 3915c85

tensorflow>=2.12.0

, so probably need to remove it_postcommit annotation from this test to not include it twice.

All of our other IT tests (tensorflow model handler IT test included) also have the it_postcommit annotation.

disregard that, it's WAI the limited load on the logs just obscured the rest of the suites listed

All of our other IT tests (tensorflow model handler IT test included) also have the it_postcommit annotation.

So, if we look at tests that have uses_tf, we have two kinds of tests: unit tests and postcommit IT tests (the latter run a whole pipeline).
Since for vertex AI you don't have unit tests and only have postcommit tests, I think can simplify the markers in your suite and only use one marker uses_vertex_ai, and collect that test in postcommit suite.

What's weird is that we're getting the gradle task invoked separately and not as part of the postcommit

postCommitIT is a one test suite out of many that run in a postcommit Jenkins / GHA suite. VertextAI suite is another suite running in parallel.

beam/build.gradle.kts

Line 497 in a90879a

tasks.register("python38PostCommit") {

to clarify: vertex ai suite is a part of inferencePostcommitIT suite:

beam/sdks/python/test-suites/dataflow/common.gradle

Line 470 in 6c4bdf1

project.tasks.register("inferencePostCommitIT") {

Tensorflow is getting installed and the test isn't getting skipped for lacking tensorflow, but the test is running on Dataflow where the worker doesn't have tensorflow - https://console.cloud.google.com/dataflow/jobs/us-central1/2023-09-19_10_21_28-17008252103869768778?project=apache-beam-testing

tvalentyn · 2023-09-19T18:09:17Z

sdks/python/apache_beam/ml/inference/vertex_ai_inference_it_test.py

+  @unittest.skipIf(
+      tf is None, 'Missing dependencies. '
+      'Test depends on tensorflow')
+  @pytest.mark.uses_tf


is this necessary?

it's ok too keep. looks like we alredy defined this marker and use it in other tests.

Perhaps not but considering the postcommits went red without a clear reason I'd rather cover all of our bases here. would have thought that missing TF would have gotten caught in the try-except-skip import block getting the pipeline code, but it didn't.

actually, i take it back. I think this decorator is serving a different puprose, see

beam/sdks/python/tox.ini

Line 398 in 3915c85

/bin/sh -c 'pytest -o junit_suite_name={envname} --junitxml=pytest_{envname}.xml -n 6 -m uses_tf {posargs}; ret=$?; [ $ret = 5 ] && exit 0 || exit $ret'

adding this annotation would make tox pick up this test some other unit test suites, and then skip b/c vertex ai deps are not installed.

jrmccluskey · 2023-09-19T18:50:44Z

Run Python 3.8 PostCommit

jrmccluskey · 2023-09-19T19:36:52Z

Specifying the requirements file for the IT test seems to have resolved the issue - https://ci-beam.apache.org/job/beam_PostCommit_Python38_PR/777/

jrmccluskey · 2023-09-19T20:03:30Z

Run Python_Coverage PreCommit

tvalentyn · 2023-09-20T21:43:28Z

somehow postcommits are still failing https://ci-beam.apache.org/job/beam_PostCommit_Python38/4612/

tvalentyn · 2023-09-20T21:50:16Z

sent #28571

Add extra marks to VertexAI IT tests

f2b92ec

github-actions bot added the python label Sep 19, 2023

jrmccluskey added 2 commits September 19, 2023 12:08

Formatting

36860c7

Linting

6870527

github-actions bot added the Next Action: Reviewers label Sep 19, 2023

damccorm approved these changes Sep 19, 2023

View reviewed changes

tvalentyn reviewed Sep 19, 2023

View reviewed changes

pass requirements file to dataflow

d9e5522

Remove unused import

3bea45a

tvalentyn merged commit a1ec055 into apache:master Sep 19, 2023
77 checks passed

tvalentyn mentioned this pull request Sep 21, 2023

[Feature Request]: Rewrite pytest markers to avoid including ML test suites into a catch-all postcommit suite. #28578

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extra marks to VertexAI IT tests #28534

Add extra marks to VertexAI IT tests #28534

jrmccluskey commented Sep 19, 2023

jrmccluskey commented Sep 19, 2023

codecov bot commented Sep 19, 2023 •

edited

Loading

jrmccluskey commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

damccorm left a comment

tvalentyn Sep 19, 2023

tvalentyn Sep 19, 2023

tvalentyn Sep 19, 2023 •

edited

Loading

tvalentyn Sep 19, 2023

jrmccluskey Sep 19, 2023

jrmccluskey Sep 19, 2023

tvalentyn Sep 19, 2023

tvalentyn Sep 19, 2023 •

edited

Loading

tvalentyn Sep 19, 2023

jrmccluskey Sep 19, 2023

tvalentyn Sep 19, 2023

tvalentyn Sep 19, 2023

jrmccluskey Sep 19, 2023

tvalentyn Sep 19, 2023

tvalentyn Sep 19, 2023

jrmccluskey commented Sep 19, 2023

jrmccluskey commented Sep 19, 2023

jrmccluskey commented Sep 19, 2023

tvalentyn commented Sep 20, 2023

tvalentyn commented Sep 20, 2023

Add extra marks to VertexAI IT tests #28534

Add extra marks to VertexAI IT tests #28534

Conversation

jrmccluskey commented Sep 19, 2023

GitHub Actions Tests Status (on master branch)

jrmccluskey commented Sep 19, 2023

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

jrmccluskey commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

damccorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvalentyn Sep 19, 2023 • 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

tvalentyn Sep 19, 2023 • 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

jrmccluskey commented Sep 19, 2023

jrmccluskey commented Sep 19, 2023

jrmccluskey commented Sep 19, 2023

tvalentyn commented Sep 20, 2023

tvalentyn commented Sep 20, 2023

codecov bot commented Sep 19, 2023 •

edited

Loading

tvalentyn Sep 19, 2023 •

edited

Loading

tvalentyn Sep 19, 2023 •

edited

Loading