Skip cloud files marked as not on disk during command discovery #18152

SeeminglyScience · 2022-09-23T20:29:14Z

PR Summary

These changes make it so that command discovery (and subsequently tab completion) no longer trigger the download of files not yet on disk when cloud storage is present in env:PSModulePath (e.g. backing up Documents with OneDrive).

I believe this would technically be a breaking change, ~~though I'm unsure if we would need to put this behind an experimental change as the experience is pretty rough if you hit this currently. Happy to add that on request though.~~ Experimental feature added as PSModuleAutoLoadSkipOfflineFiles

PR Context

PR Checklist

SeeminglyScience · 2022-09-23T20:30:10Z

src/System.Management.Automation/engine/Modules/ModuleUtils.cs

+        // added to System.IO.FileAttributes yet.
+        private const int FILE_ATTRIBUTE_RECALL_ON_DATA_ACCESS = 0x400000;
+
+        private const int FILE_ATTRIBUTE_RECALL_ON_OPEN = 0x40000;


I have not actually been able to produce a scenario where this attribute appears (despite checking from multiple win32 APIs in case it only appeared in certain structs). I kept it in incase there's a different cloud provider that uses it, or if OneDrive starts to use it at some point.

Documentation for FILE_ATTRIBUTE_RECALL_ON_OPEN reads:

This attribute only appears in directory enumeration classes (FILE_DIRECTORY_INFORMATION, FILE_BOTH_DIR_INFORMATION, etc.). When this attribute is set, it means that the file or directory has no physical representation on the local system; the item is virtual. Opening the item will be more expensive than normal, e.g. it will cause at least some of it to be fetched from a remote store.

If we can't even check it manually and even OneDrive doesn't use it, why is it PR?

I kept it in incase there's a different cloud provider that uses it, or if OneDrive starts to use it at some point. Basically, documentation tell us to look for it, so I lean towards looking for it. Even if it happens not to be present in the specific use case we're looking to solve.

You won't be the first anyway. 😸
https://github.com/search?q=org%3Adotnet+FILE_ATTRIBUTE_RECALL_ON_OPEN&type=issues

iSazonov · 2022-09-24T05:07:42Z

This seems too uncompromising a decision. If we go this way, we rather need an option. Then the main difficulty will be to decide whether it should be opt-in or opt-out.

SeeminglyScience · 2022-09-26T15:06:05Z

This seems too uncompromising a decision. If we go this way, we rather need an option. Then the main difficulty will be to decide whether it should be opt-in or opt-out.

With this change you already have the option to simply pin the modules folder. This change only excludes files that have not already been downloaded by OneDrive. The experience of module discovery attempting to import modules that have not yet been downloaded is quite bad (tab completion will wait until every module has been downloaded before returning).

iSazonov · 2022-09-26T16:18:56Z

I still think it is not PowerShell problem. What if about TwoDrive? 😄 I believe One Drive must have folder/file exclusion option.

SeeminglyScience · 2022-09-26T17:01:12Z

I still think it is not PowerShell problem. What if about TwoDrive? 😄 I believe One Drive must have folder/file exclusion option.

I would guess most cloud providers have a way to say "I want this folder to always be available". Or at least a way to trigger the download. The changes in this PR are not actually OneDrive specific either, FYI. Any cloud provider can decorate their files with the same attributes and we'll skip their not-on-disk files as well.

SteveL-MSFT · 2022-09-26T19:50:14Z

@iSazonov FYI - We've had conversations with OneDrive team and it doesn't seem they will do anything specific for us. We've heard from many customers where they weren't aware MyDocuments is now in OneDrive and complain about PS perf. Perhaps we should put this as an experimental feature as a way to advertise the change and get feedback.

iSazonov · 2022-09-27T04:09:43Z

they will do anything specific for us.

If I were them, I would also answer because the question is unfortunate - too small a niche.
In my opinion, the request should be this - we need to allow OneDrive users the flexibility to configure exactly what they want to sync now. For example, I don't want to sync large files or video files and some folders, but I want to sync Excel and Word files.
This feature will also allow users to adjust the synchronization of folders with PowerShell modules.
It is my preffered solution.

If PowerShell does use a workaround, however, it could disrupt the familiar user environment. For example, I used to actively sync modules via OneDrive for a while. At the very least, we need an option to explicitly enable or disable it. I'd start with opt-in.

SeeminglyScience · 2022-09-27T18:18:09Z

Added experimental feature PSModuleAutoLoadSkipOfflineFiles

SteveL-MSFT · 2022-10-03T16:51:20Z

@iSazonov the longer term solution for PS7 is to remove PSModulePath from including MyDocuments, it should be under AppData (see #15552). This is an interim fix addressing real world feedback.

iSazonov · 2022-10-03T17:19:49Z

@iSazonov the longer term solution for PS7 is to remove PSModulePath from including MyDocuments, it should be under AppData (see #15552). This is an interim fix addressing real world feedback.

Both of these decisions are questionable. If there are users who are bothered by it, then there are also users who use it.
Note that the problem didn't exist until OneDrive came along. This clearly suggests that it is the OneDrive setup that should give users control over the situation as they need it.
Otherwise you get a strange situation - some developers create cloud sync, others block it. It's more like a rivalry of teams.
These attributes are more for temporary and auxiliary files that should never be synced, while modules may very well be, especially if the user prefers to sync his profile.ps1.

ghost · 2022-10-11T02:01:21Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

SeeminglyScience · 2022-10-11T19:13:44Z

These attributes are more for temporary and auxiliary files that should never be synced, while modules may very well be, especially if the user prefers to sync his profile.ps1.

The attributes are for informing anything that does file based discovery that retrieving a particular file's content would be expensive. Pinned is another attribute that lets the user tell the cloud provider that a specific folder (such as the Modules folder) should always be synced. That's an easy way to configure this already built in, and not specific to OneDrive (though I personally haven't checked other cloud providers).

daxian-dbw

LGTM.

daxian-dbw

@SeeminglyScience Please file a doc issue about this new behavior.
Also, please resolve the conflicts.

PS. I looked up the history and found the experimental-feature-xxx.json change was made by #16823, as a temporary workaround for cross compiled packages like ARM and Alpine. I think for CIs, we should continue to invoke pwsh to generate the list of experimental features, and only use those 2 files for release build. Those 2 files will be updated daily by GitHub actions, so it should be fine for release builds.

pull-request-quantifier-deprecated · 2022-11-07T20:06:09Z

This PR has 32 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +30 -2
Percentile : 12.8%

Total files changed: 4

Change summary by file extension:
.json : +2 -0
.cs : +28 -2

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw · 2022-11-08T00:44:45Z

@SeeminglyScience FYI, PR #18484 was submitted to make those 2 JSON file build artifacts only.

Please open doc issue and update the PR check list with the doc issue link.

SeeminglyScience · 2022-11-08T19:40:16Z

Yep! I'll be filing an issue in the docs repo. I need to fix something though, it seems like disabling the experimental feature isn't actually reverting behavior (though I could have sworn it was previously).

Update: Disable-ExperimentalFeature doesn't seem to be updating the file... unsure if that's environmental or a new bug with the command.

Manually removed it and it's working, so unrelated. I'll get that issue filed for docs and separately figure out what's up with Disable-

Update 2: And now I can't even repro Disable-ExperimentalFeature not working, so I'm at a loss there.

SeeminglyScience · 2022-11-08T20:12:34Z

@daxian-dbw docs issue has been filed and PR description has been updated

daxian-dbw · 2022-11-08T22:35:53Z

@SeeminglyScience Thanks! I will merge this PR.

Skip files marked as offline or not fully on disk

1a5b291

pull-request-quantifier-deprecated bot added the Extra Small label Sep 23, 2022

ghost assigned adityapatwardhan Sep 23, 2022

SeeminglyScience commented Sep 23, 2022

View reviewed changes

Wrap changes in an experimental feature flag

b9081fe

SteveL-MSFT approved these changes Oct 3, 2022

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Oct 11, 2022

Merge branch 'master' into skip-cloud-command-discovery

7208e04

daxian-dbw approved these changes Nov 7, 2022

View reviewed changes

daxian-dbw assigned daxian-dbw and unassigned adityapatwardhan Nov 7, 2022

ghost removed the Review - Needed The PR is being reviewed label Nov 7, 2022

daxian-dbw requested changes Nov 7, 2022

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 7, 2022

Merge branch 'master' into skip-cloud-command-discovery

2f83e04

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 7, 2022

SeeminglyScience mentioned this pull request Nov 8, 2022

Module discovery will skip files not marked as fully on disk MicrosoftDocs/PowerShell-Docs#9417

Closed

2 tasks

daxian-dbw approved these changes Nov 8, 2022

View reviewed changes

daxian-dbw merged commit 74aa257 into PowerShell:master Nov 8, 2022

adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 20, 2022

Skip cloud files marked as not on disk during command discovery #18152

Skip cloud files marked as not on disk during command discovery #18152

Uh oh!

Conversation

SeeminglyScience commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

SeeminglyScience Sep 23, 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!

iSazonov Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 24, 2022

Uh oh!

SeeminglyScience commented Sep 26, 2022

Uh oh!

iSazonov commented Sep 26, 2022

Uh oh!

SeeminglyScience commented Sep 26, 2022

Uh oh!

SteveL-MSFT commented Sep 26, 2022

Uh oh!

iSazonov commented Sep 27, 2022

Uh oh!

SeeminglyScience commented Sep 27, 2022

Uh oh!

SteveL-MSFT commented Oct 3, 2022

Uh oh!

iSazonov commented Oct 3, 2022

Uh oh!

ghost commented Oct 11, 2022

Uh oh!

SeeminglyScience commented Oct 11, 2022

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment • 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!

pull-request-quantifier-deprecated bot commented Nov 7, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

daxian-dbw commented Nov 8, 2022

Uh oh!

SeeminglyScience commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeeminglyScience commented Nov 8, 2022

Uh oh!

daxian-dbw commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SeeminglyScience commented Sep 23, 2022 •

edited

Loading

SeeminglyScience Sep 23, 2022 •

edited

Loading

daxian-dbw left a comment •

edited

Loading

SeeminglyScience commented Nov 8, 2022 •

edited

Loading