Johnhuang/12768045 rank processor #3

johnhuang01 · 2021-12-24T09:44:54Z

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.

tyclintw · 2022-01-14T18:14:52Z

sdk/personalizer/Azure.AI.Personalizer/src/Generated/EvaluationsClient.cs

@@ -31,7 +31,7 @@ protected EvaluationsClient()
        /// <param name="endpoint"> Supported Cognitive Services endpoint. </param>
        /// <param name="credential"> A credential used to authenticate to an Azure Service. </param>
        /// <param name="options"> The options for configuring the client. </param>
-        public EvaluationsClient(string endpoint, TokenCredential credential, PersonalizerClientOptions options = null)


All clients appear to have lost their TokenCredential constructors. Please revert this change. TokenCredential is required for ActiveDirectory (AD) Authentication.

This is auto-generated. Let me check why this change was introduced.

tyclintw · 2022-01-14T21:47:08Z

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

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using Newtonsoft.Json;


The rest of the code uses System.Text.Json. Can we unify on one parsing library?

I have tried this for more than a half day and got stuck on migrating JsonRawStringListConverter. I decided to hold on this work. Also as I found other projects in azure-sdk-for-net use both System.Text.Json and Newtonsoft.Json, we should be fine to use both of them. But if you think this is highly suggested, I will create a task to work on this later.

After several hours of effort, I have made it work. See the latest commit.

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

tyclintw · 2022-01-14T22:02:51Z

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

+            }
+
+            // Remove excluded actions in options
+            options.Actions = options.Actions.Where(action => !excludedSet.Contains(action.Id));


Please try to keep functions idempotent. Function parameters should not be modified unless that's the explicit intention of the function.

tyclintw · 2022-01-14T22:04:54Z

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

+                    rankableActions.Add(action);
+                }
+                ++idx;
+            }


This logic should be unnecessary. The Rank requests supports a Lock action parameter to handle the excluded case.

I feel introducing "Lock" to the azure sdk is not necessary as long as the functionality is the same.

orenmichaely

Did you make manual changes in the Generated/ folder? if so, they should be reverted. No changes should be make in the generated folder. You can make changes in the /Models folder (not under generated) and use special decorators so that the code is generated as desired.

https://github.com/Azure/autorest.csharp#customizing-the-generated-code

orenmichaely · 2022-01-18T20:27:40Z

sdk/core/Azure.Core.TestFramework/src/Azure.Core.TestFramework.csproj

@@ -1,6 +1,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>$(RequiredTargetFrameworks);net47</TargetFrameworks>
+    <NoWarn>$(NoWarn);SYSLIB0023</NoWarn>


I think we should add these to our own project files, not to the Azure.Core.TestFramework project file.

I was not able to build this Azure.Core.TestFramework project if I don't add this line. This fix can't be in our project.

Than can you address the warning on a line by line basis?

Changed to only suppress the line.

orenmichaely · 2022-01-18T20:31:56Z

sdk/personalizer/Azure.AI.Personalizer/src/Azure.AI.Personalizer.csproj

@@ -32,6 +32,8 @@

  <ItemGroup>
    <PackageReference Include="Azure.Core" />
+    <PackageReference Include="Microsoft.RL" VersionOverride="1.0.18120003-INTERNAL" />
+    <PackageReference Include="Newtonsoft.Json" VersionOverride="10.0.3" />


We are already using System.text.Json in the sdk

Yes. Tyler had the similar comment. The following is my answer:
I have tried this for more than a half day and got stuck on migrating JsonRawStringListConverter. I decided to hold on this work. Also as I found other projects in azure-sdk-for-net use both System.Text.Json and Newtonsoft.Json, we should be fine to use both of them. But if you think this is highly suggested, I will create a task to work on this later.

orenmichaely · 2022-01-18T21:28:30Z

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

+        /// <summary> Properties from url </summary>
+        [JsonProperty("FromUrl", NullValueHandling = NullValueHandling.Ignore)]
+        [JsonConverter(typeof(JsonRawStringListConverter))]
+#pragma warning disable CA2227 // Collection properties should be read only


[nit] this should be moved to the project file. For my knowledge, why do we need SharedFromUrl to not be read only?

orenmichaely · 2022-01-18T22:37:38Z

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

 namespace Azure.AI.Personalizer
 {
+    [CodeGenModel("MultiSlotClient")]


The CodeGenModel decorator is for renaming a class, but you are not renaming it, so it is unnecessary

Right. We should probably remove this class as it is not used either.

orenmichaely · 2022-01-18T22:44:17Z

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

        /// <param name="options"> The options for configuring the client. </param>
-        public PersonalizerClient(Uri endpoint, TokenCredential credential, bool isLocalInference, PersonalizerClientOptions options = null) :
+        public PersonalizerClient(Uri endpoint, TokenCredential credential, bool isLocalInference, Configuration configuration, PersonalizerClientOptions options = null) :


[nit] create constructors without default arguments as well

Why do we need to define another constructor without a default argument? Would the current one cover both definitions?

isLocalInference needs a default value of false and Configuration should not be a parameter.

[Why do we need to define another constructor without a default argument? Would the current one cover both definitions?] -- the SDK team asked me to add constructors without the default arguments as part of my sdk review. They explained that it is a better experience for users to have a constructor without default arguments. The additional default argument tends to cause confusion if it is not used. This is why I added the PersonalizerClient constructors without the default argument which simply calls the other constructor.

orenmichaely · 2022-01-18T22:46:56Z

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

-                return await RankRestClient.RankAsync(options, cancellationToken).ConfigureAwait(false);
+                if (_isLocalInference)
+                {
+                    return _rankProcessor.Rank(options);


does rankProcessor not have an async method?

I don't see an async method needed for local inference.

orenmichaely · 2022-01-18T22:49:27Z

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

+            return Response.FromValue(value, default);
+        }
+
+        public static PersonalizerRankResult GenerateRankResult(List<PersonalizerRankableAction> originalActions,


[nit] naming -- should be GenerateRankResult

It is GenerateRankResult already. :)

orenmichaely

I noticed that a lot of the added code is taken from the decision service repo (converting a RankRequest into the correct format to be passed into vw and same thing for the response).

I think that a lot of that code can be improved (mainly variable names). When the SDK team reviews the code, they will want to understand it and they will have a hard time understanding class names like DecisionContextDocument or variable names like SharedFromUrl

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

johnhuang01 · 2022-01-14T00:22:50Z

sdk/core/Azure.Core.TestFramework/src/Azure.Core.TestFramework.csproj

@@ -1,6 +1,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>$(RequiredTargetFrameworks);net47</TargetFrameworks>
+    <NoWarn>$(NoWarn);SYSLIB0023</NoWarn>


Have to add this to make the build succeed

johnhuang01 · 2022-01-14T00:23:41Z

sdk/personalizer/Azure.AI.Personalizer/src/Azure.AI.Personalizer.csproj

@@ -32,6 +32,8 @@

  <ItemGroup>
    <PackageReference Include="Azure.Core" />
+    <PackageReference Include="Microsoft.RL" VersionOverride="1.0.18120003-INTERNAL" />


TODO: Need to take time to investigate whether we can remove VersionOverride in this file.

johnhuang01 · 2022-01-14T17:49:41Z

sdk/personalizer/Azure.AI.Personalizer.sln

@@ -17,6 +17,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Azure.Core.TestFramework", "..\core\Azure.Core.TestFramework\src\Azure.Core.TestFramework.csproj", "{0F88C67F-34D2-4C68-B5BF-08A547D4CC2E}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Azure.Core", "..\core\Azure.Core\src\Azure.Core.csproj", "{CFB35402-69EB-448F-82B7-2D284730B0A6}"


Have to add this project because Azure.Core.TestFramework depends on this.

johnhuang01 · 2022-01-14T22:10:36Z

sdk/personalizer/Azure.AI.Personalizer/src/Generated/EvaluationsClient.cs

@@ -31,7 +31,7 @@ protected EvaluationsClient()
        /// <param name="endpoint"> Supported Cognitive Services endpoint. </param>
        /// <param name="credential"> A credential used to authenticate to an Azure Service. </param>
        /// <param name="options"> The options for configuring the client. </param>
-        public EvaluationsClient(string endpoint, TokenCredential credential, PersonalizerClientOptions options = null)


This is auto-generated. Let me check why this change was introduced.

johnhuang01 · 2022-01-18T04:30:45Z

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

+                    rankableActions.Add(action);
+                }
+                ++idx;
+            }


I feel introducing "Lock" to the azure sdk is not necessary as long as the functionality is the same.

johnhuang01 · 2022-01-18T23:03:31Z

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

+            return Response.FromValue(value, default);
+        }
+
+        public static PersonalizerRankResult GenerateRankResult(List<PersonalizerRankableAction> originalActions,


It is GenerateRankResult already. :)

johnhuang01 · 2022-01-18T23:04:15Z

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

-                return await RankRestClient.RankAsync(options, cancellationToken).ConfigureAwait(false);
+                if (_isLocalInference)
+                {
+                    return _rankProcessor.Rank(options);


I don't see an async method needed for local inference.

johnhuang01 · 2022-01-18T23:58:27Z

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

        /// <param name="options"> The options for configuring the client. </param>
-        public PersonalizerClient(Uri endpoint, TokenCredential credential, bool isLocalInference, PersonalizerClientOptions options = null) :
+        public PersonalizerClient(Uri endpoint, TokenCredential credential, bool isLocalInference, Configuration configuration, PersonalizerClientOptions options = null) :


Why do we need to define another constructor without a default argument? Would the current one cover both definitions?

johnhuang01 · 2022-01-18T23:58:59Z

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

 namespace Azure.AI.Personalizer
 {
+    [CodeGenModel("MultiSlotClient")]


Right. We should probably remove this class as it is not used either.

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

johnhuang01 · 2022-01-20T09:15:51Z

I noticed that a lot of the added code is taken from the decision service repo (converting a RankRequest into the correct format to be passed into vw and same thing for the response).

I think that a lot of that code can be improved (mainly variable names). When the SDK team reviews the code, they will want to understand it and they will have a hard time understanding class names like DecisionContextDocument or variable names like SharedFromUrl

Added a RlObjectConverter to address the comment.

tyclintw · 2022-01-20T19:06:12Z

NuGet.Config

@@ -10,6 +10,7 @@
      Used for azure-sdk-tools repo until issue https://github.com/Azure/azure-sdk-tools/issues/1329 is addressed
    -->
    <add key="azure-sdk-tools" value="https://azuresdkartifacts.blob.core.windows.net/azure-sdk-tools/index.json" />
+    <add key="air-msrai-decisionservice-Consumption" value="https://msazure.pkgs.visualstudio.com/one/_packaging/air-msrai-decisionservice-Consumption/nuget/v3/index.json" />


[revert] We are not to have multiple NuGet sources in a build. Please make RLNet a downstream consumption of azure-sdk-tools.

We are not there yet. We need to first build the Nuget package and provide the package file to dotNet team. Then they can put it under their dev feed: https://dev.azure.com/azure-sdk/public/_packaging?_a=feed&feed=azure-sdk-for-net. Then we will update this line to point to the dev feed. Currently we still don't have a good Nuget package. This is still a temporary location just to make sure we can continue our implementation.

tyclintw · 2022-01-20T19:06:54Z

sdk/core/Azure.Core.TestFramework/src/Azure.Core.TestFramework.csproj

@@ -1,6 +1,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>$(RequiredTargetFrameworks);net47</TargetFrameworks>
+    <NoWarn>$(NoWarn);SYSLIB0023</NoWarn>


Than can you address the warning on a line by line basis?

tyclintw · 2022-01-20T19:08:01Z

sdk/personalizer/Azure.AI.Personalizer/src/Azure.AI.Personalizer.csproj

@@ -32,6 +32,8 @@

  <ItemGroup>
    <PackageReference Include="Azure.Core" />
+    <PackageReference Include="Microsoft.RL" VersionOverride="1.0.18120003-INTERNAL" />
+    <PackageReference Include="System.Text.Json" VersionOverride="6.0.1" />


Is System.Text.Json necessary? I thought this was already included?

The default is old 4.* version. But it won't support [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]. So I have to override the version to a higher one.

tyclintw · 2022-01-20T19:09:29Z

sdk/personalizer/Azure.AI.Personalizer/src/GlobalSuppressions.cs

@@ -4,3 +4,5 @@
 using System.Diagnostics.CodeAnalysis;

 [assembly: SuppressMessage("Usage", "AZC0016:Invalid ServiceVersion member name.", Justification = "Generated code: https://github.com/Azure/autorest.csharp/issues/1524", Scope = "type", Target = "~T:Azure.AI.Personalizer.PersonalizerClientOptions.ServiceVersion")]
+[assembly: SuppressMessage("Usage", "CA2227:Collection properties should be read only", Justification = "<Pending>", Scope = "member", Target = "~P:Azure.AI.Personalizer.DecisionContext.SharedFromUrl")]


Can you point me to the lines of code that require suppression? Warnings are generally there for a reason.

I have removed this line as I don't need it. You will see it in my next commit.

tyclintw · 2022-01-20T19:18:02Z

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

        /// <param name="options"> The options for configuring the client. </param>
-        public PersonalizerClient(Uri endpoint, TokenCredential credential, bool isLocalInference, PersonalizerClientOptions options = null) :
+        public PersonalizerClient(Uri endpoint, TokenCredential credential, bool isLocalInference, Configuration configuration, PersonalizerClientOptions options = null) :


isLocalInference needs a default value of false and Configuration should not be a parameter.

tyclintw · 2022-01-20T19:18:58Z

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

@@ -27,7 +27,7 @@ public partial class PersonalizerRankOptions
        /// should match the sequence your application would have used to display them.
        /// The first item in the array will be used as Baseline item in Offline Evaluations.
        /// </summary>
-        public IEnumerable<PersonalizerRankableAction> Actions { get; }
+        public IEnumerable<PersonalizerRankableAction> Actions { get; set; }


It is not needed either. Reverted.

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

tyclintw · 2022-01-20T19:21:54Z

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

+        {
+            if (String.IsNullOrEmpty(options.EventId))
+            {
+                options.EventId = Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture);


Do not modify the input parameters.

tyclintw · 2022-01-20T19:22:23Z

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

+            int idx = 0;
+            foreach (var action in options.Actions)
+            {
+                action.Index = idx;


Do not modify the input parameters.

tyclintw · 2022-01-20T19:25:32Z

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

+            return GenerateRankResponse(originalActions, rankableActions, excludedActions, rankedIndices, rankingProbabilities, eventId);
+        }
+
+        private static PersonalizerRankResult GenerateRankResponse(List<PersonalizerRankableAction> originalActions,


GenerateRankResponse and GenerateRankResult should have the same name. It's confusing otherwise. Also, I don't see the benefit of the two methods. Why can't all the logic be done in GenerateRankeResult?

I changed the GenerateRankResponse to GenerateRankResultInner. I don't want to combine the 2 methods as my next PR for multi-slot will call GenerateRankResultInner with different parameters.

orenmichaely · 2022-01-21T18:49:05Z

sdk/core/Azure.Core.TestFramework/src/TestRecording.cs

@@ -196,10 +196,12 @@ public TestRandom Random
 #if NET6_0_OR_GREATER
                            var liveSeed = RandomNumberGenerator.GetInt32(int.MaxValue);
 #else
+#pragma warning disable SYSLIB0023


This is external code, I dont think we should be changing it

Unfortunately, without this change, building will fail.

orenmichaely · 2022-01-21T18:56:59Z

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

+            List<PersonalizerRankableAction> rankableActions = new List<PersonalizerRankableAction>();
+            List<PersonalizerRankableAction> excludedActions = new List<PersonalizerRankableAction>();
+            int idx = 0;
+            foreach (var action in options.Actions)


[nit] I think it would be cleaner if the logic of populating originalActions, rankableActions, excludedActions was moved to a helper method

I feel this is ok as the whole method is less than 50 lines.

orenmichaely · 2022-01-21T19:46:15Z

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

+        /// <param name="rankRequest"> Personalizer Rank Options </param>
+        public DecisionContext(PersonalizerRankOptions rankRequest)
+        {
+            List<string> jsonFeatures = rankRequest.ContextFeatures.Select(f => JsonSerializer.Serialize(f)).ToList();


[naming] these are the context features, so I think a better name would be contextFeatures

I did some refactoring for this method so that the local variable does not exist any more. But I renamed the other places. Will push the comment very soon.

orenmichaely · 2022-01-21T19:48:10Z

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

+            this.Documents = rankableActions
+                .Select(action =>
+                {
+                    List<string> jsonFeatures = action.Features.Select(f => JsonSerializer.Serialize(f)).ToList();


[nit][naming] action features

orenmichaely · 2022-01-21T19:51:14Z

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

+        [JsonPropertyName("FromUrl")]
+        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+        [JsonConverter(typeof(JsonRawStringListConverter))]
+        public List<string> SharedFromUrl { get; }


"FromUrl" is the default context feature namespace we add. This namespace contains all context features which are passed in by the user. I think this property should be called ContextFeatures

orenmichaely · 2022-01-21T19:55:30Z

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

+        /// <summary>
+        /// Convert PersonalizerRankOptions object to a json context string for Rl.Net
+        /// </summary>
+        public static string ConvertToContextJson(PersonalizerRankOptions personalizerRankOptions)


DecisionContext contains information about the entire rank request, context and actions. I think you should change the name to ConvertToDecisionContext for clarity

This is actually matching the parameter name of the method definition in LiveModel:
public RankingResponse ChooseRank(string eventId, string contextJson, ActionFlags flags);
So I feel this is better.

orenmichaely · 2022-01-21T20:00:03Z

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

+            }
+
+            // Convert options to the compatible parameter for ChooseRank
+            var contextJson = RlObjectConverter.ConvertToContextJson(options);


It looks like you are computing RankableActions in two places.

in ConvertToContextJson you are creating a DecisionContext which has the rankable actions

in this method

I think you can split the ConvertToContextJson method so that you maintain an unserialized DecisionContext and then you will not need to recompute the Rankable and excluded action list

We discussed offline. I will take a different approach by defining a different constructor for DecisionContext:
public DecisionContext(IEnumerable contextFeatures, List rankableActions)

orenmichaely · 2022-01-21T20:01:36Z

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

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System.Text.Json;


Did you add unit tests for the converter and other classes you added which can be tested in isolation?

Added unit tests for RlObjectConverter, DecisionContext, JsonRawStringListConverter. Please take a look when you have time.

johnhuang01 · 2022-01-22T21:40:21Z

sdk/personalizer/Azure.AI.Personalizer/tests/Personalizer/RlObjectConverterTests.cs

+                "}";
+            Assert.IsTrue(contextJson.Equals(expectedJson));
+        }
+    }


Can't add any unit test for "GenerateRankResult" because a meaningful RankingResponse is not able to be created as it is seal class and all its properties are readonly and no constructor to set them.

tyclintw

Preliminary approval as a work in progress. Multiple items such as a the Configuration logic is incorrect, but I'm assuming they will be address with further changes.

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

* 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 <tparuchuri@microsoft.com> Co-authored-by: Tparuchuri <86433817+Tparuchuri@users.noreply.github.com> * 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 <johnhuang@microsoft.com> * 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 <personalizer@microsoft.com> * 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 <personalizer@microsoft.com> Co-authored-by: Tejaswi Paruchuri <tparuchuri@microsoft.com> Co-authored-by: Tparuchuri <86433817+Tparuchuri@users.noreply.github.com> Co-authored-by: Personalizer Team <personalizer@microsoft.com> Co-authored-by: Sharath Malladi <msharath@live.com>

johnhuang01 changed the title ~~Johnhuang/12768045 rank processor~~ Johnhuang/12768045 rank processor (Not ready for review) Jan 6, 2022

johnhuang01 changed the title ~~Johnhuang/12768045 rank processor (Not ready for review)~~ Johnhuang/12768045 rank processor Jan 14, 2022

tyclintw requested changes Jan 14, 2022

View reviewed changes

orenmichaely suggested changes Jan 18, 2022

View reviewed changes

orenmichaely reviewed Jan 18, 2022

View reviewed changes

sdk/personalizer/Azure.AI.Personalizer/src/Models/RankProcessor.cs Show resolved Hide resolved

johnhuang01 commented Jan 19, 2022

View reviewed changes

tyclintw requested changes Jan 20, 2022

View reviewed changes

orenmichaely reviewed Jan 21, 2022

View reviewed changes

johnhuang01 force-pushed the johnhuang/12768045-rankProcessor branch from 962214f to 4f4bbb9 Compare January 22, 2022 21:34

johnhuang01 commented Jan 22, 2022

View reviewed changes

tyclintw approved these changes Jan 25, 2022

View reviewed changes

orenmichaely approved these changes Jan 25, 2022

View reviewed changes

johnhuang01 force-pushed the johnhuang/12768045-rankProcessor branch from 4f4bbb9 to 1610103 Compare January 26, 2022 00:13

johnhuang01 force-pushed the johnhuang/12768045-rankProcessor branch from 1610103 to ac6bb43 Compare January 26, 2022 00:15

johnhuang01 merged commit 062871e into thickClientFeature Jan 26, 2022

Johnhuang/12768045 rank processor #3

Johnhuang/12768045 rank processor #3

Conversation

johnhuang01 commented Dec 24, 2021 • edited Loading

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

orenmichaely left a 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

orenmichaely Jan 20, 2022 • edited Loading

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

orenmichaely left a comment • edited Loading

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

johnhuang01 commented Jan 20, 2022

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

johnhuang01 Jan 21, 2022 • edited Loading

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

johnhuang01 Jan 21, 2022 • edited Loading

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

johnhuang01 commented Dec 24, 2021 •

edited

Loading

orenmichaely Jan 20, 2022 •

edited

Loading

orenmichaely left a comment •

edited

Loading

johnhuang01 Jan 21, 2022 •

edited

Loading

johnhuang01 Jan 21, 2022 •

edited

Loading