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

perf: don't unneccessarily rebuild dev assets #39

Draft
wants to merge 21 commits into
base: nightly
Choose a base branch
from

Conversation

kdmccormick
Copy link
Owner

@kdmccormick kdmccormick commented May 31, 2024

Background

Currently, Tutor Dev users who mount edx-platform end up needing to install node_modules and build static assets three separate times!

  1. in the openedx (prod) image build, which openedx-dev is based on
  2. in the openedx-dev image build
  3. in mounted-directories.sh

In addition, devs are forced to collect assets and pull translations as part of step 1, which is slow and not necessary for dev mode.

Even though the artifacts from steps 1-2 are totally ignored when bind-mounting edx-platform, those steps are re-executed every time any part of the image is to be rebuilt, e.g. adding/removing dev python packages. This is a huge time sink for edx-platform devs and package devs.

This PR

This PR makes a few critical changes:

  • Split each type of asset compilation (js/css X dev/prod) into its own minimal stage, eliminating dev-prod redundancy and avoiding spurious asset rebuilds.
  • Pull translations and collect assets for prod, but not for dev.
  • Generate assets (js, css, and node_modules) into the openedx image outside the edx-platform repo so that bind-mounts do not clobber them.
  • Use the new ln-assets script to create links from edx-platform to those artifacts.

The combined result of all of these changes is that you should only need to build static assets once, whether you're building prod or dev, whether you're bind-mounting edx-platform or not. You should only need to rebuild assets if you actually make changes to the asset sources (js, css, underscore, images, etc.).

Trying out this branch?

If you're trying this branch, I'm assuming that you are bind-mounting edx-platform and using tutor dev.

This is a based on nightly, make sure you have a recent-ish version of edx-platform master. It must include this change, merged early June.

You could also cherry-pick this Tutor commit onto Tutor master and try it out with edx-platform Redwood. You'll need a very recent version of redwood.master, including this change, merged mid-June.

Then, run:

tutor dev stop
tutor config save
tutor images build openedx-dev
tutor dev do init --limit=lms  # Will create the symlinks in edx-platform
tutor dev start -d

Getting back off this branch?

If you're switching back to upstream Tutor master or nightly, and you were using a bind-mounted edx-platform, then you'll want to remove the symlinks from your edx-platform bind-mount so that they don't interfere. From edx-platform, run:

  • make clean

Then:

tutor dev stop
tutor config save
tutor images build openedx-dev
tutor dev do init --limit=lms  # will re-generate your bind mount's artifacts, the old way
tutor dev start -d

@kdmccormick kdmccormick changed the title perf: don't unneccessarily rebuild dev assets (WIP) perf: don't unneccessarily rebuild dev assets May 31, 2024
@kdmccormick kdmccormick force-pushed the kdmccormick/assets-split branch 2 times, most recently from 012faef to 90b3b6d Compare June 5, 2024 14:17
@kdmccormick kdmccormick changed the base branch from kdmccormick/assets-link to master June 17, 2024 16:33
@kdmccormick kdmccormick changed the base branch from master to nightly June 17, 2024 16:33
@kdmccormick kdmccormick force-pushed the kdmccormick/assets-split branch 2 times, most recently from 4cf93da to aa2e674 Compare June 24, 2024 14:49
@kdmccormick kdmccormick force-pushed the kdmccormick/assets-split branch 2 times, most recently from cea0739 to fc75206 Compare June 25, 2024 14:52
@kdmccormick kdmccormick force-pushed the kdmccormick/assets-split branch 2 times, most recently from 0e444b9 to 58630de Compare July 16, 2024 15:08
regisb and others added 2 commits August 8, 2024 09:09
The warning message printed during `tutor local upgrade` was incorrectly
indented.
DawoudSheraz and others added 11 commits August 13, 2024 08:57
…#1083)

Currently, we are asking devs to set this env var manually before they run tests.
We may as well save them some keystrokes by setting it in the dev image
programmatically.
This will add a `-c` / `--clean` flag to the save command and ensure that the env directory is deleted if it exists.

Close overhangio#967
TODO:

* is the pre-assets patch in the right place?
* changelog entry
* more details in commit message
* test with 'tutor local'
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.

4 participants