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

Symbol caches: Larger download batching. Don't ask for non-General pkgs #263

Merged

Conversation

IanButterworth
Copy link
Contributor

@IanButterworth IanButterworth commented Jan 27, 2023

Given non-General packages don't have symbol caches, don't request them, to avoid requesting URLs that include the UUID & name of private packages.

Also the download batching didn't make sense to me. It seems better to limit the concurrent number of downloads, rather than the total number of batches. Update: Now batches of 100

Fixes #.

For every PR, please check the following:

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.

Cool, thanks! Left a few comments.

src/SymbolServer.jl Outdated Show resolved Hide resolved
src/SymbolServer.jl Show resolved Hide resolved
src/SymbolServer.jl Outdated Show resolved Hide resolved
@IanButterworth IanButterworth changed the title Symbol caches: Download 10 at a time rather than 50 download batches. Don't ask for non-General pkgs Symbol caches: Remove download batching. Don't ask for non-General pkgs Jan 27, 2023
src/SymbolServer.jl Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Contributor Author

IanButterworth commented Jan 27, 2023

This has proven quite painful to get working on older julia versions. I went as far as 1.1, and for 1.0 rely on a try-catch to default to just generated caches locally. (Perhaps someone else can see a fix for 1.0 where PackageEntry isn't defined?)

@IanButterworth IanButterworth requested review from davidanthoff and fredrikekre and removed request for davidanthoff January 27, 2023 21:02
@IanButterworth IanButterworth force-pushed the ib/cache_download_tweaks branch 2 times, most recently from 4ee8a56 to d98c479 Compare January 27, 2023 21:05
@IanButterworth
Copy link
Contributor Author

@davidanthoff it would be great, if you approve, to get this trialed on the pre-release

src/SymbolServer.jl Outdated Show resolved Hide resolved
src/SymbolServer.jl Outdated Show resolved Hide resolved
src/SymbolServer.jl Outdated Show resolved Hide resolved
@IanButterworth IanButterworth changed the title Symbol caches: Remove download batching. Don't ask for non-General pkgs Symbol caches: Larger download batching. Don't ask for non-General pkgs Feb 1, 2023
Comment on lines 32 to 35
regs = Pkg.Types.Context().registries
i = findfirst(r -> r.name == "General" && r.uuid == UUID("23338594-aafe-5451-b93e-139f81909106"), regs)
i === nothing && return Dict{UUID, PkgEntry}()
return regs[i].pkgs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't working in the vscode extension because Pkg.Types.Context().registries is empty

┌ Info: regs
└   regs = Pkg.Registry.RegistryInstance[]
┌ Error: Symbol cache downloading: Failed to identify which packages to omit based on the General registry.
│ All packages will be processsed locally
│   err = Could not find the General registry
└ @ SymbolServer ~/Documents/GitHub/julia-vscode/scripts/packages/SymbolServer/src/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the right way to get the General registry from here?

Copy link
Member

Choose a reason for hiding this comment

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

That's kinda tricky. We're overwriting setting the JULIA_DEPOT_PATH env variable to a depot vendored in the extension, but you could probably temporarily change the DEPOT_PATH before this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I left this behind. I think it would be great to get this finished up. Does that sound like a good plan @davidanthoff ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I've tried to implement that. I'd appreciate trying to get this merged to stop sending out non-General package name & uuids

@pfitzseb
Copy link
Member

pfitzseb commented Aug 28, 2023

Will review/merge this tomorrow. Feel free to ping me somewhere if I don't :)

@IanButterworth
Copy link
Contributor Author

Will do, thanks

@IanButterworth
Copy link
Contributor Author

Bump 🙏

@pfitzseb pfitzseb merged commit 329bd6e into julia-vscode:master Sep 4, 2023
51 of 52 checks passed
@IanButterworth IanButterworth deleted the ib/cache_download_tweaks branch September 4, 2023 11:11
@pfitzseb
Copy link
Member

pfitzseb commented Sep 4, 2023

Thanks for the PR! I'll make a few follow-up tweaks, but none of that should hold this up any further :)

@IanButterworth
Copy link
Contributor Author

Just checking, is this used on the extension now? I wasn't sure if the extension uses a non-tagged commit

@pfitzseb
Copy link
Member

pfitzseb commented Nov 6, 2023

It is. You can check here which commit the extension uses.

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.

None yet

5 participants