Skip to content
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

[UI] Show execution details in Run Details Page ML Metadata tab of steps #3457

Merged
merged 4 commits into from
Apr 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions frontend/src/components/Router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ export const RoutePageFactory = {
artifactType,
).replace(`:${RouteParams.ID}`, '' + artifactId);
},
executionDetails: (executionType: string, executionId: number) => {
return RoutePage.EXECUTION_DETAILS.replace(
`:${RouteParams.EXECUTION_TYPE}+`,
executionType,
).replace(`:${RouteParams.ID}`, '' + executionId);
},
pipelineDetails: (id: string) => {
return RoutePage.PIPELINE_DETAILS_NO_VERSION.replace(`:${RouteParams.pipelineId}`, id);
},
Expand Down
100 changes: 100 additions & 0 deletions frontend/src/lib/MlmdUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import {
Context,
Api,
Execution,
getResourceProperty,
ExecutionProperties,
ExecutionCustomProperties,
} from '@kubeflow/frontend';
import {
GetContextByTypeAndNameRequest,
GetExecutionsByContextRequest,
} from '@kubeflow/frontend/src/mlmd/generated/ml_metadata/proto/metadata_store_service_pb';

async function getContext({ type, name }: { type: string; name: string }): Promise<Context> {
const request = new GetContextByTypeAndNameRequest();
request.setTypeName(type);
request.setContextName(name);
try {
const res = await Api.getInstance().metadataStoreService.getContextByTypeAndName(request);
const context = res.getContext();
if (context == null) {
throw new Error('Cannot find specified context');
}
return context;
} catch (err) {
err.message = `Cannot find context with ${JSON.stringify(request.toObject())}: ` + err.message;
throw err;
}
}

/**
* @throws error when network error, or not found
*/
export async function getTfxRunContext(argoWorkflowName: string): Promise<Context> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this remind me of one thing. See https://github.com/tensorflow/tfx/blob/5ee3de9c77a92e904010c7656d6269d3b28744bf/tfx/orchestration/kubeflow/base_component.py#L136

Users can actually override the TFX run id to be something different than Argo workflow ID, which might cause problem to this piece of logic.

@neuromage Not sure how useful that customization is currently. Shall we consider closing the door of overriding run_id?

Copy link
Contributor Author

@Bobgy Bobgy Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! that's a good catch.
From the comment

# Allow overriding pipeline's run_id externally, primarily for testing.

looks like it was for testing.
+1 for closing the door, or intentionally keep it only for testing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this remind me of one thing. See https://github.com/tensorflow/tfx/blob/5ee3de9c77a92e904010c7656d6269d3b28744bf/tfx/orchestration/kubeflow/base_component.py#L136

Users can actually override the TFX run id to be something different than Argo workflow ID, which might cause problem to this piece of logic.

@neuromage Not sure how useful that customization is currently. Shall we consider closing the door of overriding run_id?

Can we add something explicit like workflow_name that's not overridable?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add something explicit like workflow_name that's not overridable?

Do you mean here or on the TFX side?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the TFX side.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the TFX side.

Yes. I think I'll make a CL closing the door.

// argoPodName has the general form "pipelineName-workflowId-executionId".
// All components of a pipeline within a single run will have the same
// "pipelineName-workflowId" prefix.
const pipelineName = argoWorkflowName
.split('-')
.slice(0, -1)
.join('_');
const runID = argoWorkflowName;
// An example run context name is parameterized_tfx_oss.parameterized-tfx-oss-4rq5v.
const tfxRunContextName = `${pipelineName}.${runID}`;
return await getContext({ name: tfxRunContextName, type: 'run' });
}

/**
* @throws error when network error, or not found
*/
export async function getKfpRunContext(argoWorkflowName: string): Promise<Context> {
return await getContext({ name: argoWorkflowName, type: 'KfpRun' });
}

/**
* @throws error when network error
*/
export async function getExecutionsFromContext(context: Context): Promise<Execution[]> {
const request = new GetExecutionsByContextRequest();
request.setContextId(context.getId());
try {
const res = await Api.getInstance().metadataStoreService.getExecutionsByContext(request);
const list = res.getExecutionsList();
if (list == null) {
throw new Error('response.getExecutionsList() is empty');
}
return list;
} catch (err) {
err.message =
`Cannot find executions by context ${context.getId()} with name ${context.getName()}: ` +
err.message;
throw err;
}
}

export enum KfpExecutionProperties {
KFP_POD_NAME = 'kfp_pod_name',
}

export const ExecutionHelpers = {
getPipeline(execution: Execution): string | number | undefined {
return (
getResourceProperty(execution, ExecutionProperties.PIPELINE_NAME) ||
getResourceProperty(execution, ExecutionCustomProperties.WORKSPACE, true) ||
getResourceProperty(execution, ExecutionCustomProperties.RUN_ID, true) ||
undefined
);
},
getName(execution: Execution): string | number | undefined {
return (
getResourceProperty(execution, ExecutionProperties.NAME) ||
getResourceProperty(execution, ExecutionProperties.COMPONENT_ID) ||
getResourceProperty(execution, ExecutionCustomProperties.TASK_ID, true) ||
undefined
);
},
getState(execution: Execution): string | number | undefined {
return getResourceProperty(execution, ExecutionProperties.STATE) || undefined;
},
};
152 changes: 117 additions & 35 deletions frontend/src/pages/ExecutionDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,49 +30,87 @@ import {
getArtifactTypes,
getResourceProperty,
logger,
titleCase,
ExecutionType,
} from '@kubeflow/frontend';
import { CircularProgress } from '@material-ui/core';
import React, { Component } from 'react';
import { Link } from 'react-router-dom';
import { classes, stylesheet } from 'typestyle';
import { Page } from './Page';
import { Page, PageErrorHandler } from './Page';
import { ToolbarProps } from '../components/Toolbar';
import { RoutePage, RouteParams, RoutePageFactory } from '../components/Router';
import { commonCss, padding } from '../Css';
import { ResourceInfo, ResourceType } from '../components/ResourceInfo';
import { serviceErrorToString } from '../lib/Utils';
import { GetExecutionTypesByIDRequest } from '@kubeflow/frontend/src/mlmd/generated/ml_metadata/proto/metadata_store_service_pb';

type ArtifactIdList = number[];

interface ExecutionDetailsState {
execution?: Execution;
executionType?: ExecutionType;
events?: Record<Event.Type, ArtifactIdList>;
artifactTypeMap?: Map<number, ArtifactType>;
}

export default class ExecutionDetails extends Page<{}, ExecutionDetailsState> {
constructor(props: {}) {
super(props);
this.state = {};
this.load = this.load.bind(this);
public state: ExecutionDetailsState = {};

private get id(): number {
return parseInt(this.props.match.params[RouteParams.ID], 10);
}

private get fullTypeName(): string {
return this.props.match.params[RouteParams.EXECUTION_TYPE] || '';
public render(): JSX.Element {
return (
<div className={classes(commonCss.page, padding(20, 'lr'))}>
<ExecutionDetailsContent
key={this.id}
id={this.id}
onError={this.showPageError.bind(this)}
onTitleUpdate={title =>
this.props.updateToolbar({
pageTitle: title,
})
}
/>
</div>
);
}

private get properTypeName(): string {
const parts = this.fullTypeName.split('/');
if (!parts.length) {
return '';
}
public getInitialToolbarState(): ToolbarProps {
return {
actions: {},
breadcrumbs: [{ displayName: 'Executions', href: RoutePage.EXECUTIONS }],
pageTitle: `${this.id} details`,
};
}

return titleCase(parts[parts.length - 1]);
public async refresh(): Promise<void> {
// do nothing
}
}

private get id(): string {
return this.props.match.params[RouteParams.ID];
interface ExecutionDetailsContentProps {
id: number;
onError: PageErrorHandler;
onTitleUpdate: (title: string) => void;
}
export class ExecutionDetailsContent extends Component<
ExecutionDetailsContentProps,
ExecutionDetailsState
> {
public state: ExecutionDetailsState = {};

private get fullTypeName(): string {
// This can be called during constructor, so this.state may not be initialized.
if (this.state) {
const { executionType } = this.state;
const name = executionType?.getName();
if (name) {
return name;
}
}
return '';
}

public async componentDidMount(): Promise<void> {
Expand All @@ -85,11 +123,11 @@ export default class ExecutionDetails extends Page<{}, ExecutionDetailsState> {
}

return (
<div className={classes(commonCss.page, padding(20, 'lr'))}>
<div>
{
<ResourceInfo
resourceType={ResourceType.EXECUTION}
typeName={this.properTypeName}
typeName={this.fullTypeName}
resource={this.state.execution}
/>
}
Expand Down Expand Up @@ -121,15 +159,15 @@ export default class ExecutionDetails extends Page<{}, ExecutionDetailsState> {
return {
actions: {},
breadcrumbs: [{ displayName: 'Executions', href: RoutePage.EXECUTIONS }],
pageTitle: `${this.properTypeName} ${this.id} details`,
pageTitle: `${this.fullTypeName} ${this.props.id} details`,
};
}

public async refresh(): Promise<void> {
return this.load();
}
private refresh = async (): Promise<void> => {
await this.load();
};

private async load(): Promise<void> {
private load = async (): Promise<void> => {
const metadataStoreServiceClient = Api.getInstance().metadataStoreService;

// this runs parallelly because it's not a critical resource
Expand All @@ -140,13 +178,13 @@ export default class ExecutionDetails extends Page<{}, ExecutionDetailsState> {
});
})
.catch(err => {
this.showPageError('Failed to fetch artifact types', err);
this.props.onError('Failed to fetch artifact types', err, 'warning', this.refresh);
});

const numberId = parseInt(this.id, 10);
const numberId = this.props.id;
if (isNaN(numberId) || numberId < 0) {
const error = new Error(`Invalid execution id: ${this.id}`);
this.showPageError(error.message, error);
const error = new Error(`Invalid execution id: ${this.props.id}`);
this.props.onError(error.message, error, 'error', this.refresh);
return;
}

Expand All @@ -162,31 +200,67 @@ export default class ExecutionDetails extends Page<{}, ExecutionDetailsState> {
]);

if (!executionResponse.getExecutionsList().length) {
this.showPageError(`No ${this.fullTypeName} identified by id: ${this.id}`);
this.props.onError(
`No ${this.fullTypeName} identified by id: ${this.props.id}`,
undefined,
'error',
this.refresh,
);
return;
}

if (executionResponse.getExecutionsList().length > 1) {
this.showPageError(`Found multiple executions with ID: ${this.id}`);
this.props.onError(
`Found multiple executions with ID: ${this.props.id}`,
undefined,
'error',
this.refresh,
);
return;
}

const execution = executionResponse.getExecutionsList()[0];
const executionName =
getResourceProperty(execution, ExecutionProperties.COMPONENT_ID) ||
getResourceProperty(execution, ExecutionCustomProperties.TASK_ID, true);
this.props.updateToolbar({
pageTitle: executionName ? executionName.toString() : '',
});
this.props.onTitleUpdate(executionName ? executionName.toString() : '');

const typeRequest = new GetExecutionTypesByIDRequest();
typeRequest.setTypeIdsList([execution.getTypeId()]);
const typeResponse = await metadataStoreServiceClient.getExecutionTypesByID(typeRequest);
const types = typeResponse.getExecutionTypesList();
let executionType: ExecutionType | undefined;
if (!types || types.length === 0) {
this.props.onError(
`Cannot find execution type with id: ${execution.getTypeId()}`,
undefined,
'error',
this.refresh,
);
return;
} else if (types.length > 1) {
this.props.onError(
`More than one execution type found with id: ${execution.getTypeId()}`,
undefined,
'error',
this.refresh,
);
return;
} else {
executionType = types[0];
}

const events = parseEventsByType(eventResponse);

this.setState({
events,
execution,
executionType,
});
} catch (err) {
this.showPageError(serviceErrorToString(err));
this.props.onError(serviceErrorToString(err), err, 'error', this.refresh);
}
}
};
}

function parseEventsByType(
Expand Down Expand Up @@ -332,7 +406,15 @@ const ArtifactRow: React.FC<{ id: number; name: string; type?: string; uri: stri
id
)}
</td>
<td className={css.tableCell}>{name}</td>
<td className={css.tableCell}>
{type && id ? (
<Link className={commonCss.link} to={RoutePageFactory.artifactDetails(type, id)}>
{name}
</Link>
) : (
name
)}
</td>
<td className={css.tableCell}>{type}</td>
<td className={css.tableCell}>{uri}</td>
</tr>
Expand Down
Loading