Client configuration by Tparuchuri · Pull Request #4 · johnhuang01/azure-sdk-for-net

Tparuchuri · 2022-01-13T22:23:25Z

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.

Tparuchuri · 2022-01-13T22:29:57Z

                }
                else
                {
+                    Models.PersonalizerClientProperties result = await ClientConfigurationRestClient.PostAsync(cancellationToken).ConfigureAwait(false);


Calling the ClientConfigurationClient here instead when isLocalInference is true (these two lines will be moved) just to test the new code is working when RankTest is called. But few things have to be modified while testing for the test case to work. Tested in my local machine and it worked as below.
Standard Output:
InternalId: 86aa7660871843dca1c703400557b71c
StorageBlobUri: BlobURI
LearningMode: 0
Exploration Percentage: 1
initialCommandline: --cb_explore_adf --json --quiet --epsilon 0.0 --first_only --id N/A

The point of the thick client, is decrease latency. By making a configuration network request on every rank call, the latency is effectively increased. This configuration should also be shared across the entire client, not just the rank call. Consider moving to a wider scope and changing the client configuration to a TTL lazy initialization.

tyclintw · 2022-01-14T00:32:06Z

                }
                else
                {
+                    Models.PersonalizerClientProperties result = await ClientConfigurationRestClient.PostAsync(cancellationToken).ConfigureAwait(false);


The point of the thick client, is decrease latency. By making a configuration network request on every rank call, the latency is effectively increased. This configuration should also be shared across the entire client, not just the rank call. Consider moving to a wider scope and changing the client configuration to a TTL lazy initialization.

tyclintw · 2022-01-14T00:32:52Z

        /// <param name="cancellationToken"> The cancellation token to use. </param>
        public virtual async Task<Response<PersonalizerRankResult>> RankAsync(IEnumerable<PersonalizerRankableAction> actions, IEnumerable<object> contextFeatures, CancellationToken cancellationToken = default)
        {
+            Models.PersonalizerClientProperties result = await ClientConfigurationRestClient.PostAsync(cancellationToken).ConfigureAwait(false);


As mentioned in the previous comment, this is duplicated logic.

tyclintw · 2022-01-14T00:33:49Z

        public virtual async Task<Response<PersonalizerRankResult>> RankAsync(IEnumerable<PersonalizerRankableAction> actions, IEnumerable<object> contextFeatures, CancellationToken cancellationToken = default)
        {
+            Models.PersonalizerClientProperties result = await ClientConfigurationRestClient.PostAsync(cancellationToken).ConfigureAwait(false);
+            Console.WriteLine("InternalId: " + result.ApplicationID + "\nStorageBlobUri: " + result.ModelBlobUri + "\nLearningMode: " + result.ProtocolVersion + "\nExploration Percentage: " + result.InitialExplorationEpsilon + "\ninitialCommandline: " + result.InitialCommandLine);


Do not write to console in production code. If something needs to be output, it should be done through a logger. For this particular case, I would suggest removing entirely.

TIP: When you want to print the entire object is usually best to use JSON serialization. Console.Write(JsonConvert.SerializeObject(result));

This reverts commit c586920.

This reverts commit 43967d3.

This reverts commit 1e400a8.

This reverts commit 7ec50ec.

johnhuang01 · 2022-01-14T19:51:44Z

+            config["appid"] = result.Value.ApplicationID;
+
+            // set up the model
+            config["model.source"] = result.Value.ModelBlobUri;


should be "model.blob.uri"

johnhuang01 · 2022-01-14T19:54:52Z

+
+            // set up the model
+            config["model.source"] = result.Value.ModelBlobUri;
+            config["model.vw.initial_command_line"] = result.Value.InitialCommandLine;


Should be "vw.commandline"

This reverts commit d6df6b5.

…azure-sdk-for-net into ClientConfiguration

tyclintw · 2022-01-14T21:36:43Z

+
+namespace Azure.AI.Personalizer.Models
+{
+    internal class ClientConfigurationRestClient


Why isn't this class in the generated folder? Rest clients should not be hand written, only extended.

As client Post is not part of swagger because of which it's not part of generated. So implemented it outside generated to get client config details

tyclintw · 2022-01-14T21:38:53Z

+        /// <summary> Gets the configuration details for the live model to use </summary>
+        internal Configuration GetClientConfigurationForLiveModel()
+        {
+            result = new Lazy<Models.PersonalizerClientProperties>(() => ClientConfigurationRestClient.Post());


This should have a TTL in case the configuration on the server is updated.

Also, I would suggest the Lazy TTL be managed by the caller, not inside this method. In other words, the caller should use Lazy.

tyclintw · 2022-01-14T21:39:32Z

+            result = new Lazy<Models.PersonalizerClientProperties>(() => ClientConfigurationRestClient.Post());
+            Configuration config = new Configuration();
+            // configure the personalizer loop
+            config["appid"] = result.Value.ApplicationID;


Instead of call result.Value half a dozen times, please use a temporary reference.

tyclintw · 2022-01-14T21:42:59Z

+{
+    internal partial class PersonalizerClientProperties
+    {
+        internal static PersonalizerClientProperties DeserializePersonalizerServiceProperties(JsonElement element)


Can you explain why we need a custom deserializer instead of using JsonSerializer.Deserialize<PersonalizerClientProperties>(string)?

This reverts commit 6b75695.

johnhuang01 and others added 7 commits December 10, 2021 19:20

12767907: Add a package dependency on Microsoft.RL

c586920

Add RankProcessor

43967d3

Remove unused file

1e400a8

Remove some key

8e1be98

Toggle commented codes so that most tests can pass

7ec50ec

Getting the required config details for rankprocessor to enable livemode

0d83604

Updating the wrong variable name

7bef96f

Tparuchuri commented Jan 13, 2022

View reviewed changes

Tparuchuri changed the base branch from main to thickClentFeature January 13, 2022 23:03

tyclintw reviewed Jan 14, 2022

View reviewed changes

Tparuchuri and others added 6 commits January 14, 2022 09:59

Cleanup

db8c651

Revert "12767907: Add a package dependency on Microsoft.RL"

d6df6b5

This reverts commit c586920.

Revert "Add RankProcessor"

70c9738

This reverts commit 43967d3.

Revert "Remove unused file"

ebbbdad

This reverts commit 1e400a8.

Revert "Toggle commented codes so that most tests can pass"

9856a35

This reverts commit 7ec50ec.

Delete DisposeHelper.cs

4255840

johnhuang01 reviewed Jan 14, 2022

View reviewed changes

Tparuchuri added 3 commits January 14, 2022 12:11

Revert "Revert "12767907: Add a package dependency on Microsoft.RL""

86532d3

This reverts commit d6df6b5.

Merge branch 'ClientConfiguration' of https://github.com/johnhuang01/…

8156ddb

…azure-sdk-for-net into ClientConfiguration

Cleanup after reverting

c37c4d3

tyclintw requested changes Jan 14, 2022

View reviewed changes

generating client configuration using autorest

6b75695

johnhuang01 approved these changes Jan 18, 2022

View reviewed changes

Tparuchuri added 4 commits January 20, 2022 08:04

Revert "generating client configuration using autorest"

1f0a161

This reverts commit 6b75695.

Configuration details for livemodel

59e364e

cleanup

92f6ba3

cleanup

2c7e79c

johnhuang01 reviewed Jan 20, 2022

View reviewed changes

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

Tparuchuri added 2 commits January 20, 2022 14:36

Added ToDo comments

23d1c93

correct the version in endpoints

a914445

tyclintw approved these changes Jan 25, 2022

View reviewed changes

Tparuchuri merged commit 355227d into thickClentFeature Jan 25, 2022

Conversation

Tparuchuri commented Jan 13, 2022

All SDK Contribution checklist:

Additional management plane SDK specific contribution checklist:

Management plane SDK Troubleshooting

Old outstanding PR cleanup

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyclintw Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tyclintw Jan 14, 2022 •

edited

Loading