-
Notifications
You must be signed in to change notification settings - Fork 122
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
Support Experimental Features in PowerShell #114
Conversation
We went through and discussed all comments in this PR. The discussion has been incorporated into the RFC.
'Get-ExperimentalFeature' returns the available 'ExperimentalFeature' instances. For the experimental features that are enabled, the property 'IsEnabled' should be set to true accordingly.
``` | ||
|
||
When adding a new experimental feature, an instance representing the feature needs to be added to the collection for it to be discoverable to users. | ||
The `Source` for engine experimental features should be specified as `PSEngine`. |
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.
Is this always true? If so then why do we need a settable 'Source' property?
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.
Modules can expose experimental features, and for those, the 'Source' would be the modules names. Therefore, the 'Source' property needs to be settable.
|
||
#### Get-ExperimentalFeature | ||
|
||
A new cmdlet `Get-ExperimentalFeature` will be added to return all available experimental features of a PowerShell session. |
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.
What do you mean by 'available'? Does this include 'hidden' experimental features? Should there be an option to show 'hidden' experimental features?
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.
Here "available" means all experimental features, enabled or not enabled. The returned ExperimentalFeature
type has a IsEnabled
property, so you will know which ones are enabled/not enabled.
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.
@PaulHigin - experimental implies that people should experiment with these features hence the need for discoverability. What would be the purpose of a "hidden" experimental feature? (And since it's open source, nothing is really hidden)
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.
I'd expect Enable-ExperimentalFeature
and Disable-ExperimentalFeature
to change config file.
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.
@BrucePay: There is no concept of a hidden experimental feature. In the referenced statement, available could be removed.
For each of them, the `Name` and `Description` are from the metadata. The `Source` will be the module name. | ||
|
||
Lastly, the cmdlet can go through not-yet-loaded modules that are in the module path, | ||
in a similar way as `Get-Command` does, |
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.
Would it make more sense to have 'Get-Command -Experimental' ?
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.
When an experimental feature is not enabled, the cmdlets or functions subject to this feature is ignored in the command registration phase, so it doesn't make sense to have Get-Command -Experimental
.
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.
Yes, but even if it is not enabled I might be interested to learn what experimental features are possible if I opt in. I assume Get-ExperimentalFeature will show 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.
Yes, please see the type definition of ExperimentalFeature
at line 357. The type ExperimentalFeature
has a property Description
that will introduce the experimental feature.
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.
@PaulHigin To be clear, Get-Experimental feature enumerates the experimental features but the return type does not explicitly indicate what cmdlets, parameters, or code is impacted by the feature. In short, we're relying on the feature author to provide an appropriate description.
however, PowerShell loads the enabled experimental feature list at very early stage, | ||
where the engine is not ready for the operations required for resolving dependencies, | ||
such as module discovery. | ||
Also, there will be an impact to the startup time if we do this resolution when loading the configuration file. |
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.
Since Experimental features are non-GA it seems like a modest start-up impact is not too bad. With the extra module processing it seems likely that command discovery will be slower in general too. I think we would want to ensure the module analysis cache file is updated to include experimental information.
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.
Slowdown startup is not acceptable for GA.
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.
That is what I said. So for non-GA a modest impact should be Ok.
The Experimental Feature support in PowerShell is to provide a mechanism for experimental features to coexist | ||
with existing stable features in PowerShell or PowerShell modules. | ||
An experimental feature would not be enabled | ||
unless a user opt in by properly configuring the `powershell.config.json` file. |
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.
Should we get previously RFC for powershell.config.json
and the configuration API?
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.
@iSazonov: Would you rephrase the question? I don't know what you're asking.
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.
I believe @iSazonov is talking about another RFC PowerShell Core configuration.
The implementation of this RFC will inevitably touch PSConfiguration.cs
to add APIs for reading the experimental feature settings.
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.
Yes, I believe the RFC depends on PowerShell Core configuration
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.
I wouldn't say it depends on that RFC. The configuration RFC discusses how the configuration settings should be organized, and if approved, the implementation is done separately anyways.
with existing stable features in PowerShell or PowerShell modules. | ||
An experimental feature would not be enabled | ||
unless a user opt in by properly configuring the `powershell.config.json` file. | ||
Experimental features should not affect any stable features |
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.
What if new experimental feature is for replacing current feaure? New file system provider? I see tha sample below so my question is about the phrase only.
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 FileSystemProviderV2
would replace the existing FileSystemProvider
only if that experimental feature is enabled. When it's not enabled, it should not affect the existing FileSystemProvider
.
During the development, | ||
the `FileSystemProviderV2` can coexist with the existing one | ||
and be exposed to the user as an experimental feature, | ||
so that PowerShell team can get early feedback and issue reports. |
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.
I think it would be very useful to see activated experimental features in PSVersionTable
so we could see this in Issues.
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.
I'm inclined to not adding this information to PSVersionTable
yet. The new Get-ExperimentalFeature
cmdlet could have switches -Enabled
and -Disabled
though.
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.
RFC updated to add -Enabled
and -Disabled
switches.
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.
Without this we have to ask, for ex., in every file system Issue - Did you enable FileSystemProviderV2?
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 can add another environment section in the issue template called Get-ExperimentalFeatures -Enabled
.
If there is really a demand, we can add information about experimental features to $PSVertionTable
as an enhancement, but I don't think it should be added to the RFC yet.
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.
Rather than modify $PSVersionTable or depend on a this command specific to this feature, I think it may be beneficial to consider a an RFC for a generalized PowerShell support cmdlet that provides common environment details asked for by the PowerShell project and many other open source projects for PowerShell. It would be convenient for us and the community. I envision it as a kind of phpinfo()
for PowerShell. Adding the command to the template is a good stop-gap though and if anyone else sees value in such a command for PowerShell I will consider an RFC.
|
||
### Experimental Feature Names | ||
|
||
Experimental features for PowerShell components and built-in modules should be named as `PSXXX`, |
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.
I'd expect EF
or EXP
or EXPS
.
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.
PowerShell team owns the prefix PS
, and there have been PowerShell built-in modules whose names follow this pattern, like PSDesiredStateConfiguration
, PSDiagnostics
, PSScheduledJob
and etc. So I think we should use PS
here.
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.
I am thinking about easy searching in code sources - what about PSEX
or PSEXP
?
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.
I'm open to PSEXP
.
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.
Had a second thought about the naming. I want to avoid using a string of feature name for existence test in engine code, like if (context.EnabledExpFeatures.Contains("PSEXPBlahFeature"))
. Directly using a string for checks like this will impact readability.
My original design is to have an internal enum that contains the engine experimental features, and the description of each feature goes in a resource file called ExperimentalFeatureDescription.resx
. When initializing the engine, it constructs a ReadOnlyCollection<ExperimentalFeature>
out of the enum -- the name of an enum member is the experimental feature's name; read the description from the embedded resource. So when referencing an engine experimental feature in the engine code, you have to use the enum. It would serve the readability very well and also reduce errors like spelling a feature name incorrectly when directly using a string.
But I got feedback that
- separating the description to a resource file is heavy given experimental features might not live long and we don't do resource string localization anyways;
- having name and description together in the code is more readable.
So I gave up the original design and turn to having an exp-feature author to manually add an instance for the read-only collection when adding a new exp-feature.
Now I again tend to have the enum, or a set of const
string variables representing each exp-feature, so that we don't directly use literal strings when referencing a exp-feature in the engine.
For example: | ||
|
||
```none | ||
ModuleName.ExperimentName |
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.
Does ExperimentName
contains prefix like PSXXX
?
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.
I don't think there's any reason to. The PSXXX are for first-party experiments.
however, PowerShell loads the enabled experimental feature list at very early stage, | ||
where the engine is not ready for the operations required for resolving dependencies, | ||
such as module discovery. | ||
Also, there will be an impact to the startup time if we do this resolution when loading the configuration file. |
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.
Slowdown startup is not acceptable for GA.
|
||
A new command-line flag `-settingsFile` was added to `pwsh` to allow | ||
a user to override the instance configuration file `$PSHOME\powershell.config.json` using another configuration file. | ||
This flag is very useful in testing the experimental features. |
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.
For local testing it would be convenient to have pwsh -Experimental Feature1,Feature2
.
For CI testing it would be convenient to have pwsh -Experimental EnableAll
.
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.
I don't think EnableAll is viable for a few reasons. The primary reasons being
- Not all experimental features are compatible
- Some experiments are, by design, mutually exclusive.
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 processing of command line parameters doesn't happen early enough (we pointed it out in "Open Issues" section when discussing the problems with -settingsFile
). So pwsh -Experimental
would have the same problem.
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 seems we could fix 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.
Refactoring the command-line parsing is not covered in this RFC. It should be a separate issue.
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 need to have test pass for every experimental feature introduced in PowerShell built-in components regularly. | ||
Code changes that are not part of an experimental feature work may cause regression to the experimental feature. | ||
If we don't have test pass for every built-in experimental feature, | ||
the experimental features may be broken without us knowing it. |
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.
Experimental feature can be a breaking change - so we'll be not only add new tests but we'll have to skip some regular tests.
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.
Excellent point and one we hadn't considered; disabling a test when an experiment is enabled.
|
||
The first proposal will cost way more than the second proposal. | ||
Since the configuration file overriding functionality is mainly for testing, | ||
the second proposal may be more efficient cost-wise. |
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.
I believe it makes sense to split on early parsing and later appling of command-line parameters.
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.
I tend to agree but it may not be justified here.
### Whether to have telemetry for experimental feature usage | ||
|
||
It may be good to have telemetry for the usage of experimental features. | ||
This work may fall into the more general telemetry support in PowerShell. |
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 should contain what features enabled.
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.
With the caveat that this section is no more of a placeholder...
I suspect a usage metric would be more desirable (or both). The fact that an experiment is enabled doesn't mean it's was used.
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.
Authors definitely need some way to get feedback about what features are enabled and how they are being used otherwise what's the point of the experiment. Perhaps we could do extra usage telemetry when experimental features are turned on.
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.
Hmm - maybe we should provide cmdlets to allow the users to provide explicit feedback e.g. Submit-ExperimentalFeatureFeedBack -Feature abc -Vote 5 -Comment "Eh - it's ok"
.
2. Validating whether references to an non-existing experimental feature have been removed. | ||
At some point, the experiment is completed and either removed from or incorporated into the code base. | ||
At this point, a tool to scan script or compiled code would be needed to ensure any references to the experiment have been removed. | ||
|
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.
On the static analysis front, I would be in favor of any design choices (like maybe the enum rather than prefixed strings) that would make it easier to run a tool over code. Of course nothing is future proof, but it would be nice to avoid a "Windows 9"
kind of situation.
Add support to experimental features RFC: PowerShell/PowerShell-RFC#114 Goals: Allow experimental features to be declared by PowerShell engine and modules. Allow experimental features to be enabled via powershell.config.json Allow Function, Cmdlet, parameters and parameter sets to be shown to the user or hiden from the user depending on whether the associated experimetnal feature is on or off. Allow discover experimental features using cmdlet Get-ExperimentalFeature
…res.md to 4-Experimental-Accepted/RFC0029-Support-Experimental-Features.md
This RFC is for adding support to Experimental Features in PowerShell.
/cc @dantraMSFT