azure-rest-api-specs
azure-rest-api-specs copied to clipboard
Azure
Data Plane AzureML/Azure-AI - Prompt Asset
Data Plane API - Pull Request
Adding a Prompt subclass of AssetVersion for use with prompt engineering tasks
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
- Link to API Spec engagement record issue:
Is this review for (select one):
- [ ] a private preview
- [x] a public preview
- [ ] GA release
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.
- Design Document:
- Previous API Spec Doc:
- Updated paths:
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.
Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the Swagger-Suppression-Process to get approval.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
- 💬 Teams Channel
Click here for links to tools, specs, guidelines & other good stuff
Tooling
- Open API validation tools were run on this PR. Go here to see how to fix errors
- Spectral Linting
- Open API Hub
Guidelines & Specifications
Helpful Links
Checks stuck in `queued` state?
If the PR CI checks appear to be stuck in `queued` state, please add a comment with contents `/azp run`. This should result in a new comment denoting a `PR validation pipeline` has started and the checks should be updated after few minutes.
Next Steps to Merge
✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.![openapi-pipeline-app[bot] avatar](https://avatars.githubusercontent.com/in/65541?v=4 "Published by a pub.dev verified publisher"){kind=small}
Swagger Validation Report
️❌BreakingChange: 7 Errors, 0 Warnings failed [Detail]
| Compared specs (v0.10.9) | new version | base version |
|---|---|---|
| azure-ai-assets.json | 2024-04-01-preview(ca8829c) | 2024-04-01-preview(main) |
️⚠️Breaking Change(Cross-Version): 8 Warnings warning [Detail]
| Compared specs (v0.10.9) | new version | base version |
|---|---|---|
| azure-ai-assets.json | 2024-05-01-preview(ca8829c) | 2024-04-01-preview(main) |
The following breaking changes are detected by comparison with the latest preview version:
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 3 Warnings warning [Detail]
| Compared specs (v2.2.2) | new version | base version |
|---|---|---|
| package-2024-04-01-preview | package-2024-04-01-preview(ca8829c) | package-2024-04-01-preview(main) |
| package-2024-05-01-preview | package-2024-05-01-preview(ca8829c) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
| Rule | Message | Related RPC [For API reviewers] |
|---|---|---|
| :warning: ParameterDefaultNotAllowed | A required parameter should not specify a default value. Location: Microsoft.MachineLearningServices/preview/2024-05-01-preview/azure-ai-assets.json#L571 |
|
| :warning: PathParameterSchema | Path parameter should specify a maximum length (maxLength) and characters allowed (pattern). Location: Microsoft.MachineLearningServices/preview/2024-05-01-preview/azure-ai-assets.json#L644 |
|
| :warning: PathParameterSchema | Path parameter should specify a maximum length (maxLength) and characters allowed (pattern). Location: Microsoft.MachineLearningServices/preview/2024-05-01-preview/azure-ai-assets.json#L694 |
The following errors/warnings exist before current PR submission:
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
️️✔️ApiDocPreview succeeded [Detail] [Expand]
Please click here to preview with your @microsoft account.
️❌ azure-sdk-for-python failed [Detail]
❌Code Generator Failed in generating from cb80c5613905e9e83097749c9c14c08cc17e77c9. SDK Automation 14.0.0command sh scripts/automation_init.sh ../azure-sdk-for-python_tmp/initInput.json ../azure-sdk-for-python_tmp/initOutput.json cmderr [automation_init.sh] W: Target Packages (main/binary-amd64/Packages) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target Packages (main/binary-all/Packages) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target Translations (main/i18n/Translation-en) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target CNF (main/cnf/Commands-amd64) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target CNF (main/cnf/Commands-all) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target Packages (main/binary-amd64/Packages) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target Packages (main/binary-all/Packages) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target Translations (main/i18n/Translation-en) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target CNF (main/cnf/Commands-amd64) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] W: Target CNF (main/cnf/Commands-all) is configured multiple times in /etc/apt/sources.list.d/azure-cli.list:1 and /etc/apt/sources.list.d/azure-cli.sources:1 cmderr [automation_init.sh] WARNING: Skipping azure-nspkg as it is not installed. cmderr [automation_init.sh] notice cmderr [automation_init.sh] npm notice New minor version of npm available! 10.5.0 -> 10.8.0 cmderr [automation_init.sh] npm notice Changelog: <https://github.com/npm/cli/releases/tag/v10.8.0> cmderr [automation_init.sh] npm notice Run `npm install -g [email protected]` to update! cmderr [automation_init.sh] npm notice warn specification/machinelearningservices/data-plane/readme.md skipped due to azure-sdk-for-python not found in swagger-to-sdk command sh scripts/automation_generate.sh ../azure-sdk-for-python_tmp/generateInput.json ../azure-sdk-for-python_tmp/generateOutput.json cmdout [automation_generate.sh] [Autorest]/mnt/vss/_work/1/s/azure-sdk-for-python_tmp/venv-sdk/auto_temp.json does not exist!!!Error happened during codegen error Script return with result [failed] code [1] signal [null] cwd [azure-sdk-for-python]: sh scripts/automation_generate.sh warn Skip package processing as generation is failed
Generated ApiView
| Language | Package Name | ApiView Link |
|---|---|---|
| Swagger | Microsoft.MachineLearningServices | https://apiview.dev/Assemblies/Review/af9cc69e6ae94f27ab2ca41fddf6ba46?revisionId=6e63c534df014618974a76e7489fbdf5 |
| TypeSpec | AzureAI.Assets | https://apiview.dev/Assemblies/Review/2e4c2542e85c42e4805177b00db683de?revisionId=c6969852fa7c47aa8576811a1bf59190 |
could you also remove v2.0 from the generic asset url? Here: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/machinelearningservices/AzureAI.Assets/main.tsp
I left a bunch of comments. Many of these are on the existing "Index" type and I apologize that I missed these in the earlier review. Most of these also apply to the new Prompt type -- I just didn't want to try to flag every instance. So please consider all the comments to apply across the whole API even if they are only on specific lines.
Also, I am still uncomfortable with the versioning approach being used here and I think it warrants a deeper review and discussion. I'm not questioning the need to support versioning, but just the specific approach being used, where there are indexes and versions of indexes that are both actually the same thing, but from the URL structure look like different resources.
It isn't necessary to fix all of these in this API version, but I think most should be addressed before the API goes GA, so fixing them as soon as you can is probably best for your users.
We can re-open the discussion before GA. However, this PR is truly meant to be an iteration off of the last PR. It's unfortunate that these reservations were not discussed more thoroughly during the Index API PR, where I had introduced this model and was hoping to address all concerns at that time in order to shelter this PR from those discussions.
As mentioned before, this modeling for "assets" was already GA'd and published in ARM as an accepted REST API model for versioning resources: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/stable/2024-04-01/mfe.json - for example resource ..data/{name}/versions Although there is always room for improvement, we already have experience working with customers actively on this versioning model and have a proof point of it working for our purposes. The reason we strongly prefer to adhere to this model is for two reasons
- We want to interop with our existing GA'd ARM asset set as much as possible
- It's an ideal model for creating lineage because we can keep "copies" of each version. This not only allows us to reference them uniquely, but also make select updates to them individually. In this sense we are treating them as separate resources.
If you still require a deeper review, then can we please address in our next API version? At least going into that API we will be aware of the pre-existing review requirement. Thank you for your understanding in this matter.
I really think you need to fix this PR to not break or even change released API versions.
It didn't at the time I asked you to review - the only new change was removing the 2.0 from the route (which was an approved breaking change in another PR that I simply added here). After addressing some of the comments from the first round, I'm seeing tests failures, so I'll get things back green.
@mikekistler regarding then nextVersion logic. I've updated the descriptions to lay it out, but I'll make sure to explain here.
Index uses the Data team's backend for service calls. This is not something our team controls and this pattern has been GA for years. Their nextVersion logic is configured as follows
- Any string is allowed to be set as a version (e.g. your example of "one" or "my-favorite")
- At creation time, Data service screens all the version strings to see if any are integer representations
- If so and if that value is greater than the last seen integer value, nextVersion becomes that value + 1. If there has never been any integer value supplied, the default nextVersion is 1.
In sum, nextVersion = max(lastIntegerVersion + 1, 1)