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

Add fast generator for cog build #2108

Merged
merged 21 commits into from
Jan 15, 2025
Merged

Add fast generator for cog build #2108

merged 21 commits into from
Jan 15, 2025

Conversation

8W9aG
Copy link
Contributor

@8W9aG 8W9aG commented Jan 10, 2025 •
edited
Loading

  • When the x-fast flag is enabled, switch the generator to use the FastGenerator instead of the StandardGenerator
  • This loads from monobase, then performs the following steps:
  1. Loads the monobase generation, it uses a userspace cache for the monobase to share generation hydrations across projects.
  2. Copies the weights (if found) via hardlinking (so no copying big files is performed). This performs a digest on the weights with SHA256 but then records the modified time and size of the file, if the file has the same modified time and size in a subsequent build it does not recalculate the SHA256.
  3. Installs the apt-get packages in a layer.
  4. Installs the python packages in a layer through monobase (to de-duplicate packages from the base).
  5. Copies the source files without the weights (again using hardlinking).
  6. Symlinks the weights to their position in the r8 srv directory.

@8W9aG 8W9aG requested a review from a team January 10, 2025 20:04
@8W9aG 8W9aG marked this pull request as draft January 10, 2025 20:05
nevillelyh
nevillelyh reviewed Jan 10, 2025
View reviewed changes
pkg/dockerfile/fast_generator.go Outdated Show resolved Hide resolved
pkg/dockerfile/fast_generator.go Outdated Show resolved Hide resolved
@nevillelyh
Copy link
Contributor

nevillelyh commented Jan 10, 2025 •
edited
Loading

  1. Do not copy over weights in the source copy, instead make sure they are symlinked to the weights in the copy weights section

You mean in the weigths layer that we do NOT upload, we have symlink /src/weights.pth -> /srv/r8/fuse-rpc/<sha256>? That way we ensure user code works with symlink but it leaks a little impl detail, but probably fine 🤷

@8W9aG 8W9aG marked this pull request as ready for review January 14, 2025 21:19
@8W9aG 8W9aG requested review from meatballhat, a team and nevillelyh January 14, 2025 21:20
@8W9aG
Copy link
Contributor Author

8W9aG commented Jan 14, 2025

I intend to write more unit tests before I declare this complete, but the basic structure is ready for review.

nevillelyh
nevillelyh reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@nevillelyh nevillelyh left a comment

Choose a reason for hiding this comment

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

Comments to sync up with replicate/monobase#63

pkg/dockerfile/fast_generator.go Outdated Show resolved Hide resolved
pkg/dockerfile/fast_generator.go Outdated Show resolved Hide resolved
pkg/dockerfile/fast_generator.go Show resolved Hide resolved
pkg/dockerfile/fast_generator.go Outdated Show resolved Hide resolved
nevillelyh
nevillelyh reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@nevillelyh nevillelyh left a comment

Choose a reason for hiding this comment

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

A few more minor comments.
Once all converged, the monobase vs user venv layers should be cleanly separate.

pkg/dockerfile/fast_generator.go Show resolved Hide resolved
pkg/dockerfile/fast_generator.go Outdated Show resolved Hide resolved
@8W9aG 8W9aG requested a review from nevillelyh January 15, 2025 20:14
nevillelyh
nevillelyh reviewed Jan 15, 2025
View reviewed changes
pkg/config/requirements.go

// SplitPinnedPythonRequirement returns the name, version, findLinks, and extraIndexURLs from a requirements.txt line
// in the form name==version [--find-links=<findLink>] [-f <findLink>] [--extra-index-url=<extraIndexURL>]
func SplitPinnedPythonRequirement(requirement string) (name string, version string, findLinks []string, extraIndexURLs []string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized this. Do we still need this, now that monobase.user dedups the requirements and handles Torch index anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh we need to perform a dedup ourselves on the cog.yaml, it offers both python_packages and python_requirements as an option.

@8W9aG 8W9aG requested a review from nevillelyh January 15, 2025 20:33
@8W9aG 8W9aG merged commit 9efb306 into main Jan 15, 2025
21 checks passed
@8W9aG 8W9aG deleted the add-fast-generator branch January 15, 2025 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nevillelyh nevillelyh nevillelyh approved these changes

@meatballhat meatballhat Awaiting requested review from meatballhat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@8W9aG @nevillelyh