Move workspace and grader hosts into apps directory #7572
Conversation
Dockerfile
Outdated
| # Note that we also have to copy the `apps` and `packages` directories so that | ||
| # `yarn` can resolve the workspaces inside it and set up symlinks correctly. | ||
| # This is suboptimal, as a change to any file in these directories will | ||
| # invalidate this layer's cache, but it's unavoidable. |
There was a problem hiding this comment.
This is going to really suck once all of PrairieLearn is inside apps. The best solution I have for this is to individually COPY every app or package package.json file, probably with some script to enforce that we didn't forget any? I don't love this, but I also really want to take advantage of layer caching :/
There was a problem hiding this comment.
I also can't think of any other way around it.
However, I just want to make sure that we really should be so worried about layer caching. What's the use case here? People doing local dev? Afaik, our executor image won't benefit from layer caching because we never pull updated versions?
Assuming we COPY packages/package-1/package.json, etc, then apps/app-1/package.json, I can see three common cases:
- All
package.jsonfiles are unmodified: big win from caching. - The PL
package.jsonhas a change, but all of thepackages/have not changed. Some win from caching, but I'm not sure how much of a win this actually is? PL presumably has by far the largestnode_modules? - One of the first COPYed
packages/has a change inpackage.json: almost all caches are blown away.
So I guess I'm wondering just much win we get in case (2), which I think is quite common, and how often we get case (1)? Unless people are pulling new images very frequently, I think they are likely to normally hit case (2). Certainly if they pull once a month they are guaranteed to hit (2) because of dependabot.
Am I thinking about this correctly?
There was a problem hiding this comment.
The main thing I'm thinking about is CI times. For a typical master build that changes code but not dependencies, copying the entire apps/ directory in one go means that we'll incur the cost of downloading + building all deps and the cost of pushing new layers to Docker Hub. That may be several minutes, which isn't a ton, but I'm already struggling to keep our time from merge to deploy low, and this just makes that worse.
Docker appears to be completely unwilling to allow for reasonable glob behavior for COPY: moby/moby#15858.
There was a problem hiding this comment.
I took the individual copy approach in f402ab6, which seems to work as expected. I added tools/validate-dockerfile.mjs to make sure we don't forget to add any packages.
There was a problem hiding this comment.
Ugh, it inexplicably takes 2.5 minutes to do all the COPY steps on GitHub Actions?? It's essentially instant for me locally...
There was a problem hiding this comment.
I'm trying the "copy all of apps/ and packages/" approach in 784ec9e, we'll see how that fares.
There was a problem hiding this comment.
It's marginally faster in the case of a cache miss, at the cost of incurring cache misses more often.
I think we just need to hope that Docker implements COPY --parents soon (moby/moby#35639) 😢
There was a problem hiding this comment.
Ok, I agree this is all we can do for now. And thanks for reminding me about CI being the main use case for this! I always manage to forget about CI 😅
.github/workflows/main.yml
Outdated
| # Ideally we'd use `with.cache: 'yarn'` in the `setup-node` action, but that | ||
| # doesn't support restoring from fallback cache keys in the case of an exact | ||
| # cache hit: https://github.com/actions/setup-node/issues/328. |
There was a problem hiding this comment.
actions/setup-node#328 - there are a few WIP PRs that address this limitation, so hopefully we can switch back to using actions/setup-node for caching soon!
| @@ -97,7 +97,7 @@ pl_tmux_start_workspace_pane () { | |||
| echo "Starting workspace host..." | |||
| # A background process spams this pane, so pipe it to cat | |||
| # to prevent a shell prompt from appearing also: | |||
| make start-workspace-host | cat | |||
| make dev-workspace-host | cat | |||
There was a problem hiding this comment.
@echuber2 what're the desired semantics of this script? That is, should it start PL+workspaces in a way that's suitable for local development of PrairieLearn itself, or for a course? I'm treating it as the former, hence the usage of dev scripts here, but I'd like your opionion.
If this script is no longer useful to you, I'd be happy to resolve this question by just deleting it 😅
This PR introduces an
apps/directory. It will function similarly to the existingpackages/directory in the sense that its contents will be treated as Yarn workspaces and will generally have abuildscript that will be run automatically; it will just contain "apps" (distinct runnable services) instead of sharable packages.The
workspace_hostandgrader_hostdirectories have been renamed toapps/workspace-hostandapps/grader-host, respectively. I intend to do the same with the PrairieLearn source code too in a separate PR.This work will unlock our ability to use TypeScript and other built assets in apps.