Update the `ICommandPredictor` to provide more feedback and also make feedback easier to be corelated by daxian-dbw · Pull Request #14649 · PowerShell/PowerShell

daxian-dbw · 2021-01-21T23:18:41Z

PR Summary

Update the ICommandPredictor interface APIs to allow a predictor to get feedback about what suggestions from it were displayed to the user, and also make it easier for a predictor to corelate feedback events.

Add ICommandPredictor.OnSuggestionDisplayed, to let a predictor know what results were displayed to the user when rendering the results: void OnSuggestionDisplayed(uint session, int countOrIndex);
- The countOrIndex parameter can help a predictor to tell if the display was in ListView or InlineView:
  - When the value is > 0, it's the number of displayed suggestions from the list returned in , starting from the index 0. This indicates the ListView is in use.
  - When the value is <= 0, it means a single suggestion from the list got displayed, and the index is the absolute value. This indicates the InlineView is in use.
Add clientId and session parameters to the existing interface APIs. The clientId parameter helps a predictor to know what host client is calling it (PSReadLine only today, but it could be the PS VSCode extension some day in future); the session parameter helps a predictor to corelate feedback events.
- GetSuggestion and StartEarlyProcessing both take a string parameter clientId, so it knows the client that makes the call
- GetSuggestion returns a uint type session value, along with the list of suggestion entries it provides.
- OnSuggestionDisplayed and OnSuggestionAccepted both take a uint parameter session, which indicates where the displayed or accepted suggestions were from.

/cc @kceiw

This PR also includes a few changes to the scripts that generate PowerShell SDK NuGet packages, to make it works properly with the other changes from this PR, and also make NuGet packages include the XML document files.

PR Checklist

iSazonov · 2021-01-25T15:31:54Z

src/System.Management.Automation/engine/Subsystem/CommandPrediction/ICommandPredictor.cs

+        void OnSuggestionDisplayed(uint session, int countOrIndex);
+
+        /// <summary>
+        /// The suggestion provided by the predictor was accepted.
+        /// </summary>
+        /// <param name="session">Represents the mini-session where the accepted suggestion came from.</param>
+        /// <param name="acceptedSuggestion">The accepted suggestion text.</param>
+        void OnSuggestionAccepted(uint session, string acceptedSuggestion);


We could merge both methods if they are always used together.

They are not used together. OnSuggestionDisplayed will be fired way more than OnSuggestionAccepted.

My thoughts are if we get 100 suggestions we could weight all 100 (including displayed and accepted ones) and return the result to the prediction service so that it could make more better analyze. So users could prefer a case-sensitive selection or use other sources for selection (local configs, history or other prediction services) to inform the prediction service.

My thoughts are if we get 100 suggestions we could weight all 100 (including displayed and accepted ones) and return the result to the prediction service so that it could make more better analyze.

It's pretty common that we get suggestions from a predictor but user doesn't accept anything, so it's not clear to me how to always include "displayed and accepted ones" in a feedback.

After this change, "OnSuggestionDisplayed" event will be fired very often. I very much want to reduce it ..., so any ideas are very welcome. Can you maybe elaborate your thoughts more?

So users could prefer a case-sensitive selection or use other sources for selection (local configs, history or other prediction services) to inform the prediction service.

Not sure I understand what you mean, it would be nice if you can clarify more. Basically, we try to avoid sending more user local infor to a predictor than really needed.

We've heard more than once that users want to restore a PowerShell session after a restart. For this service it might be useful too.

My thoughts may be vague, but I believe that if a service does not store permanently the (session) state, then this state should be stored locally.

What is a session data? (1) Personal sensitive user data (stored only locally), (2) service data. Service data may be (1) complex data structure (like subset of ML model), (2) simple tips (references on internal service models)

I think that if user data should not be stored on the service, then there might be the following options:

No data is stored locally between sessions

All session data is stored locally (including model)

All sensitive data is stored locally (including service tips).

Also I think there are many claims about sensing user data to Cloud. The service could be data agnostic and gets encrypted data.

This feels like a design proposal for a separate feature "restore a PowerShell session after a restart" :)

iSazonov · 2021-01-25T15:33:50Z

src/System.Management.Automation/engine/Subsystem/CommandPrediction/CommandPrediction.cs

+        /// <summary>
+        /// Gets the mini-session id that represents a specific invocation to the <see cref="ICommandPredictor.GetSuggestion"/> API of the predictor.
+        /// </summary>
+        public uint Session { get; }


Why uint? Maybe Guid to be globally independent?

The session is always under a context, for example, a period of time when a predictor module is loaded and used, a Runspace instance id where the module is loaded. That context can be uniquely identified, so I think there is no need for the mini-session to be globally unique.

I use uint only because it provides a large space and easy to increment to get a new mini-session id. Azure PowerShell team is using this prediction API for the Az Predictor. I'm waiting on the their feedback on this.

The Az.Predictor team has confirmed that uint works fine, becuase they already have other context information that is uniquely identifiable. /cc @kceiw

…nd also make 'Session' use 'uint?' type

daxian-dbw · 2021-02-01T19:22:19Z

@rjmholt and @anmenaga this PR is ready, please help review it, thank!
@kceiw I will appreciate it if you can review this PR as well.

powercode · 2021-02-01T20:41:47Z

src/System.Management.Automation/engine/Utils.cs

            }
        }

+        internal static void NotNullOrEmpty(ICollection value, string paramName)


Perhaps use [CallerArgumentExpression("value")] ?

It's not implemented yet, right? See the proposal doc and the open issue: dotnet/csharplang#287

You're correct! I was misled by the docs. I have used CallerMemberName and read the documentation for CAE and just assumed it worked as well.

anmenaga

Looks fine.
Please file a doc issue and link it in "Documentation needed"/"Issue filed:" in PR Checklist; thank you.

anmenaga · 2021-02-04T01:42:49Z

tools/packaging/packaging.psm1

            Write-Log "Reference assembly '$assemblyName.dll' built and copied to $RefAssemblyDestinationPath"

+            Copy-Item $dllXmlDoc $RefAssemblyDestinationPath -Force
+            Write-Log "Xml document '$assemblyName.xml' copied to $RefAssemblyDestinationPath"


... make NuGet packages include the XML document files.
This is not really related to prediction APIs. Could have been a separate PR.

Not directly related, but sort of. The script needs to be updated after the changes in API because new attributes are used which need to be scrubbed when generating the reference assembly, so I have to update the packaging scripts. While doing so, I found the XML docs are missing when using our NuGet packages in VS Code or Visual Studio, and thus I fix that too.
Since we need to include part of the fixes in packaging.psm1 anyways, I prefer to keep all changes in this PR.

anmenaga · 2021-02-04T01:43:11Z

tools/packaging/packaging.psm1

+        @{
+            ApplyTo = @("System.Management.Automation")
+            Pattern = "[System.Runtime.CompilerServices.CompilerGeneratedAttribute, System.Runtime.CompilerServices.IsReadOnlyAttribute]"
+            Replacement = "/* [System.Runtime.CompilerServices.CompilerGeneratedAttribute, System.Runtime.CompilerServices.IsReadOnlyAttribute] */ "


Can you please explain why this is needed?

The CompilerGeneratedAttribute and IsReadOnlyAttribute are added by the C# compiler to the readonly members of a struct in the generated IL. Those attributes can only be used by the C# compiler and thus will result in complication error when they are in the source code. Therefore, we need to scrub them out before compiling to get our reference assembly.

daxian-dbw · 2021-02-04T18:48:15Z

@anmenaga Thanks for the review!
There is no need to update PowerShell docs. The PSSubsystemPluginModel experimental feature is only mentioned here: https://docs.microsoft.com/en-us/powershell/scripting/learn/experimental-features?view=powershell-7.1#pssubsystempluginmodel, and it doesn't talk about specific APIs. We only need to update the XML document for the APIs, which is done by the updates to packaging.psm1.

Using Experimental Features in PowerShell - PowerShell
Lists the currently available experimental features and how to use them.

ghost · 2021-02-12T02:07:16Z

🎉v7.2.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

daxian-dbw added 4 commits January 11, 2021 21:24

Update the ICommandPredictor interface with API changes

3d2cbce

Use 'uint' type for the mini-session id

589666e

Minor update

203683e

Fix script that create new NuGet package

4eb7aac

ghost assigned anmenaga Jan 21, 2021

Address CodeFactor issues

4500afb

daxian-dbw force-pushed the predict branch from ff8a72c to 4500afb Compare January 22, 2021 01:35

Update tests and also add more tests

e3eb21d

daxian-dbw changed the title ~~Predict~~ Update the ICommandPredictor to provide more feedback and also make feedback easier to be corelated Jan 22, 2021

Address codefactor issues

bea03e3

daxian-dbw force-pushed the predict branch from 351a118 to bea03e3 Compare January 22, 2021 18:52

iSazonov reviewed Jan 25, 2021

View reviewed changes

daxian-dbw added 2 commits January 28, 2021 13:05

Pass 'ClientId' to 'OnSuggestionDisplay' and 'OnSuggestionAccepted' a…

6bf5817

…nd also make 'Session' use 'uint?' type

Add the API xml document files to the generated NuGet packages

0b7e42d

daxian-dbw marked this pull request as ready for review February 1, 2021 19:13

daxian-dbw requested review from TravisEz13, adityapatwardhan and anmenaga as code owners February 1, 2021 19:13

daxian-dbw requested a review from rjmholt February 1, 2021 19:17

powercode reviewed Feb 1, 2021

View reviewed changes

anmenaga added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 4, 2021

anmenaga approved these changes Feb 4, 2021

View reviewed changes

anmenaga merged commit e803d3b into PowerShell:master Feb 4, 2021

daxian-dbw deleted the predict branch February 5, 2021 01:13

iSazonov added this to the 7.2.0-preview.3 milestone Feb 5, 2021

andrew-boyarshin mentioned this pull request Feb 17, 2021

PSReadLine doesn't work on 7.2.0-preview.3 PowerShell/PSReadLine#2221

Closed

Conversation

daxian-dbw commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

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!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Feb 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode Feb 4, 2021 • 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!

anmenaga left a comment

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!

daxian-dbw commented Feb 4, 2021 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daxian-dbw commented Jan 21, 2021 •

edited

Loading

powercode Feb 4, 2021 •

edited

Loading

daxian-dbw commented Feb 4, 2021 •

edited by unfurl-links bot

Loading