Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update ServerlessEndpoints design #25593
Update ServerlessEndpoints design #25593
Changes from 1 commit
666e7aa
d07667d
d908cda
79103cc
7899fe1
be6f5d3
15e9f19
29752b6
8109593
5addebf
62a75a6
9f18b19
9e7d67c
7a1bfeb
767baac
d447cb8
1754025
bb6c086
93462c1
88ab27c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional properties pattern is an ARM anti-pattern.
Generally, this pattern should only be used when needed and the object properties are dynamic, unknown, or user defined. Ref: https://armwiki.azurewebsites.net/api_contracts/guidelines/openapi.html#oapi032-only-use-additionalproperties-when-the-object-properties-are-dynamic-unknown-or-user-defined.
Can you schematize this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the information dynamic. The list of metrics will be model dependent, some of which may come from 3P ISVs etc. Hence it is modeled as a generic string dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please book an ARM modeling office hours slot and go over the design with the ARM API reviewer to assess if this pattern can be approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have booked an office hour, however we are concerned about the time delay so wondering if we can close on this faster over IM or a team call.
The key thing here is that our service is not in control of all possible keys in this dictionary, because this data comes from customers (specifically, model providers). We are hoping this is a valid use case of additional properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this read only as was discussed in office hours. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this read only property here as discussed earlier. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readonly is already there, it looks like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback here. Can this be schematized? Otherwise it's difficult and risky for clients to consume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same situation here applies. The content here is model dependent. It is intended as a generic contract to the client to populate these required headers when sending inferencing requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To elaborate, the possible "headers" is unbounded based on customer (specifically, the model provider), so there is no way for us to specify all possible values in the schema upfront. I hope this is a valid use case of additional properties.