Skip to content
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

Retrieve remote caches #204

Merged
merged 17 commits into from
Jan 22, 2021
Merged

Retrieve remote caches #204

merged 17 commits into from
Jan 22, 2021

Conversation

ZacLN
Copy link
Contributor

@ZacLN ZacLN commented Jan 13, 2021

A first draft of how this may look on the client side- thoughts?

  • Change cache names (incl folder structure) to match web store.
  • Async downloading
  • Move cloud check to caller side (i.e. LS)
  • Ship stdlib caches?

@ZacLN ZacLN mentioned this pull request Jan 13, 2021
@davidanthoff
Copy link
Member

I think that probably will work for starters, but I think medium term we could:

  • have the whole download story happen in the LS process, right? I think this PR downloads in the symbol server process, right? It would be nice if a user has an environment where everything is available in the cloud if we then didn't even have to start the symbol server process at all but just do everything in the LS process instead.
  • I think we should download in parallel, so essentially first compile a list of all the files we need to download, and then download inside a call to asyncmap with some reasonable ntask limit. My best guess is that things would be a ton faster if we don't serialize the downloads of the different cache files.

@davidanthoff
Copy link
Member

We should now use this code to download:

Pkg.PlatformEngines.download_verify_unpack("https://symbolcache.julia-vscode.org/store/v1/packages/A/AndExport_4484d908-372d-4f90-b90b-0ec97f005b8c/v1.0.1_ebc539a19124dd3f67087d50376adc010c577536.tar.gz", nothing, folderpath_where_you_want_the_extracted_file)

That is also the current URL format.

@davidanthoff
Copy link
Member

New format for the URLs for now:

https://www.julia-vscode.org/symbolcache/store/v1/packages/D/DataFrames_a93c6f00-e57d-5684-b7b6-d8193f3e46c0/v0.22.1_20159837c2e5e196793a313cd700b8199fd8f985.tar.gz

@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 15, 2021

  • have the whole download story happen in the LS process, right? I think this PR downloads in the symbol server process, right? It would be nice if a user has an environment where everything is available in the cloud if we then didn't even have to start the symbol server process at all but just do everything in the LS process instead.

I think ideally, yes, but we may run into issues trying to interogate where packages are installed locallly without activating the target environment. This will be needed when modifying the downloaded caches to point to the correct local paths.

@davidanthoff
Copy link
Member

Another question: should we store cloud retrieved cache files and locally generated cache files in different locations, just so that we can later distinguish where they came from? I think in the end we probably want to look in three different locations always no matter what: 1) in a read-only folder in the extension itself (for caches that we ship out of the box), 2) in a location for cloud downloads, and 3) in a location for locally generated caches.

@pfitzseb
Copy link
Member

I think we need an option to disable cloud downloads somewhere -- maybe just check an environment variable (JULIA_LANGUAGESERVER_SYMBOL_DOWNLOAD?) in get_file_from_cloud?

@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 19, 2021

Do we want it as an ENV rather than just an argument passed to getstore (or SymbolServerInstance) ?

@pfitzseb
Copy link
Member

An argument is better, but needs to be piped through all the way from the LanguageServerInstance constructor, which is kinda annoying.

@ZacLN
Copy link
Contributor Author

ZacLN commented Jan 19, 2021

Yeah that's slightly more annoying but seems the best way to me, unless there other good reasons not to

@davidanthoff
Copy link
Member

Agreed that an argument is nicer :) Yes, more work, but I think there is also just value in handling all config the same way.

Copy link
Member

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't review carefully, but lets ship to insider and see whether it works.

@ZacLN ZacLN merged commit 3914d59 into master Jan 22, 2021
@ZacLN ZacLN deleted the cloud branch January 22, 2021 21:00
@oppo-source oppo-source removed the request for review from a team April 16, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants