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

[serving] Adds workflow model loading for SageMaker #661

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

frankfliu
Copy link
Contributor

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@frankfliu frankfliu requested review from zachgk and a team as code owners April 25, 2023 04:03
return parse(uri, new InputStreamReader(input, StandardCharsets.UTF_8));
public static WorkflowDefinition parse(String name, URI uri) throws IOException {
String type = FilenameUtils.getFileExtension(Objects.requireNonNull(uri.toString()));
try (InputStream is = uri.toURL().openStream();

Check failure

Code scanning / CodeQL

Server-side request forgery

Potential server-side request forgery due to a [user-provided value](1).
1. Removed global preferences for workflow definition
2. Updated README
for (Entry<String, ModelInfo<Input, Output>> emd : models.entrySet()) {
ModelInfo<Input, Output> md = emd.getValue();
md.setId(emd.getKey());
md.setQueueSize(firstValid(md.getQueueSize(), queueSize, wlmc.getJobQueueSize()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we are removing these parameters from the workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. global settings is just the default value, per model setting is override the settings in serving.properties. this is a bit confusing
  2. In most cases (especially on GPU), each model will have different per model settings
  3. We only covered a subset of per model settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still lean towards keeping it.

  1. It's a basic system of more specific rules override less specific. Global < workflow settings < model serving.properties < workflow model settings
  2. The workflow has both settings for all models in the workflow and for particular models
  3. Do you mean loadOnDevices? It might be worth adding it, then.

I think having this is still useful. When importing models defined externally (like in model hub), this still allows defining some model properties without having to do something difficult to introduce the serving.properties.

But, I can see the argument towards removing it and I don't think it's too important. I'll just approve it and leave it up to you

@frankfliu frankfliu merged commit fb68c5b into deepjavalibrary:master Apr 25, 2023
@frankfliu frankfliu deleted the wf branch April 25, 2023 22:08
frankfliu added a commit that referenced this pull request May 18, 2023
* [serving] Adds workflow model loading for SageMaker

1. Removed global preferences for workflow definition
2. Updated README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants