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

feat: Pre-emptively decode binary literals #889

Merged
merged 4 commits into from
Nov 12, 2024
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
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
"dependencies": {
"@commitlint/cli": "^17.3.0",
"@commitlint/config-conventional": "^17.3.0",
"@msgpack/msgpack": "^3.0.0-beta2",
"@semantic-release/changelog": "^5.0.1",
"@semantic-release/commit-analyzer": "^8.0.1",
"@semantic-release/exec": "^6.0.3",
Expand Down
2 changes: 1 addition & 1 deletion packages/common/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"tslib": "^2.4.1"
},
"dependencies": {
"@flyteorg/flyteidl": "^1.10.7",
"@flyteorg/flyteidl": "^1.13.5",
"@protobuf-ts/runtime": "^2.6.0",
"@protobuf-ts/runtime-rpc": "^2.6.0"
},
Expand Down
5 changes: 5 additions & 0 deletions packages/common/src/flyteidl/google.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { google } from '@flyteorg/flyteidl/gen/pb-js/flyteidl';

/** Message classes for google entities */

export default google;
1 change: 1 addition & 0 deletions packages/oss-console/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"lossless-json": "^1.0.3",
"memoize-one": "^5.0.0",
"moment-timezone": "^0.5.28",
"msgpackr": "^1.11.2",
"msw": "^1.3.2",
"notistack": "^3.0.1",
"object-hash": "^1.3.1",
Expand Down
1 change: 1 addition & 0 deletions packages/oss-console/src/App/index.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import '../common/setupProtobuf';
import '../common/decodeMsgPackLiterals';

Check warning on line 2 in packages/oss-console/src/App/index.tsx

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/App/index.tsx#L2

Added line #L2 was not covered by tests
import React, { PropsWithChildren } from 'react';
import Collapse from '@mui/material/Collapse';
import { FlyteApiProvider } from '@clients/flyte-api/ApiProvider';
Expand Down
64 changes: 64 additions & 0 deletions packages/oss-console/src/common/decodeMsgPackLiterals.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import core from '@clients/common/flyteidl/core';
import google from '@clients/common/flyteidl/google';

Check warning on line 2 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L1-L2

Added lines #L1 - L2 were not covered by tests
import $protobuf from 'protobufjs';
import { unpack } from 'msgpackr';
import isNil from 'lodash/isNil';
import entries from 'lodash/entries';

Check warning on line 6 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L4-L6

Added lines #L4 - L6 were not covered by tests

// Convert a JavaScript object to Protobuf Struct
function convertToStruct(obj: any) {
const struct = new google.protobuf.Struct({

Check warning on line 10 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L9-L10

Added lines #L9 - L10 were not covered by tests
fields: {},
});

entries(obj).forEach(([key, value]) => {
struct.fields[key] = convertToValue(value);

Check warning on line 15 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View workflow job for this annotation

GitHub Actions / lint_project

'convertToValue' was used before it was defined

Check warning on line 15 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L14-L15

Added lines #L14 - L15 were not covered by tests
});

return struct;

Check warning on line 18 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L18

Added line #L18 was not covered by tests
}

// Convert values to Protobuf Value type
function convertToValue(value: any) {
const protoValue = new google.protobuf.Value();

Check warning on line 23 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L22-L23

Added lines #L22 - L23 were not covered by tests

if (Array.isArray(value)) {
const listValues = value.map(convertToValue);
protoValue.listValue = new google.protobuf.ListValue({ values: listValues });
} else if (typeof value === 'object' && value !== null) {
protoValue.structValue = convertToStruct(value);
} else if (typeof value === 'string') {
protoValue.stringValue = value;
} else if (typeof value === 'number') {
protoValue.numberValue = value;
} else if (typeof value === 'boolean') {
protoValue.boolValue = value;
} else if (value === null) {
protoValue.nullValue = google.protobuf.NullValue.NULL_VALUE;

Check warning on line 37 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L25-L37

Added lines #L25 - L37 were not covered by tests
}

return protoValue;

Check warning on line 40 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L40

Added line #L40 was not covered by tests
}

const originalLiteralDecode = core.Literal.decode;

Check warning on line 43 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L43

Added line #L43 was not covered by tests
// Overriding the decode method of Literal to convert msgpack binary literals to protobuf structs
core.Literal.decode = (reader: $protobuf.Reader | Uint8Array, length?: number) => {
const result = originalLiteralDecode(reader, length);

Check warning on line 46 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L45-L46

Added lines #L45 - L46 were not covered by tests

if (result?.scalar?.binary?.tag === 'msgpack') {

Check warning on line 48 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L48

Added line #L48 was not covered by tests
// We know that a binary literal with tag 'msgpack' is a STRUCT
const value = result?.scalar?.binary?.value;
const msgpackResult = isNil(value) ? value : unpack(value);

Check warning on line 51 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L50-L51

Added lines #L50 - L51 were not covered by tests
// Convert the msgpack result to a protobuf struct
const protobufStruct = convertToStruct(msgpackResult);

Check warning on line 53 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L53

Added line #L53 was not covered by tests

return {

Check warning on line 55 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L55

Added line #L55 was not covered by tests
metadata: result.metadata,
hash: result.hash,
scalar: {
generic: protobufStruct,
},
} as core.Literal;
}
return result;

Check warning on line 63 in packages/oss-console/src/common/decodeMsgPackLiterals.ts

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/common/decodeMsgPackLiterals.ts#L63

Added line #L63 was not covered by tests
};
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import Chip from '@mui/material/Chip';
import makeStyles from '@mui/styles/makeStyles';

type ValuesType = {[p: string]: string};
type ValuesType = { [p: string]: string };
interface Props {
values: ValuesType;
}
Expand All @@ -12,21 +12,20 @@ const useStyles = makeStyles({
display: 'flex',
flexWrap: 'wrap',
width: '100%',
maxWidth: '420px'
maxWidth: '420px',
},
chip: {
margin: '2px 2px 2px 0',
},
});


export const ExecutionLabels: React.FC<Props> = ({values}) => {
export const ExecutionLabels: React.FC<Props> = ({ values }) => {
const classes = useStyles();
return (
<div className={classes.chipContainer}>
{Object.entries(values).map(([key, value]) => (
<Chip
color='info'
color="info"
key={key}
label={value ? `${key}: ${value}` : key}
className={classes.chip}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,7 @@
const workflowId = execution?.closure?.workflowId;

const { labels } = execution.spec;
const {
referenceExecution,
systemMetadata ,
parentNodeExecution,
} = execution.spec.metadata;
const { referenceExecution, systemMetadata, parentNodeExecution } = execution.spec.metadata;
const cluster = systemMetadata?.executionCluster ?? dashedValueString;

const details: DetailItem[] = [
Expand Down Expand Up @@ -130,11 +126,13 @@
if (labels != null && labels.values != null) {
details.push({
label: ExecutionMetadataLabels.labels,
value: (
Object.entries(labels.values).length > 0 ?
<ExecutionLabels values={labels.values} /> : dashedValueString
)
})
value:
Object.entries(labels.values).length > 0 ? (
<ExecutionLabels values={labels.values} />
) : (
dashedValueString

Check warning on line 133 in packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionMetadata.tsx

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionMetadata.tsx#L133

Added line #L133 was not covered by tests
),
});
}

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@

const config =
// eslint-disable-next-line no-nested-ternary
env.CODE_SNIPPET_USE_AUTO_CONFIG === "true"
env.CODE_SNIPPET_USE_AUTO_CONFIG === 'true'

Check warning on line 37 in packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionNodeURL.tsx

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionNodeURL.tsx#L37

Added line #L37 was not covered by tests
? 'Config.auto()'
: isHttps
? // https snippet
`Config.for_endpoint("${window.location.host}")`
: // http snippet
`Config.for_endpoint("${window.location.host}", True)`;
const code = `from flytekit.remote.remote import FlyteRemote
const code = `from flytekit.remote.remote import FlyteRemote

Check warning on line 44 in packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionNodeURL.tsx

View check run for this annotation

Codecov / codecov/patch

packages/oss-console/src/components/Executions/ExecutionDetails/ExecutionNodeURL.tsx#L44

Added line #L44 was not covered by tests
from flytekit.configuration import Config
remote = FlyteRemote(
${config},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,31 @@ import '@testing-library/jest-dom';
import { ExecutionLabels } from '../ExecutionLabels';

jest.mock('@mui/material/Chip', () => (props: any) => (
<div data-testid="chip" {...props}>{props.label}</div>
<div data-testid="chip" {...props}>
{props.label}
</div>
));

describe('ExecutionLabels', () => {
it('renders chips with key-value pairs correctly', () => {
const values = {
'random/uuid': 'f8b9ff18-4811-4bcc-aefd-4f4ec4de469d',
'bar': 'baz',
'foo': '',
bar: 'baz',
foo: '',
};

render(<ExecutionLabels values={values} />);

expect(screen.getByText('random/uuid: f8b9ff18-4811-4bcc-aefd-4f4ec4de469d')).toBeInTheDocument();
expect(
screen.getByText('random/uuid: f8b9ff18-4811-4bcc-aefd-4f4ec4de469d'),
).toBeInTheDocument();
expect(screen.getByText('bar: baz')).toBeInTheDocument();
expect(screen.getByText('foo')).toBeInTheDocument();
});

it('applies correct styles to chip container', () => {
const values = {
'key': 'value',
key: 'value',
};

const { container } = render(<ExecutionLabels values={values} />);
Expand All @@ -38,9 +42,9 @@ describe('ExecutionLabels', () => {

it('renders correct number of chips', () => {
const values = {
'key1': 'value1',
'key2': 'value2',
'key3': 'value3',
key1: 'value1',
key2: 'value2',
key3: 'value3',
};

render(<ExecutionLabels values={values} />);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const durationTestId = `metadata-${ExecutionMetadataLabels.duration}`;
const interruptibleTestId = `metadata-${ExecutionMetadataLabels.interruptible}`;
const overwriteCacheTestId = `metadata-${ExecutionMetadataLabels.overwriteCache}`;
const relatedToTestId = `metadata-${ExecutionMetadataLabels.relatedTo}`;
const parentNodeExecutionTestId = `metadata-${ExecutionMetadataLabels.parent}`
const parentNodeExecutionTestId = `metadata-${ExecutionMetadataLabels.parent}`;
const labelsTestId = `metadata-${ExecutionMetadataLabels.labels}`;

jest.mock('../../../../models/Launch/api', () => ({
Expand Down Expand Up @@ -113,15 +113,15 @@ describe('ExecutionMetadata', () => {
it('shows related to if metadata is available', () => {
const { getByTestId } = renderMetadata();
expect(getByTestId(relatedToTestId)).toHaveTextContent('name');
})
});

it('shows parent execution if metadata is available', () => {
const { getByTestId } = renderMetadata();
expect(getByTestId(parentNodeExecutionTestId)).toHaveTextContent('name');
})
});

it('shows labels if spec has them', () => {
const { getByTestId } = renderMetadata();
expect(getByTestId(labelsTestId)).toHaveTextContent("key: value");
})
expect(getByTestId(labelsTestId)).toHaveTextContent('key: value');
});
});
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import React, { FC, useCallback, useEffect, useMemo, useState } from 'react';
import React, { FC, useCallback, useMemo, useState } from 'react';
import { Form } from '@rjsf/mui';
import validator from '@rjsf/validator-ajv8';
import styled from '@mui/system/styled';
import * as msgpack from '@msgpack/msgpack';
import { InputProps } from '../types';
import { protobufValueToPrimitive, PrimitiveType } from '../inputHelpers/struct';
import { StyledCard } from './StyledCard';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import Protobuf from '@clients/common/flyteidl/protobuf';
import Core from '@clients/common/flyteidl/core';
import * as msgpack from '@msgpack/msgpack';
import { InputType, InputValue } from '../types';
import { structPath } from './constants';
import { ConverterInput, InputHelper, InputValidatorParams } from './types';
import { extractLiteralWithCheck, formatParameterValues } from './utils';

export type PrimitiveType = string | number | boolean | null | object;

function asValueWithKind(value: Protobuf.IValue): Protobuf.Value {
export function asValueWithKind(value: Protobuf.IValue): Protobuf.Value {
return value instanceof Protobuf.Value ? value : Protobuf.Value.create(value);
}

Expand Down Expand Up @@ -91,25 +90,9 @@ function objectToProtobufStruct(obj: Dictionary<any>): Protobuf.IStruct {
return { fields };
}

function parseBinary(binary: Core.IBinary): string {
if (!binary.value) {
throw new Error('Binary value is empty');
}

if (binary.tag === 'msgpack') {
return JSON.stringify(msgpack.decode(binary.value));
}

// unsupported binary type, it might be temporary
return '';
}

function fromLiteral(literal: Core.ILiteral): InputValue {
if (literal.scalar?.binary) {
return parseBinary(literal.scalar.binary);
}

const structValue = extractLiteralWithCheck<Protobuf.IStruct>(literal, structPath);

const finalValue = formatParameterValues(InputType.Struct, protobufStructToObject(structValue));
return finalValue;
}
Expand Down
Loading
Loading