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

Support multiple deployments per cognitive service #3448

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Apr 6, 2024

Fixes #3409

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Apr 6, 2024
@sebastienros
Copy link
Member Author

sebastienros commented Apr 6, 2024

@JoshLove-msft is that something that could be made public, or in the opposite not used?

By that I mean this PR uses private reflection to access Resource.AddDependency().

@JoshLove-msft
Copy link
Contributor

@JoshLove-msft is that something that could be made public, or in the opposite not used?

By that I mean this PR uses private reflection to access Resource.AddDependency().

Yeah this can be made public.

@JoshLove-msft
Copy link
Contributor

@JoshLove-msft is that something that could be made public, or in the opposite not used?
By that I mean this PR uses private reflection to access Resource.AddDependency().

Yeah this can be made public.

Azure/azure-sdk-for-net#43257

@aaronpowell
Copy link
Contributor

Should the AOAI demo in the playground folder be expanded to have multiple model deployments so there's an implementation

Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

This is good. Just need the updated version of CDK so we don't need to use reflection.

@danmoseley danmoseley added area-components Issues pertaining to Aspire Component packages and removed area-codeflow for labeling automated codeflow. intentionally a different color! labels Apr 8, 2024
@danmoseley
Copy link
Member

@joperezr I wonder whether it would help to retrain the labeler bot

@danmoseley danmoseley added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed area-components Issues pertaining to Aspire Component packages labels Apr 8, 2024
@danmoseley
Copy link
Member

although these things on the hosting side that are specific to a component, I'm not sure what to label

// to ensure they are not created in parallel. This is equivalent to @batchSize(1)
// which can't be defined with the CDK
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to @batchsize(1)
// which can't be defined with the CDK

When will this be available? That seems like the better long-term fix.

Copy link
Member

@mitchdenny mitchdenny Apr 9, 2024

Choose a reason for hiding this comment

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

We are waiting for this to merge and a build to be produced:

https://github.com/Azure/azure-sdk-for-net/pull/43285/files

@JoshLove-msft and @tg-msft can talk to timelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will be shipping today

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, reacting ASAP

Copy link
Member Author

Choose a reason for hiding this comment

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

When will this be available? That seems like the better long-term fix.

I think the question was not about the AddDependency being made public, but the support for @batchSize(1).

@sebastienros
Copy link
Member Author

sebastienros commented Apr 10, 2024

Updated with public method and tried it with one and two deployments successfully.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@sebastienros sebastienros merged commit a618042 into main Apr 10, 2024
8 checks passed
@sebastienros sebastienros deleted the sebros/serialdeployments branch April 10, 2024 04:53
@eerhardt
Copy link
Member

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8885059518

Copy link
Contributor

@eerhardt backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Support multiple deployments per cognitive service
Applying: Use local method
Applying: Use public method
Using index info to reconstruct a base tree...
M	Directory.Packages.props
Falling back to patching base and 3-way merge...
Auto-merging Directory.Packages.props
CONFLICT (content): Merge conflict in Directory.Packages.props
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Use public method
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@eerhardt an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

eerhardt pushed a commit to eerhardt/aspire that referenced this pull request Apr 29, 2024
RussKie pushed a commit that referenced this pull request Apr 30, 2024
Co-authored-by: Sébastien Ros <sebastienros@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2024
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.

Provisioning with two Azure OpenAI model deployments fails
6 participants