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: added RawOutputDataConfig in ExecutionSpec #patch #329

Merged
merged 10 commits into from
Mar 18, 2022
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"@commitlint/cli": "^8.3.5",
"@commitlint/config-conventional": "^8.3.4",
"@date-io/moment": "1.3.9",
"@flyteorg/flyteidl": "0.21.24",
"@flyteorg/flyteidl": "0.23.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did smoke test, the new field looks good.
Is it expected, that if provided Config is not accessible it is using default value?
See here:
https://user-images.githubusercontent.com/55718143/158853593-7c554fba-b544-4017-9607-41fe7ddc1c4a.mp4
Otherwise LGTM, and I can merge it later.

Copy link
Member Author

@pingsutw pingsutw Mar 18, 2022

Choose a reason for hiding this comment

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

yes, We should use the default value if the bucket is not accessible

"@material-ui/core": "^4.0.0",
"@material-ui/icons": "^4.0.0",
"@material-ui/pickers": "^3.2.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const ExecutionMetadataExtra: React.FC<{
const commonStyles = useCommonStyles();
const styles = useStyles();

const { launchPlan: launchPlanId, maxParallelism, authRole } = execution.spec;
const { launchPlan: launchPlanId, maxParallelism, rawOutputDataConfig, authRole } = execution.spec;

const [launchPlanSpec, setLaunchPlanSpec] = React.useState<Partial<LaunchPlanSpec>>({});

Expand All @@ -53,7 +53,7 @@ export const ExecutionMetadataExtra: React.FC<{
},
{
label: ExecutionMetadataLabels.rawOutputPrefix,
value: launchPlanSpec?.rawOutputDataConfig?.outputLocationPrefix,
value: rawOutputDataConfig?.outputLocationPrefix || launchPlanSpec?.rawOutputDataConfig?.outputLocationPrefix,
},
{
label: ExecutionMetadataLabels.parallelism,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ function useRelaunchWorkflowFormState({ execution }: RelaunchExecutionFormProps)
launchPlan,
disableAll,
maxParallelism,
rawOutputDataConfig,
labels,
annotations,
authRole,
Expand All @@ -53,6 +54,7 @@ function useRelaunchWorkflowFormState({ execution }: RelaunchExecutionFormProps)
workflowId,
disableAll,
maxParallelism,
rawOutputDataConfig,
labels,
annotations,
authRole,
Expand Down
13 changes: 8 additions & 5 deletions src/components/Launch/LaunchForm/LaunchFormAdvancedInputs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@ export const LaunchFormAdvancedInputs = React.forwardRef<
setMaxParallelism(`${other.maxParallelism}`);
}
if (
launchPlan?.spec.rawOutputDataConfig?.outputLocationPrefix !== undefined &&
launchPlan?.spec.rawOutputDataConfig.outputLocationPrefix !== null
other?.rawOutputDataConfig?.outputLocationPrefix !== undefined &&
other.rawOutputDataConfig.outputLocationPrefix !== null
) {
setRawOutputDataConfig(launchPlan.spec.rawOutputDataConfig.outputLocationPrefix);
setRawOutputDataConfig(other.rawOutputDataConfig.outputLocationPrefix);
}
const newLabels = {
...(other.labels?.values || {}),
Expand All @@ -100,14 +100,17 @@ export const LaunchFormAdvancedInputs = React.forwardRef<
};
setLabelsParamData(newLabels);
setAnnotationsParamData(newAnnotations);
}, [other.disableAll, other.maxParallelism, other.labels, other.annotations, launchPlan?.spec]);
}, [other.disableAll, other.maxParallelism, other.rawOutputDataConfig, other.labels, other.annotations, launchPlan?.spec]);

React.useImperativeHandle(
ref,
() => ({
getValues: () => {
return {
disableAll,
rawOutputDataConfig: {
outputLocationPrefix: rawOutputDataConfig
},
maxParallelism: parseInt(maxParallelism || '', 10),
labels: {
values: labelsParamData,
Expand All @@ -121,7 +124,7 @@ export const LaunchFormAdvancedInputs = React.forwardRef<
return true;
},
}),
[disableAll, maxParallelism, labelsParamData, annotationsParamData],
[disableAll, maxParallelism, rawOutputDataConfig, labelsParamData, annotationsParamData],
);

const handleDisableAllChange = React.useCallback(() => {
Expand Down
1 change: 1 addition & 0 deletions src/components/Launch/LaunchForm/launchMachine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export interface WorkflowLaunchContext extends BaseLaunchContext {
defaultAuthRole?: Admin.IAuthRole;
disableAll?: boolean | null;
maxParallelism?: number | null;
rawOutputDataConfig?: Admin.IRawOutputDataConfig | null;
labels?: Admin.ILabels | null;
annotations?: Admin.IAnnotations | null;
securityContext?: Core.ISecurityContext | null;
Expand Down
1 change: 1 addition & 0 deletions src/components/Launch/LaunchForm/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export interface WorkflowInitialLaunchParameters extends BaseInitialLaunchParame
securityContext?: Core.ISecurityContext;
disableAll?: boolean | null;
maxParallelism?: number | null;
rawOutputDataConfig?: Admin.IRawOutputDataConfig | null;
labels?: Admin.ILabels | null;
annotations?: Admin.IAnnotations | null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ async function submit(

const { authRole, securityContext } = roleInputRef.current?.getValue() as LaunchRoles;
const literals = formInputsRef.current.getValues();
const { disableAll, labels, annotations, maxParallelism } =
const { disableAll, labels, annotations, maxParallelism, rawOutputDataConfig } =
advancedOptionsRef.current?.getValues() || {};
const launchPlanId = launchPlan.id;
const { domain, project } = workflowVersion;
Expand All @@ -179,6 +179,7 @@ async function submit(
securityContext,
disableAll,
maxParallelism,
rawOutputDataConfig,
domain,
labels,
launchPlanId,
Expand Down Expand Up @@ -241,6 +242,7 @@ export function useLaunchWorkflowFormState({
values: defaultInputValues,
disableAll,
maxParallelism,
rawOutputDataConfig,
labels,
annotations,
securityContext,
Expand Down Expand Up @@ -273,6 +275,7 @@ export function useLaunchWorkflowFormState({
sourceId,
disableAll,
maxParallelism,
rawOutputDataConfig,
labels,
annotations,
},
Expand Down
2 changes: 1 addition & 1 deletion src/components/Literals/__stories__/Collection.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const stories = storiesOf('Literals/Collection', module);
stories.addDecorator(CardDecorator);

function renderCollection(label: string, collection: LiteralCollection) {
return <LiteralValue label={label} literal={{ collection, value: 'collection' }} />;
return <LiteralValue label={label} literal={{ collection, value: 'collection', hash: '' }} />;
}

stories.add('Binary', () =>
Expand Down
2 changes: 1 addition & 1 deletion src/components/Literals/__stories__/Map.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const stories = storiesOf('Literals/Map', module);
stories.addDecorator(CardDecorator);

function renderMap(label: string, map: LiteralMap) {
return <LiteralValue label={label} literal={{ map, value: 'map' }} />;
return <LiteralValue label={label} literal={{ map, value: 'map', hash: '' }} />;
}

stories.add('Binary', () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function renderStruct(label: string, struct: ProtobufStruct) {
literal={{
scalar: { value: 'generic', generic: struct },
value: 'scalar',
hash: '',
}}
/>
);
Expand Down
4 changes: 3 additions & 1 deletion src/components/Literals/__stories__/literalValues.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ import {
* each value in a `Literal` and setting the appropriate `value` to allow
* lookup of whichever field is populated.
*/
function toLiterals<T>(type: keyof Core.ILiteral, values: Dictionary<T>): Dictionary<Literal> {
function toLiterals<T>(type: 'scalar' | 'collection' | 'map', values: Dictionary<T>): Dictionary<Literal> {
return mapValues(values, (value: T) => ({
value: type,
[type]: value,
hash: '',
}));
}

Expand All @@ -30,4 +31,5 @@ export const schemaLiterals = toLiterals('scalar', schemaScalars);
export const noneTypeLiteral: Literal = {
value: 'scalar',
scalar: noneTypeScalar,
hash: ''
};
3 changes: 2 additions & 1 deletion src/models/Common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,11 @@ export type UrlBlob = Admin.IUrlBlob;
export type Error = RequiredNonNullable<Core.IError>;

export interface Literal extends Core.Literal {
value: keyof Core.ILiteral;
value: 'scalar' | 'collection' | 'map';
collection?: Core.ILiteralCollection;
map?: Core.ILiteralMap;
scalar?: Scalar;
hash: string;
}

/** A Core.ILiteral guaranteed to have all subproperties necessary to specify
Expand Down
6 changes: 6 additions & 0 deletions src/models/Execution/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export interface CreateWorkflowExecutionArguments {
disableAll?: boolean | null;
labels?: Admin.ILabels | null;
maxParallelism?: number | null;
rawOutputDataConfig?: Admin.IRawOutputDataConfig | null;
inputs: Core.ILiteralMap;
launchPlanId: Identifier;
project: string;
Expand All @@ -102,6 +103,7 @@ export const createWorkflowExecution = (
disableAll,
labels,
maxParallelism,
rawOutputDataConfig,
inputs,
launchPlanId: launchPlan,
project,
Expand Down Expand Up @@ -136,6 +138,10 @@ export const createWorkflowExecution = (
spec.maxParallelism = maxParallelism;
}

if (rawOutputDataConfig?.outputLocationPrefix) {
spec.rawOutputDataConfig = rawOutputDataConfig;
}

return postAdminEntity<Admin.IExecutionCreateRequest, Admin.ExecutionCreateResponse>(
{
data: {
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1731,10 +1731,10 @@
minimatch "^3.0.4"
strip-json-comments "^3.1.1"

"@flyteorg/flyteidl@0.21.24":
version "0.21.24"
resolved "https://registry.yarnpkg.com/@flyteorg/flyteidl/-/flyteidl-0.21.24.tgz#08e2c53a56d09ddcf2d6a2c3c183ce4d8231c23e"
integrity sha512-1gcbmTKS3sppPqE61zgYhk93DQh6vf36qHftjQ6bdMMeozM6JRwbYk/cKbyrc3DCk+PGoxf46jjSXSK1NJhASQ==
"@flyteorg/flyteidl@0.23.1":
version "0.23.1"
resolved "https://registry.yarnpkg.com/@flyteorg/flyteidl/-/flyteidl-0.23.1.tgz#da88166e1c0bd404a4f00db425fe07afaaa76a8e"
integrity sha512-3M6o3EObbE35cFoY88/DNPcKEp+g2mOuFc2DNlCFntKH1lsB+EEl9k+chuWy8F8iWe7DHkCKJvM5AeEjSseahg==

"@gar/promisify@^1.0.1":
version "1.1.3"
Expand Down