lazy load rank processor, refresh config and relaod token on expiry #12

Tparuchuri · 2022-02-08T16:16:45Z

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

Please open PR in Draft mode if it is:
- Work in progress or not intended to be merged.
- Encountering multiple pipeline failures and working on fixes.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

Include updated management metadata.
Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.
If the check fails at the Verify Code Generation step, please ensure:
- Do not modify any code in generated folders.
- Do not selectively include/remove generated files in the PR.
- Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
  Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.
Note: We have recently updated the PSH module called by generate.ps1 to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:
```
  `dotnet msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules`
```

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

johnhuang01 · 2022-02-08T17:37:41Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

@@ -65,6 +69,8 @@ public PersonalizerClient(Uri endpoint, TokenCredential credential, Personalizer
            MultiSlotEventsRestClient = new MultiSlotEventsRestClient(_clientDiagnostics, _pipeline, stringEndpoint);
            ServiceConfigurationRestClient = new ServiceConfigurationRestClient(_clientDiagnostics, _pipeline, stringEndpoint);
            PolicyRestClient = new PolicyRestClient(_clientDiagnostics, _pipeline, stringEndpoint);
+            _credentialType = "Token";


As the enum you defined in RlClient, can you define the same enum type here?

removed type and directly validating based on the assigned credential

johnhuang01 · 2022-02-08T17:41:06Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

@@ -548,34 +552,59 @@ public virtual Response ActivateMultiSlot(string eventId, CancellationToken canc
            }
        }

-        /// <summary> Gets the configuration details for the live model to use </summary>
-        internal Configuration GetConfigurationForLiveModel(string authType, string authValue)
+        /// <summary> Gets the rank process initiated with live model to use </summary>


rank processor?

I still don't see the rename.

johnhuang01 · 2022-02-08T17:43:59Z

...e.AI.Personalizer/tests/SessionRecords/RankTests/SingleSlotRankLocalInferenceTestsAsync.json

-              "weather": "rainy"
+    "Entries": [
+        {
+            "RequestUri": "https://singleslotrecordsdktests.cognitiveservices.azure.com/personalizer/v1.1-preview.1/configurations/service",


Do you need to update sdktestmultislot in MultiSlotTest.json?

This is single-slot tests. As we are not calling rank API when islocalInference is true, in the json file rank API response are not required

johnhuang01 · 2022-02-08T17:45:53Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

+        {
+            if ((_credentialType == "Token" &&
+                DateTimeOffset.Compare(tokenExpiry, DateTimeOffset.MinValue) != 0 &&
+                DateTimeOffset.Compare(tokenExpiry, DateTimeOffset.UtcNow) < 0) ||


Should we change to <=? I am wondering about the case when the token expires right now.

I agree with John. We need a buffer. We should be checking if token expires within a minute or two to avoid race conditions and clock skew.

updated it to <=

tyclintw · 2022-02-08T18:41:37Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

@@ -20,11 +20,16 @@ public class PersonalizerClient
        private readonly HttpPipeline _pipeline;
        private readonly bool _isLocalInference;
        private string stringEndpoint;
-        private string apiKey;
+        private AzureKeyCredential _azureKeyCredential;


Please try to be consistent with naming conventions. Either all private variables have an underscore or none of them do. Underscore is not a CSharp standard so I would suggest leaving them out.

removed _ for private variables and did cleanup

tyclintw · 2022-02-08T18:46:15Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

+                CancellationToken cancellationToken = default;
+                AccessToken token = _tokenCredential.GetToken(tokenRequestContext, cancellationToken);
+                config["http.api.key"] = "Bearer " + token.Token;
+                config["HTTP_API_HEADER_KEY_NAME"] = "Authorization";


I must have missed this in the VW PR. Please fix this naming to be consistent.

"http.api.key" and "HTTP_API_HEADER_KEY_NAME" are not in alignment at all. They have different delimiters, case, and structure. Please make them close to the same.

updated the name

tyclintw · 2022-02-08T18:50:22Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

+        private AzureKeyCredential _azureKeyCredential;
+        private TokenCredential _tokenCredential;
+        private int liveModelRefreshTimeInMinutes = 15;
+        private string _credentialType;


Better to define _credentialType as an enum than a string. String operations are error prone, expensive, and are not IDE friendly. With that said, is _credentialType even necessary? Why not the following?

if (_tokenCredential != null) { // do work } else if (_azureKeyCredential != null) { // do work }

tyclintw · 2022-02-08T18:53:15Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

-                //config["http.token.key"] = authValue;
+                var tokenRequestContext = new TokenRequestContext(scopes);
+                CancellationToken cancellationToken = default;
+                AccessToken token = _tokenCredential.GetToken(tokenRequestContext, cancellationToken);


If the implementation requires a cancellationToken it's best to propagate it to the function signature.

tyclintw · 2022-02-08T18:56:10Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

+        {
+            if ((_credentialType == "Token" &&
+                DateTimeOffset.Compare(tokenExpiry, DateTimeOffset.MinValue) != 0 &&
+                DateTimeOffset.Compare(tokenExpiry, DateTimeOffset.UtcNow) < 0) ||


I agree with John. We need a buffer. We should be checking if token expires within a minute or two to avoid race conditions and clock skew.

tyclintw · 2022-02-08T18:57:31Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

        {
-            _personalizerServiceProperties = ServiceConfigurationRestClient.Get();
-            _personalizerPolicy = PolicyRestClient.Get();
+            liveModelLastRefresh = DateTimeOffset.UtcNow;


This time should be set after the model has been updated in case an exception is thrown during update.

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

tyclintw · 2022-02-08T21:51:15Z

sdk/personalizer/Azure.AI.Personalizer/src/Models/PersonalizerClient.cs

+        {
+            if ((tokenCredential != null &&
+                DateTimeOffset.Compare(tokenExpiry, DateTimeOffset.MinValue) != 0 &&
+                DateTimeOffset.Compare(tokenExpiry, DateTimeOffset.UtcNow) <= 0) ||


Token expiry need to be a couple minutes away from now.

until the token expiry the same token is generated. So even with the buffer of 1 min unless the token is expired new token won't be generated with the new expiry. That's what I understood

* new version client generation * fix build error * record test in new version * add changelog * update assembly and metadata * fix code generation discrepency * vnet and platform version tests (#7) * add system data validation (#9) * vnet and platform version tests * validate system data Co-authored-by: Kacie <[email protected]> * backup restore using msi (#10) * new changes * bump version * added test for private endpoint (#11) * playback * Update private endpoint test (#12) * added test for private endpoint * updated test Co-authored-by: Kacie Kang <[email protected]> Co-authored-by: Samir Solanki <[email protected]> Co-authored-by: Alan Feng <[email protected]>

* 12767912: Add isLocalInference in the public SDK * Rename the variable * Thick clent feature (#5) * 12767907: Add a package dependency on Microsoft.RL * Add RankProcessor * Remove unused file * Remove some key * Toggle commented codes so that most tests can pass * Getting the required config details for rankprocessor to enable livemode * Updating the wrong variable name * Cleanup * Revert "12767907: Add a package dependency on Microsoft.RL" This reverts commit c586920. * Revert "Add RankProcessor" This reverts commit 43967d3. * Revert "Remove unused file" This reverts commit 1e400a8. * Revert "Toggle commented codes so that most tests can pass" This reverts commit 7ec50ec. * Delete DisposeHelper.cs * Revert "Revert "12767907: Add a package dependency on Microsoft.RL"" This reverts commit d6df6b5. * Cleanup after reverting * generating client configuration using autorest * Revert "generating client configuration using autorest" This reverts commit 6b75695. * Configuration details for livemodel * cleanup * cleanup * Added ToDo comments * correct the version in endpoints Co-authored-by: Tejaswi Paruchuri <[email protected]> Co-authored-by: Tparuchuri <[email protected]> * 12768045: Add rank processor for single slot (#3) Add RankProcessor Remove unused file Remove some key Toggle commented codes so that most tests can pass RankProcessor is working. Still need to clean up the codes Add test json files only construct LiveModel when localInference is true Move rankprocessor to under model Add latest generated files from autorest Add autogenerated files from autorest Fix the setting in DecisionContext Remove wrong comment Revert unexpected auto-generated file changes Addressed a comment Move suppression to GlobalSuppressions.cs Remove the decoration in MultiSlotClient.cs Refactor rankProcessor so that the actions is not modified and restored; added a converter between sdk and Rl.Net; replace Newtonsoft.json with System.Text.Json Address comments Refactor RlObjuectConverter; remove some unneeded setters, etc. Added unit tests for RlObjectConverter, DecisionContext, JsonRawStringListConverter Remove unused using * 13009290: Create RankProcessor class to Azure Personalizer client library for .NET for multi slot (#7) * 13009290: Create RankProcessor class to Azure Personalizer client library for .NET for multi slot * Address comments * Subsampling * Cleanup and added tests * Cleanup and added tests * cleanup * cleanup * Single SubSample rate * Cleanup * Cleanup * Variable name * 13192221: Point to the Rl.Net nuget package from SDK dev drop (#9) * Adding test code for Model/Put api and moving to preview.3 * Johnhuang/13216589 reward activate local inference api (#10) * Update API view file * 13216589: Add local inference reward api 13216590: Add local inference activate api * Fix the comments * Remove IsDisposed method * Revert "Update API view file" This reverts commit 0a4dcbf. * Remove MultiSlotClient class as it is never used * Add more change after other PRs were merged * Revert "Add more change after other PRs were merged" This reverts commit 35a3120. * 13225147: Update API view file for thick client (#11) * 13225147: Update API view file for thick client * Remove some suppressing warning * Add more changes after 2 PRs were merged * lazy load rank processor, refresh config and relaod token on expiry (#12) * lazy load rank processor, refresh config and relaod token on expiry * Comments & Cleanup * Cleanup * Addressed comments * Cleanup * Fix conflict Co-authored-by: John Huang <[email protected]> * 13309375: Improve the tests by mocking LiveModel when test mode is (#14) * 13309375: Improve the tests by mocking LiveModel when test mode is Playback but using real LiveModel when test mode is Live * Change some public classes to private * Address those comments in the main PR * Update Microsoft.RL version * Rename some properties * Update Microsoft.RL version * Fix spelling check * Remove the unneeded warning * Update live model config * Use similar method names for export and import apis * Johnhuang/13170630 address thick client sdk review (#18) * 13170630: Address comments from SDK review * Update API view file * Remove unnecessary file under Gerated folder * Add more test for RLObjectConverter * Remove unused parameter * Remove unneeded comment * Update recording sessions for tests that were changed in earlier commit. (#19) Co-authored-by: Personalizer Team <[email protected]> * Update api view file after model api change (#20) * Rename modelStream to modelBody (#21) * Update Microsoft.RL version * 13678440: Some cosmetic change to address comments from DotNet team (#22) * Update method names for Export/Import models as per suggestions. (#23) * Update method names for Export/Import models as per suggestions. * Updates after running codecheck.ps1 script Co-authored-by: Personalizer Team <[email protected]> Co-authored-by: Tejaswi Paruchuri <[email protected]> Co-authored-by: Tparuchuri <[email protected]> Co-authored-by: Personalizer Team <[email protected]> Co-authored-by: Sharath Malladi <[email protected]>

johnhuang01 requested changes Feb 8, 2022

View reviewed changes

tyclintw requested changes Feb 8, 2022

View reviewed changes

johnhuang01 reviewed Feb 8, 2022

View reviewed changes

tyclintw reviewed Feb 8, 2022

View reviewed changes

johnhuang01 approved these changes Feb 8, 2022

View reviewed changes

tyclintw approved these changes Feb 9, 2022

View reviewed changes

Tparuchuri and others added 6 commits February 9, 2022 12:05

lazy load rank processor, refresh config and relaod token on expiry

bd244b3

Comments & Cleanup

32642da

Cleanup

5c7af08

Addressed comments

3d418ff

Cleanup

ff4ca54

Fix conflict

3eafa1b

johnhuang01 force-pushed the refreshlivemodel branch from 11db8ec to 3eafa1b Compare February 10, 2022 07:51

johnhuang01 merged commit fc8a166 into thickClientFeature Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lazy load rank processor, refresh config and relaod token on expiry #12

lazy load rank processor, refresh config and relaod token on expiry #12

Tparuchuri commented Feb 8, 2022

johnhuang01 Feb 8, 2022

Tparuchuri Feb 8, 2022

johnhuang01 Feb 8, 2022

Tparuchuri Feb 8, 2022

johnhuang01 Feb 8, 2022

johnhuang01 Feb 8, 2022

Tparuchuri Feb 8, 2022

johnhuang01 Feb 8, 2022

tyclintw Feb 8, 2022

Tparuchuri Feb 8, 2022

tyclintw Feb 8, 2022

Tparuchuri Feb 8, 2022

tyclintw Feb 8, 2022

Tparuchuri Feb 8, 2022

tyclintw Feb 8, 2022

tyclintw Feb 8, 2022

tyclintw Feb 8, 2022

tyclintw Feb 8, 2022 •

edited

Loading

tyclintw Feb 8, 2022

Tparuchuri Feb 8, 2022

lazy load rank processor, refresh config and relaod token on expiry #12

lazy load rank processor, refresh config and relaod token on expiry #12

Conversation

Tparuchuri commented Feb 8, 2022

All SDK Contribution checklist:

Additional management plane SDK specific contribution checklist:

Management plane SDK Troubleshooting

Old outstanding PR cleanup

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyclintw Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyclintw Feb 8, 2022 •

edited

Loading