-
Notifications
You must be signed in to change notification settings - Fork 17
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
Simplify e2e tests #33
Changes from 11 commits
914aa9e
d03d07e
acca598
81b37ad
af5a003
a9a5c83
3e9f63b
362ad8a
9907264
6951b47
01abc17
15afe3a
1393bc6
77f2584
59305a6
be7cc57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,8 +45,8 @@ steps: | |
curl -sSL https://install.python-poetry.org | python3 - && \ | ||
export PATH="/builder/home/.local/bin:$$PATH" && \ | ||
make install && \ | ||
make e2e-tests pipeline=training && \ | ||
make e2e-tests pipeline=prediction | ||
make run pipeline=training build=false wait=true && \ | ||
make run pipeline=prediction build=false wait=true | ||
Comment on lines
+48
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @browningjp-datatonic why don't we build containers when running e2e tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The containers are built in the previous pipeline step. We pass |
||
env: | ||
- ENABLE_PIPELINE_CACHING=${_TEST_ENABLE_PIPELINE_CACHING} | ||
- VERTEX_LOCATION=${_TEST_VERTEX_LOCATION} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,12 +20,14 @@ | |
def trigger_pipeline( | ||
template_path: str, | ||
display_name: str, | ||
wait: bool = False, | ||
) -> aiplatform.PipelineJob: | ||
"""Trigger a Vertex Pipeline run from a (local) compiled pipeline definition. | ||
|
||
Args: | ||
template_path (str): file path to the compiled YAML pipeline | ||
display_name (str): Display name to use for the PipelineJob | ||
wait (bool): Wait for the pipeline to finish running | ||
|
||
Returns: | ||
aiplatform.PipelineJob: the Vertex PipelineJob object | ||
|
@@ -67,6 +69,10 @@ def trigger_pipeline( | |
network=network, | ||
) | ||
|
||
if wait: | ||
# Wait for pipeline to finish running before returning | ||
pl.wait() | ||
|
||
return pl | ||
|
||
|
||
|
@@ -84,10 +90,22 @@ def trigger_pipeline( | |
type=str, | ||
) | ||
|
||
parser.add_argument( | ||
"--wait", | ||
help="Wait for the pipeline to finish running", | ||
type=str, | ||
) | ||
# Get commandline args | ||
args = parser.parse_args() | ||
|
||
if args.wait.lower() == "true": | ||
wait = True | ||
elif args.wait.lower() != "false": | ||
raise ValueError("wait variable must be 'true' or 'false'") | ||
wait = False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @becky-dt can you please check for similar logic elsewhere in this repo? would like to follow the same logic or even import a reusable function for parsing boolean flags from the command-line. thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felix-datatonic There are a couple more booleans in the Makefile but in this case the logic is all run in the makefile (unlike above where the wait param is passed into the python function) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @roberta-dt and I added this logic to the makefile in the latest commit :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @browningjp-datatonic can you please take a look at the change we made to the run command? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other place where similar logic is used in Python is for The For consistency in behaviour, I would suggest to modify your Python logic here to: if args.wait == "true":
wait = True
elif args.wait != "false":
raise ValueError("wait variable must be 'true' or 'false'")
wait = False (value from user must be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
trigger_pipeline( | ||
template_path=args.template_path, | ||
display_name=args.display_name, | ||
wait=wait, | ||
) |
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'm happy with the changes, just noticed that
test
task isn't documented in thisMakefile
.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 might have missed this during the rebase. @roberta-dt please could you take a look?
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.
15afe3a