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.

Sign up for GitHub

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

Jump to bottom

Re-work package resolution and make runners use it #3852

Merged
merged 64 commits into from
May 25, 2023
Merged

Re-work package resolution and make runners use it #3852

merged 64 commits into from
May 25, 2023

Conversation

Michael-F-Bryan
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan commented May 9, 2023 •
edited
Loading

This PR refactors runners to take advantage of #3841 and #3831. @theduke and I have also completely re-worked the package resolution workflow to better suit Deploy's needs.

TL;DR:

  • The Runner trait now has two methods
    • an associated can_run_command() function for checking whether a runner can run something, and
    • a run_command() function which gets the command name, a WEBC container, and a WasiRuntime
  • All runtime-related state (task manager, module caching, stores, compilation, etc.) is now retrieved from the WasiRuntime instead of being stored as a field in the runner
  • The BinaryPackage type has been repurposed so it represents not just a single package, but the entire environment that will be available at runtime (all commands, file systems, etc.)
  • New concepts:
    • PackageLoader - something that can load a *.webc file
    • Source - something that can answer questions about package dependencies (WAPM, local directory for testing, etc.)
    • Registry - a collection of Sources
    • Summary - just enough metadata about a single package to do package resolution
    • resolve() - the top-level function that takes a "root" package and a Registry, and figures out both the concrete dependency graph (DependencyGraph) and what the resulting package would look like (ResolvedPackage)
    • load_package_tree() - a new method on WasiRuntime that you can override when loading ResolvedPackage into a BinaryPackage, primarily so Deploy can add custom caching and such

Later on we'll probably refactor BinaryPackageCommand so commands and filesystems can be lazily loaded, but I'm leaving that for future Michael.

Fixes #3603, fixes #3748, fixes #3812.

@Michael-F-Bryan Michael-F-Bryan changed the title Updated runners Make runners use PackageResolver and ModuleCache May 9, 2023
@Michael-F-Bryan Michael-F-Bryan changed the title Make runners use PackageResolver and ModuleCache Re-work package resolution and make runners use it May 15, 2023
@Michael-F-Bryan Michael-F-Bryan force-pushed the updated-runners branch 3 times, most recently from c6b70d8 to 3987ce9 Compare May 17, 2023 06:37
@Michael-F-Bryan Michael-F-Bryan marked this pull request as ready for review May 17, 2023 10:02
@theduke theduke mentioned this pull request May 17, 2023
Closed
theduke
theduke reviewed May 17, 2023
View reviewed changes
Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Overall: amazing work!

lib/wasi/src/runtime/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/package_loader/builtin_loader.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/package_loader/builtin_loader.rs Show resolved Hide resolved
lib/wasi/src/runtime/resolver/utils.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/resolver/source.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/module_cache/types.rs Show resolved Hide resolved
lib/wasi/src/runtime/resolver/inputs.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/resolver/source.rs Outdated Show resolved Hide resolved
theduke
theduke reviewed May 17, 2023
View reviewed changes
Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Overall: amazing work!

@Michael-F-Bryan Michael-F-Bryan mentioned this pull request May 17, 2023
Closed
theduke
theduke reviewed May 17, 2023
View reviewed changes
lib/wasi/src/runtime/mod.rs Show resolved Hide resolved
lib/wasi/src/runtime/package_loader/builtin_loader.rs Outdated Show resolved Hide resolved
lib/wasi/src/state/env.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/resolver/source.rs Outdated Show resolved Hide resolved
lib/wasi/src/runtime/resolver/inputs.rs Outdated Show resolved Hide resolved
@syrusakbary syrusakbary mentioned this pull request May 18, 2023
Closed
@theduke theduke mentioned this pull request May 19, 2023
Open
@Michael-F-Bryan Michael-F-Bryan enabled auto-merge May 19, 2023 10:50
syrusakbary
syrusakbary approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Preapproving it, we just need to simplify the trait implementation for now. But feel free to merge once that's implemented

This was referenced May 22, 2023
Closed
Closed
@Michael-F-Bryan Michael-F-Bryan force-pushed the updated-runners branch 2 times, most recently from 10c4cdd to f1bd711 Compare May 25, 2023 04:18
@theduke theduke force-pushed the updated-runners branch from f1bd711 to 23b999d Compare May 25, 2023 10:16
Michael-F-Bryan added 24 commits May 25, 2023 12:10
@theduke
Added some extra error messages to WebSource
c0f9211
@theduke
Renamed Summary to PackageSummary
cc65c61
@theduke
Implement a FileSystem source
f7c4658
@theduke
The run-unstable tests were using the wrong --registry
c287bdc
@theduke
Make sure packages from "--uses" are loaded into "/bin"
e9de05f
@theduke
An exit code of 0 is no longer an error
48a47a3
@theduke
Use a polyfill to make the JS tests pass
0acd680
@theduke
Remove the Registry trait
398f1d8
@theduke
Ran rustfmt
ed2bf8b
@theduke
Remove some references to the old Registry
426a095
@theduke
Use #[ignore] so snapshot tests can still be run on MacOS using --ign…
9581df7 
…ored
@theduke
Added a bunch of debugging annotations to the wasmer_wasix::fs module
09cec3e
@theduke
Add a special case for "some/package@latest"
bda7d07
@theduke
Make WapmSource handle queries where the package doesn't exist
4c11a07
@theduke
Removed some dbg!() statements from wasmer_wasix::fs so test would ke…
27f3056 
…ep passing
@theduke
The BuiltinPackageLoader::get_cached() method should log at debug level
fbd361d
@theduke
Emit a warning when we hit the WasiFsRoot::Backing TODO
a890697
@theduke
Fix http_get's logic to also apply timeouts to reading body
b8c578c
@theduke
Add a sanity check for duplicate package versions
40791c9
@theduke
Log the dependency graph
e109e43
@theduke
Temporarily skip test_snapshot_web_server
11836c3
@theduke
Fixed some compile errors after a rebase
52048f3
@theduke
Add extra logging to package resolution
79e6149
@theduke
Made the failing tests go away
30386f5
@theduke theduke force-pushed the updated-runners branch from 9cae74e to 30386f5 Compare May 25, 2023 12:10
@syrusakbary syrusakbary disabled auto-merge May 25, 2023 13:23
@syrusakbary syrusakbary merged commit e89b313 into master May 25, 2023
@syrusakbary syrusakbary deleted the updated-runners branch May 25, 2023 13:23
@Michael-F-Bryan Michael-F-Bryan mentioned this pull request May 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theduke theduke theduke approved these changes

@syrusakbary syrusakbary syrusakbary approved these changes

@john-sharratt john-sharratt Awaiting requested review from john-sharratt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Michael-F-Bryan @syrusakbary @theduke @ptitSeb