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.

Sign up for GitHub

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

Add in proper handling of spaces and quotes in ISVC parsing #3592

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add in proper handling of spaces and quotes in ISVC parsing #3592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions frontend/src/api/k8s/inferenceServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { ContainerResources } from '~/types';
import { AcceleratorProfileFormData } from '~/utilities/useAcceleratorProfileFormState';
import { AcceleratorProfileState } from '~/utilities/useReadAcceleratorState';
import { getModelServingProjects } from './projects';
import { assemblePodSpecOptions } from './utils';
import { assemblePodSpecOptions, parseCommandLine } from './utils';

export const assembleInferenceService = (
data: CreatingInferenceServiceObject,
Expand Down Expand Up @@ -45,6 +45,8 @@ export const assembleInferenceService = (
const dataConnectionKey = secretKey || dataConnection;

const nonEmptyArgs = servingRuntimeArgs?.filter(Boolean) || [];
// Ensure that we properly handle separating args
const splitArgs: string[] = nonEmptyArgs.flatMap(parseCommandLine);
const nonEmptyEnvVars = servingRuntimeEnvVars?.filter((ev) => ev.name) || [];

const updateInferenceService: InferenceServiceKind = inferenceService
Expand Down Expand Up @@ -87,7 +89,7 @@ export const assembleInferenceService = (
path,
},
}),
args: nonEmptyArgs,
args: splitArgs,
env: nonEmptyEnvVars,
},
},
Expand Down Expand Up @@ -135,7 +137,7 @@ export const assembleInferenceService = (
path,
},
}),
args: nonEmptyArgs,
args: splitArgs,
env: nonEmptyEnvVars,
},
},
Expand Down
21 changes: 21 additions & 0 deletions frontend/src/api/k8s/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,24 @@
name: 'shm',
emptyDir: { medium: 'Memory', ...(sizeLimit && { sizeLimit }) },
});

export const parseCommandLine = (input: string): string[] => {
const args: string[] = [];
const regex = /(?:[^\s"']+|"[^"]*"|'[^']*')+/g;
let match: RegExpExecArray | null;

while ((match = regex.exec(input)) !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking closer at this - what iterates this loop? does regex.exec(input) with the same input multiple times step to the next match? A quick glance at docs says yes - interesting, I haven't used it that way before.

I wonder if it would be simpler to use regex.match instead of regex.exec to just get back an array of all the matches, and then do a forEach or reduce on those? I'm nitpicking though, it's probably fine as-is.

Copy link
Member Author

@Xaenalt Xaenalt Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine with changing solution, that's just the one that it gave me

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works it works, but I may tinker with an alternative when I have more time to review. Did you still plan on adding / have time to add unit tests here?

let arg: string = match[0];

// Remove surrounding quotes if any
if (arg.startsWith('"') && arg.endsWith('"')) {
arg = arg.slice(1, -1).replace(/\\"/g, '"'); // Unescape double quotes

Check warning on line 88 in frontend/src/api/k8s/utils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/api/k8s/utils.ts#L88 

Added line #L88 was not covered by tests
} else if (arg.startsWith("'") && arg.endsWith("'")) {
arg = arg.slice(1, -1); // Remove single quotes

Check warning on line 90 in frontend/src/api/k8s/utils.ts

View check run for this annotation

Codecov / codecov/patch

frontend/src/api/k8s/utils.ts#L90 

Added line #L90 was not covered by tests
}

args.push(arg);
}

return args;
};
Loading