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

update generated crates to only use the latest spec #726

Closed
wants to merge 6 commits into from

Conversation

bmc-msft
Copy link
Contributor

@bmc-msft bmc-msft commented Apr 19, 2022

Based on discussions during the SDK meeting today, @heaths mentioned that the gen2 SDKs are only targeting the latest REST API specification for each service in main.

By only targeting the latest spec, we significantly reduce the burden on our build infrastructure.

Brian Caswell added 5 commits April 19, 2022 15:15
Based on discussions in the SDK meeting, this changes the code generator to only generate the latest tag.
Copy link
Contributor

@ctaggart ctaggart left a comment

Choose a reason for hiding this comment

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

We should discuss further. It is important to be able to support multiple versions.

@heaths
Copy link
Member

heaths commented Apr 20, 2022

We should discuss further. It is important to be able to support multiple versions.

All our SDKs do support different versions but take advantage of our strict breaking change policy as I've outlined here. We need to be able to specify an api-version and do support that already, but generate the clients based on the latest swaggers - beta SDKs (name is important) for preview swaggers, and GA SDKs for stable swaggers. Only 1 or 2 of our SDKs actually ship multiple api-versions.

@cataggar
Copy link
Member

I'd like to maintain a separation between generated crates (Rust packages) and more ergonomic hand-crafted SDK crates. It makes sense for the SDK crates to be based on the latest. The generated crates should continue supporting all tags in the azure-rest-api-specs.

@cataggar cataggar closed this Apr 21, 2022
@heaths
Copy link
Member

heaths commented Apr 22, 2022

@cataggar thing is, all crates should have generated code. This is the direction the Azure SDK team is going, and all but python (I believe) always use latest. With strict breaking change policies, why ship larger crates (increased build times, RLS scanning, etc.) than necessary for no value added?

Can you clarify any technical reasons shipping multiple versions is necessary?

@cataggar
Copy link
Member

The crate size is minimal. https://crates.io/crates/azure_mgmt_compute is 3.36 MB. There is not increased build times for users. The end user will usually pick one tag via Rust features and the build time will be quick. Many users will want to pin to a specific tag. Rolling out services to clouds and regions in clouds takes time. The APIs are sometimes published before they are available everywhere.

As an API author, I want to be able to use clients for every azure_mgmt_vmware tag.

For https://github.com/Azure/azure-sdk-for-rust/tree/main/services/mgmt/resources, multiple tags must be used to cover the different specs. There are other examples like this.

The azure-rest-api-specs repo should prune versions when they are no longer supported. The generated crates should make available from Rust, what is there.

@heaths
Copy link
Member

heaths commented Apr 25, 2022

@cataggar how does pinning to a tag or rolling out service late factor in? If you look at the examples and guidelines I posted, we still support older versions including pinning to them. For the vast majority of cases, v2 of an API will be backward compatible with v1 and so on. This is because of our breaking changes policies. So a client generated from v2 can still pin and use v1. All our SDKs support this and most of them even test it against live resources nightly. Internally, the pinned api version is passed as the api-version (or similar) for the generated REST client. There's no loss in functionality.

What benefit is there to shipping numerous source sets for what is essentially the same API when we already support pinning? I get that rust effectively statically links. The main concern was how this affects rust-analyzer since it now has to parse all source.

@cataggar
Copy link
Member

cataggar commented Apr 26, 2022

A tag is not the same as an api-version. For example, in https://crates.io/crates/azure_mgmt_compute, the most recent tag is:

package-2021-12-01 has 276 operations from 5 API versions: 2021-03-01, 2021-07-01, 2021-10-01, 2021-11-01, 2021-12-01. Use crate feature package-2021-12-01 to enable. The operations will be in the package_2021_12_01 module.

Pinning to a single api-version would not work for the other operations that do not support that api-version.

For https://crates.io/crates/azure_mgmt_resources, tags are used for different sets of operations. The most recent one does not cover everything.

The backward compatible guarantees and adherence to the specification is not always reality (internal link https://aka.ms/swaggerquality). In many situations, I'd prefer to pin to stable tags with stable specs and api-versions). rust-analyzer should be able to efficiently handle since we are using Rust features.

@bmc-msft
Copy link
Contributor Author

bmc-msft commented May 2, 2022

The source sizes are not minimal.

Without this PR: svc is 51MB and mgmt is 546MB. (76 crates above 1MB, 12 above 10MB, with the largest being mgmt/network at 105MB.)

With this PR: svc is 9.9MB and mgmt is 64MB. (13 crates above 1MB, with the largest mgmt/web at 3.9).

@bmc-msft
Copy link
Contributor Author

bmc-msft commented May 2, 2022

To compare against other SDKs,

It appears that python & go include generated code for every tag.
It appears that dotnet, java, and javascript (node) include generated code for only one tag.

It would be good if these were consistent, and we followed that consistency.

@heaths
Copy link
Member

heaths commented May 2, 2022

@JeffreyRichter @johanste could you clarify why Python and Go contain generated code for each supported API version? In both customers call into a higher-level abstraction anyway e.g. azkeys.

@cataggar
Copy link
Member

cataggar commented May 2, 2022

The term used in AutoRest is "Multi-API". It is not just Python & Go. There are several use cases for Multi API.
https://github.com/Azure/azure-rest-api-specs/search?q=multi-api

@heaths
Copy link
Member

heaths commented May 2, 2022

Most I saw in a couple pages were go, or not used at all with our autorest-based code generation. There's a lot of old code / config in that repo. My point is that most of the Azure SDK languages have not had a need to support multiple APIs in a single package (source or binary) and that's been working just fine.

@johanste
Copy link
Member

johanste commented May 2, 2022

@JeffreyRichter @johanste could you clarify why Python and Go contain generated code for each supported API version? In both customers call into a higher-level abstraction anyway e.g. azkeys.

Two main reasons:

  1. the latest API version may not be available on the target cloud (national clouds, azure stack etc.) - and thus the API version used in the latest library package may not be supported.
  2. the behavior of the API may have subtly changed between API version, and you want to lock to a specific behavior. This is why we recommend that developers of reusable components/libraries specify the exact API version they expect to avoid getting dragged into untested behavior when an application updates a reference to a later version.

Regarding things having been working fine, that was mostly true for application authors targeting public azure. For library authors/users targeting other clouds, slightly less so.

@bmc-msft bmc-msft deleted the only-generate-latest-version branch June 13, 2022 20:17
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.

None yet

5 participants