-
Notifications
You must be signed in to change notification settings - Fork 240
[sdk_v2] migrate c# and js sdk_v2 implementation and build pipeline to public GH repo #389
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
Conversation
…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ithub.com/microsoft/Foundry-Local into prathikrao/migrate-cs-sdk-v2-to-public-gh
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "TestModelCacheDirName": "test-data-shared" | |||
| } | |||
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.
Should this have a different default for the public repo?
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.
Does developer need to manually set this before running the tests? Can we add a default dir that we think is reasonable for developers to not need to change instead?
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.
Default could be ${home}\.foundry\cache\models? That's the default model cache for foundry local
| dotnet-version: '9.0.x' | ||
| source-url: https://pkgs.dev.azure.com/microsoft/windows.ai.toolkit/_packaging/Neutron/nuget/v3/index.json | ||
| env: | ||
| NUGET_AUTH_TOKEN: ${{ secrets.AZURE_DEVOPS_PAT }} |
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.
Unfortunately, secrets do not work very well for external contributions. External contributions come in mostly from forks of the repository. The forked repository does not inherit the secrets of the parent repository. And as a result, PRs created from forks will fail to run.
Let's not block this PR. But we need to think of another way to address this.
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.
sure, let me think on it
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.
Would it be possible to share testdata between cs and js?
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.
yes, can do separately
Changes made for public sdk_v2
sdk_v2\cs\test\FoundryLocal.Tests\FoundryLocalManagerTest.cs
removed comment about implementing test-data-shared feature since we already did, no longer need to intercept and mock get_model_list command
sdk_v2\cs\test\FoundryLocal.Tests\LOCAL_MODEL_TESTING.md
changed docs to not expose test-data-shared instructions and just asks user to set TestModelCacheDirName to their model cache
sdk_v2\cs\test\FoundryLocal.Tests\Utils.cs
removed CachePath and Web.ExternalUri config vars
.github/workflows/sdk_v2
modified pipeline to only include debug build for c# sdk as a proof-of-concept
AZURE_DEVOPS_PAT stored and accessed as a github actions secret (needs to be updated weekly at the moment, can switch to a stable/auto-regenerable access token later)
updated readme to not include test-data-shared details and instructs users to set modelCacheDir if they want to customize the tests
Removed signing as that requires private auth creds and only needs to be done for official releases