UPSTREAM: <carry>: reconcile task states with run state on cancellation/failure - #397
UPSTREAM: <carry>: reconcile task states with run state on cancellation/failure#397jira-autofix[bot] wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Commit Checker results: |
📝 WalkthroughWalkthroughSecurity note (CWE-693): This change alters client-side state derivation from run status; no dependency, CI/CD, IDE config, or
Estimated code review effort: 2 (Simple) | ~15 minutes Changes
Related issues: None provided. Related PRs: None provided. Suggested labels: frontend, bug-fix Suggested reviewers: None provided. Poem: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
|
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/v1
kind: DataSciencePipelinesApplication
metadata:
name: pr-397
spec:
dspVersion: v2
apiServer:
image: "quay.io/opendatahub/ds-pipelines-api-server:pr-397"
argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-397"
argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-397"
persistenceAgent:
image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-397"
scheduledWorkflow:
image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-397"
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-397"
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/397/head
git checkout -b pullrequest dda95204c39640037de7563846c1c138d59d0ac8
oc apply -f dspa.pr-397.yamlMore instructions here on how to deploy and test a Data Science Pipelines Application. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/lib/v2/DynamicFlow.test.ts (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap: parallel-for and sub-DAG branches untested.
All four new tests exercise only the
NodeTypeNames.EXECUTIONbranch (DynamicFlow.tslines 298-301). Neither the parallel-for sub-DAG branch (lines 280-283) nor theNodeTypeNames.SUB_DAGbranch (lines 332-335) — both of which also hadgetLastKnownState()replaced withreconcileExecutionState(...)— are covered by a run-state test case.Also applies to: 147-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/v2/DynamicFlow.test.ts` at line 28, Add run-state test coverage in DynamicFlow.test for the branches in DynamicFlow that handle parallel-for sub-DAG and NodeTypeNames.SUB_DAG, not just NodeTypeNames.EXECUTION. Update or add cases around the DynamicFlow logic that now calls reconcileExecutionState(...) in those branches, and assert the resulting state behavior from getRunState()/run-state handling using the existing V2beta1RuntimeState test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/lib/v2/DynamicFlow.test.ts`:
- Line 28: Add run-state test coverage in DynamicFlow.test for the branches in
DynamicFlow that handle parallel-for sub-DAG and NodeTypeNames.SUB_DAG, not just
NodeTypeNames.EXECUTION. Update or add cases around the DynamicFlow logic that
now calls reconcileExecutionState(...) in those branches, and assert the
resulting state behavior from getRunState()/run-state handling using the
existing V2beta1RuntimeState test setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 856c5cbe-9965-4d45-859a-b7056ed9c0d2
📒 Files selected for processing (3)
frontend/src/lib/v2/DynamicFlow.test.tsfrontend/src/lib/v2/DynamicFlow.tsfrontend/src/pages/RunDetailsV2.tsx
…on/failure When a pipeline run is cancelled or fails, individual tasks that are still in RUNNING or NEW state in MLMD are now overridden to show CANCELED or FAILED in the UI. Previously, updateFlowElementsState only read MLMD execution states without considering the run-level state, so cancelled runs would still display in-progress tasks as running. **Description of your changes:** - Add runState parameter to updateFlowElementsState() in DynamicFlow.ts - Add reconcileExecutionState() helper that overrides RUNNING/NEW execution states to CANCELED or FAILED when the run is terminated - Pass run.state from RunDetailsV2.tsx into the flow state update - Add tests for RUNNING→CANCELED, RUNNING→FAILED, NEW→CANCELED overrides, and verify no override when run is still RUNNING **Checklist:** - [x] The title for your pull request (PR) follows the title convention - [x] Make sure the changes work with DSPO Ref: RHOAIENG-74717 Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
jira-autofix
Bot
changed the title
dda9520 to
4649ac1
Compare
Commit Checker results: |
|
Change to PR detected. A new PR build was completed. |
1 participant
Description of your changes:
When a pipeline run is canceled or fails, the RHOAI UI was showing individual tasks as still "progressing" even though backend pods had errored out. This fix reconciles individual task display states with the run-level state.
Changes:
isRunTerminated()andreconcileExecutionState()helpers inDynamicFlow.tsthat override RUNNING/NEW execution states to CANCELED/FAILED when the run itself is terminatedrun.statefromRunDetailsV2.tsxintoupdateFlowElementsState()and added it to the memo dependency arrayFixes: RHOAIENG-74717
Checklist: