Skip to content

Conversation

@sebastienros
Copy link
Member

@sebastienros sebastienros commented Oct 16, 2024

Description

Fixes #6298

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@sebastienros sebastienros requested a review from joperezr October 16, 2024 17:17
@davidfowl
Copy link
Member

We want to mark the API as experimental. When you call AddPythonApp it should have the attribute on it. Like this https://github.com/eerhardt/aspire/blob/5603e928797a629d972c0084e2d6414169d1295c/src/Aspire.Hosting.Azure.CognitiveServices/AzureOpenAIExtensions.cs#L39

@davidfowl
Copy link
Member

Although, maybe pre-release is enough to signal what we want.

@sebastienros
Copy link
Member Author

sebastienros commented Oct 16, 2024

I could not find this attribute used anywhere though.

Update: not saying we shouldn't use it, but all the experimental stuff we have is based on the package version only from what I see. Waiting to learn more ;)

<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<!-- In preview until the public API is validated. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this in the csproj?

Copy link
Member

Choose a reason for hiding this comment

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

See conversation here: #6195 (comment)

cc @joperezr

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on this PR this is the way to do it now
https://github.com/dotnet/aspire/pull/6195/files

Copy link
Member

Choose a reason for hiding this comment

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

That is no longer the case, it was only for RC1 as we had things going RC1 and some other things staying in preview, but now everything should fall back to be preview or stable.

@davidfowl davidfowl merged commit 3671947 into main Oct 16, 2024
9 checks passed
@davidfowl davidfowl deleted the sebros/experimentalpython branch October 16, 2024 18:27
@eerhardt
Copy link
Member

My thoughts:

  1. Making a nuget package not stable (i.e. have a -preview suffix in the version) should be enough signal to people that the API can change before it becomes stable. Users don't have to install a "pre release" package and then ALSO suppress a warning about using experimental APIs inside that prerelease package.

  2. Aspire.Hosting.Python already shipped stable in 8.1 and 8.2. If we don't ship a "stable" 9.0.0 package, people will continue using the 8.2.1 Aspire.Hosting.Python package with Aspire 9.0. This maybe won't cause problems, but it will be confusing.

@davidfowl
Copy link
Member

@DamianEdwards thoughts?

@eerhardt
Copy link
Member

Another problem is that shipped https://www.nuget.org/packages/Aspire.Hosting.Python/9.0.0-rc.1.24511.1. Which means when we ship 9.0.0 GA, the Python package produced with that build will still have -preview and will be LOWER than the rc1 package. So even if people move to the 9.0.* package, they will get the rc1 package, which is old.

Maybe we need to ship this as stable and mark the APIs as [Experimental]....

@DamianEdwards
Copy link
Member

Maybe we need to ship this as stable and mark the APIs as [Experimental]....

I thought that was the proposal?

@eerhardt
Copy link
Member

Maybe we need to ship this as stable and mark the APIs as [Experimental]....

I thought that was the proposal?

What ended up getting changed was the package is no longer building as "stable", so when it gets built it will have a -preview version.

@sebastienros
Copy link
Member Author

I will do the change

@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mark AddPythonApp API as experimental

6 participants