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
Update the ICommandPredictor
to provide more feedback and also make feedback easier to be corelated
#14649
Conversation
ICommandPredictor
to provide more feedback and also make feedback easier to be corelated
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); |
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.
We could merge both methods if they are always used together.
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.
They are not used together. OnSuggestionDisplayed
will be fired way more than OnSuggestionAccepted
.
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.
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.
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.
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.
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.
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.
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.
This feels like a design proposal for a separate feature "restore a PowerShell session after a restart" :)
/// <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; } |
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.
Why uint? Maybe Guid to be globally independent?
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.
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.
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.
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
@@ -2312,6 +2312,14 @@ internal static void NotNullOrEmpty(string value, string paramName) | |||
} | |||
} | |||
|
|||
internal static void NotNullOrEmpty(ICollection value, string paramName) |
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.
Perhaps use [CallerArgumentExpression("value")]
?
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.
It's not implemented yet, right? See the proposal doc and the open issue: dotnet/csharplang#287
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.
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.
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.
Looks fine.
Please file a doc issue and link it in "Documentation needed"/"Issue filed:" in PR Checklist; thank you.
@@ -2268,6 +2276,9 @@ function New-ReferenceAssembly | |||
Copy-Item $refBinPath $RefAssemblyDestinationPath -Force | |||
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" |
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.
... make NuGet packages include the XML document files.
This is not really related to prediction APIs. Could have been a separate PR.
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.
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.
@{ | ||
ApplyTo = @("System.Management.Automation") | ||
Pattern = "[System.Runtime.CompilerServices.CompilerGeneratedAttribute, System.Runtime.CompilerServices.IsReadOnlyAttribute]" | ||
Replacement = "/* [System.Runtime.CompilerServices.CompilerGeneratedAttribute, System.Runtime.CompilerServices.IsReadOnlyAttribute] */ " |
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.
Can you please explain why this is needed?
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.
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.
@anmenaga Thanks for the review!
|
🎉 Handy links: |
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);
countOrIndex
parameter can help a predictor to tell if the display was inListView
orInlineView
:> 0
, it's the number of displayed suggestions from the list returned in , starting from the index 0. This indicates theListView
is in use.<= 0
, it means a single suggestion from the list got displayed, and the index is the absolute value. This indicates theInlineView
is in use.Add
clientId
andsession
parameters to the existing interface APIs. TheclientId
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); thesession
parameter helps a predictor to corelate feedback events.GetSuggestion
andStartEarlyProcessing
both take astring
parameterclientId
, so it knows the client that makes the callGetSuggestion
returns auint
typesession
value, along with the list of suggestion entries it provides.OnSuggestionDisplayed
andOnSuggestionAccepted
both take auint
parametersession
, 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
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.PSSubsystemPluginModel