UPSTREAM: <carry>: add last_run_creation #52
Conversation
Commit Checker results: |
Commit Checker results: |
|
A set of new images have been built to help with testing out this PR: |
|
An OCP cluster where you are logged in as cluster admin is required. The Data Science Pipelines team recommends testing this using the Data Science Pipelines Operator. Check here for more information on using the DSPO. To use and deploy a DSP stack with these images (assuming the DSPO is deployed), first save the following YAML to a file named apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
name: pr-52
spec:
dspVersion: v2
apiServer:
image: "quay.io/opendatahub/ds-pipelines-api-server:pr-52"
argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-52"
argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-52"
persistenceAgent:
image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-52"
scheduledWorkflow:
image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-52"
mlmd:
deploy: true # Optional component
grpc:
image: "quay.io/opendatahub/mlmd-grpc-server:latest"
envoy:
image: "registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2"
mlpipelineUI:
deploy: true # Optional component
image: "quay.io/opendatahub/ds-pipelines-frontend:pr-52"
objectStorage:
minio:
deploy: true
image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'Then run the following: cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines.git
cd data-science-pipelines/
git fetch origin pull/52/head
git checkout -b pullrequest 733f60bc3ac55cf16eed32d6be1c14cd45419758
oc apply -f dspa.pr-52.yamlMore instructions here on how to deploy and test a Data Science Pipelines Application. |
|
Change to PR detected. A new PR build was completed. |
Commit Checker results: |
|
Change to PR detected. A new PR build was completed. |
Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>
Commit Checker results: |
|
Change to PR detected. A new PR build was completed. |
|
/lgtm Tested all instructions and it worked fine. |
|
/hold When testing with jobs, it did not work. |
| // Upon run creation, update owning experiment | ||
| err = r.experimentStore.UpdateLastRun(newRun) | ||
| if err != nil { | ||
| return nil, util.Wrap(err, fmt.Sprintf("Failed to update last_run_created_at in experiment %s for run %s", newRun.ExperimentId, newRun.UUID)) | ||
| } | ||
|
|
There was a problem hiding this comment.
(language nitpicks, feel free to ignore both)
UpdateLastRun makes me think we're doing something to the run. We're actually just setting a timestamp on the experiment, so I think I would have named it SetLastRunTimestamp or similar.
I'd also say that maybe that log message knows a little too much about what's happening behind closed doors -- maybe leave the column name out ("failed to set last run timestamp on experiment")
There was a problem hiding this comment.
agree on both, will fix in a follow up pr thanks @gregsheremeta
|
/unhold
this was an issue with @rimolive environment |
|
from @rimolive :
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gmfrasca The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 2aacfe2
into
opendatahub-io:master
Commit Checker results: |
Description of your changes:
Resolves: https://issues.redhat.com/browse/RHOAIENG-7692
Testing instructions:
instruction.txt
Prepare environment:
expAandexpBrun1Aandrun1BinexpAandexpBrespectivelyselecton run table and experiment tableGo to mariadb pod, open the terminal
Confirm that you don't see a column
LastRunCreatedAtInSec.Now deploy the changes:
Update the current DSPA with the updated images, you should only need to update the API server image
Test Runs:
Now create a run named
run2A,run3A,run4AinexpAexperiment, make sure the runs are not cached.You can use this sample pipeline: here
Now either using postman or curl, the experiment and sort by
last_run_created_at:Set
API_SERVER_ROUTE,YOUR_OC_AUTH_TOKEN,YOUR_DSPA_NSaccordingly.You should see
expAandexpBlisted in a json sorted bylast_run_created_at. Confirm the presence of this fieldlast_run_created_atin every experiment listed.Note that after deploying we did not create a new run in
expB, thus we expectexpB'slast_run_created_atto be the first epoch time:"last_run_created_at": "1970-01-01T00:00:00Z"confirm this is the case. This should confirm the update scenario for existing experiments. Because of this since we are sorting bydescwe expectexpBto show up last. Now try to useascinstead ofdscconfirm the order is reversed. You can repeat this test with multiple experiments instead of just 2.We also need to test the behavior with scheduled runs because these are persisted by persistence Agent so they work differently.
Test Jobs:
run2BunderexpB. Set the cron for every 1 min.run2Botherwise you'll get spammed with runs.last_run_created_at. Confirm expected behavior.expBshould not show first if sorting bydescand thelast_run_created_atshould match this run'screation time(see this in the runs page, or curl the runs api)ascsorting has expected behaviorCreate a new experiment
exp3after you have deployed the new changes.exp3should show up last when sorting bydescbecause it has no runs. Confirm that it has the first epoch time stamp.exp3, and curl the api again and sort bylast_run_created_atconfirm expected behavior, sort againVerify DB data
Now go to mariadb, as described earlier, display the experiments table, confirm
LastRunCreatedAtInSecis populated as expected this maps tolast_run_created_at.For converting epoch, you can use this tool for relative time