-
Notifications
You must be signed in to change notification settings - Fork 267
Finetuning Extension code #6459
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
base: main
Are you sure you want to change the base?
Conversation
* adding draft for fine tuning * adding service target * fixing root command format * cleaning unused commands and adding operation/sub-operation commands * adding more details to command * fixes for relative path * adding registry entry * adding git download * adding 0.0.3 * fixing bug in deploy * adding printing format * re-structuring code * adding 0.0.5 * 0.0.6 * adding low level design details * fixing ext for build * adding restructured code * reverting any registry changes
…ronment and validation utils (#6430) * initial changes * Add retry util for exponential backoff with jitter strategy * Clean-up * Standardize CLI text to lowercase conventions, and remove redundant error messages * Address comments * Add common util for fetch environment variables * Cosmetic changes * Cosmetic changes 2 * Fix runtime error * Fix error message format
* Create command for ft cli * handling null pointer for state * removing changes from old converter and yaml logic * pr review changes + scheme implementation for job params * a few formatting changes * removing ofAuto value for hyperparameters * more formatting changes * adding command line parameters + formatting * minor merge fixes
* Adding pipeline scripts * Adding pipeline scripts * fixing version path * fixing build command
* adding init command * update version
* Adding pipeline scripts * Adding pipeline scripts * fixing version path * fixing build command * Djurek/test-network-isolation (#6455) * Update 1es-redirect.yml * Trivial change to test pipeline * Show GOPROXY * Use golang internalModuleProxy --------- Co-authored-by: Daniel Jurek <djurek@microsoft.com>
* adding init command * update version * cleaning some code
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.
Pull request overview
This PR introduces a comprehensive Azure AI Fine Tuning extension for the azd CLI, establishing a well-architected foundation for fine-tuning workflows with multi-vendor support. The implementation follows a three-layer architecture (Domain Models, Service Layer, Provider Layer) with clear separation of concerns.
Key Changes:
- Added complete extension architecture with domain models, services, and provider interfaces
- Implemented cross-platform build scripts and CI/CD pipeline configuration
- Included example YAML configurations for supervised, DPO, and reinforcement learning fine-tuning
- Provided comprehensive architecture documentation and implementation guides
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
eng/pipelines/templates/steps/setup-go.yml |
Added GOPROXY environment variable logging for debugging |
eng/pipelines/templates/stages/1es-redirect.yml |
Enhanced pipeline configuration with golang proxy, network isolation, and SDL controls |
eng/pipelines/release-ext-azure-ai-finetune.yml |
New CI/CD pipeline definition for the extension |
cli/azd/extensions/azure.ai.finetune/version.txt |
Version file for the extension (0.0.6-preview) |
cli/azd/extensions/azure.ai.finetune/pkg/models/*.go |
Domain models for fine-tuning jobs, deployments, errors, and requests |
cli/azd/extensions/azure.ai.finetune/internal/utils/*.go |
Utility functions for time formatting, status display, retry logic, parsing, and environment handling |
cli/azd/extensions/azure.ai.finetune/internal/services/*.go |
Service layer interfaces and implementations for business logic |
cli/azd/extensions/azure.ai.finetune/internal/providers/*.go |
Provider layer with OpenAI and Azure implementations |
cli/azd/extensions/azure.ai.finetune/internal/cmd/*.go |
CLI commands for init, operations, validation, and version |
cli/azd/extensions/azure.ai.finetune/main.go |
Extension entry point |
cli/azd/extensions/azure.ai.finetune/go.mod |
Go module dependencies |
cli/azd/extensions/azure.ai.finetune/extension.yaml |
Extension metadata and configuration |
cli/azd/extensions/azure.ai.finetune/examples/*.yaml |
Example configurations for different fine-tuning methods |
cli/azd/extensions/azure.ai.finetune/design/*.md |
Architecture documentation and implementation summary |
cli/azd/extensions/azure.ai.finetune/build.* |
Cross-platform build scripts |
cli/azd/extensions/azure.ai.finetune/ci-build.ps1 |
CI-specific build script with advanced flags |
cli/azd/extensions/azure.ai.finetune/changelog.md |
Release history |
cli/azd/extensions/azure.ai.finetune/README.md |
Extension documentation |
Note: Due to the large scope of this PR with 30+ new files, I've conducted a comprehensive architectural review. The code demonstrates good separation of concerns and follows the repository's coding standards. All files include proper copyright headers, and the architecture is well-documented.
| # `azd` Demo Extension | ||
|
|
||
| An AZD Demo extension |
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.
nit: should probably be updated
cli/azd/extensions/azure.ai.finetune/internal/services/finetune_service.go
Outdated
Show resolved
Hide resolved
cli/azd/extensions/azure.ai.finetune/internal/services/finetune_service.go
Outdated
Show resolved
Hide 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.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 4 comments.
cli/azd/extensions/azure.ai.finetune/internal/services/finetune_service.go
Outdated
Show resolved
Hide resolved
…n.yaml Co-authored-by: JeffreyCA <jeffreychen@microsoft.com>
Co-authored-by: JeffreyCA <jeffreychen@microsoft.com>
| // getComposedResourcesResponse, err := azdClient.Compose().ListResources(ctx, &azdext.EmptyRequest{}) | ||
| // if err != nil { | ||
| // return fmt.Errorf("failed to get composed resources: %w", err) | ||
| // } |
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.
remove?
| func extractProjectDetails(projectResourceId string) (*FoundryProject, error) { | ||
| /// Define the regex pattern for the project resource ID | ||
| pattern := `^/subscriptions/([^/]+)/resourceGroups/([^/]+)/providers/Microsoft\.CognitiveServices/accounts/([^/]+)/projects/([^/]+)$` | ||
|
|
||
| regex, err := regexp.Compile(pattern) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to compile regex pattern: %w", err) | ||
| } | ||
|
|
||
| matches := regex.FindStringSubmatch(projectResourceId) | ||
| if matches == nil || len(matches) != 5 { | ||
| return nil, fmt.Errorf("the given Microsoft Foundry project ID does not match expected format: /subscriptions/[SUBSCRIPTION_ID]/resourceGroups/[RESOURCE_GROUP]/providers/Microsoft.CognitiveServices/accounts/[ACCOUNT_NAME]/projects/[PROJECT_NAME]") | ||
| } | ||
|
|
||
| // Extract the components | ||
| return &FoundryProject{ | ||
| SubscriptionId: matches[1], | ||
| ResourceGroupName: matches[2], | ||
| AiAccountName: matches[3], | ||
| AiProjectName: matches[4], | ||
| }, nil | ||
| } |
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.
Consider using arm.ParseResourceID from "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource
| func getExistingEnvironment(ctx context.Context, flags *initFlags, azdClient *azdext.AzdClient) *azdext.Environment { | ||
| var env *azdext.Environment | ||
| if flags.env == "" { | ||
| if envResponse, err := azdClient.Environment().GetCurrent(ctx, &azdext.EmptyRequest{}); err == nil { | ||
| env = envResponse.Environment | ||
| } | ||
| } else { | ||
| if envResponse, err := azdClient.Environment().Get(ctx, &azdext.GetEnvironmentRequest{ | ||
| Name: flags.env, | ||
| }); err == nil { | ||
| env = envResponse.Environment | ||
| } | ||
| } | ||
|
|
||
| return env | ||
| } |
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.
consider replacing flags argument with name *string - since you are only using the flags.env.
This makes the method signature better to understand.
Right now from looking at it, it is surprising to see that you need initFlags to get an env.
| if envResponse, err := azdClient.Environment().GetCurrent(ctx, &azdext.EmptyRequest{}); err == nil { | ||
| env = envResponse.Environment | ||
| } | ||
| } else { | ||
| if envResponse, err := azdClient.Environment().Get(ctx, &azdext.GetEnvironmentRequest{ | ||
| Name: flags.env, | ||
| }); err == nil { | ||
| env = envResponse.Environment |
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.
Do not hide the error. Update the logic here to return the error.
Otherwise there could be an error and you would still be returning env as nil w/o telling why
| existingEnv = getExistingEnvironment(ctx, flags, azdClient) | ||
| if existingEnv == nil { | ||
| return nil, fmt.Errorf("azd environment not found, please create an environment (azd env new) and try again") | ||
| } |
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.
The error should not hide if there was a different error. It could be confusing for a user to know there is an enviroment but some other error is making this to be returned
wbreza
left a comment
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.
Looks great overall - just a few minor comments and questions before approving.
| # Use this for preference-based fine-tuning with preferred vs non-preferred outputs | ||
|
|
||
| model: gpt-4o-mini | ||
| training_file: "local:./dpo_training_data.jsonl" |
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.
I'm assuming local refers to local file system - does this support other providers?
| func extractProjectDetails(projectResourceId string) (*FoundryProject, error) { | ||
| /// Define the regex pattern for the project resource ID | ||
| pattern := `^/subscriptions/([^/]+)/resourceGroups/([^/]+)/providers/Microsoft\.CognitiveServices/accounts/([^/]+)/projects/([^/]+)$` | ||
|
|
||
| regex, err := regexp.Compile(pattern) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to compile regex pattern: %w", err) | ||
| } | ||
|
|
||
| matches := regex.FindStringSubmatch(projectResourceId) | ||
| if matches == nil || len(matches) != 5 { | ||
| return nil, fmt.Errorf("the given Microsoft Foundry project ID does not match expected format: /subscriptions/[SUBSCRIPTION_ID]/resourceGroups/[RESOURCE_GROUP]/providers/Microsoft.CognitiveServices/accounts/[ACCOUNT_NAME]/projects/[PROJECT_NAME]") | ||
| } | ||
|
|
||
| // Extract the components | ||
| return &FoundryProject{ | ||
| SubscriptionId: matches[1], | ||
| ResourceGroupName: matches[2], | ||
| AiAccountName: matches[3], | ||
| AiProjectName: matches[4], | ||
| }, nil | ||
| } |
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.
You should be able to the the Azure Go SDK ARM resource to parse a resource id instead of rolling your own implementation.
|
|
||
| // RetryOperation executes the given operation with retry logic | ||
| // The operation should return an error if it should be retried | ||
| func RetryOperation(ctx context.Context, config *RetryConfig, operation func() error) error { |
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.
There is a very popular OSS retry package that we use in azd core that I would recommend instead of rolling your own implementation here.
| usage: azd ai finetuning init | ||
| - name: deploy | ||
| description: Deploy AI fine-tuning job to Azure. | ||
| usage: azd ai finetuning deploy |
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 be honest I am a bit concerned about reusing the same deploy verb that we have in azd core. Lets keep it for now and see how this looks as we look for full release.
| - name: Use1ESOfficial | ||
| type: boolean | ||
| default: true | ||
| - name: GenerateBaselines |
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.
Any specific reasons we need to update this? Adding @danieljurek for review.
| Write-Host "##vso[task.prependpath]$goBin" | ||
| displayName: Add Go bin to PATH | ||
| - pwsh: | |
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.
Can you elaborate more on this change?
This pull request introduces a new Azure AI Fine Tuning extension for the
azdCLI, providing a well-structured foundation for fine-tuning workflows, multi-vendor support, and robust build tooling. The changes include a comprehensive architecture summary, build scripts for multiple platforms, example configuration files for various fine-tuning methods, and initial documentation and metadata.Extension Architecture and Design:
IMPLEMENTATION_SUMMARY.md, outlining the folder structure, interfaces, domain models, provider and service layers, and next steps for development. This document clarifies separation of concerns, import rules, and testability for future contributors.Build and CI Tooling:
build.sh(Bash) andbuild.ps1(PowerShell) for building the extension binaries for multiple OS/architecture targets, including logic for versioning and commit tracking. Added a CI-specific build scriptci-build.ps1with advanced build flags and code coverage options. [1] [2] [3]Configuration and Example Files:
Documentation and Metadata:
README.mddescribing the extension,changelog.mdfor release history, andextension.yamldefining extension metadata, capabilities, and usage examples for theazdCLI. [1] [2] [3]These changes lay the groundwork for a scalable, multi-vendor fine-tuning extension, enabling rapid development and future enhancements.