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

feat: full edx-platform setup with tutor dev launch -m ... #813

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

kdmccormick
Copy link
Collaborator

@kdmccormick kdmccormick commented Mar 13, 2023

Preface

This is the part of effort to make it easier to work with mounted repositories.

@regisb , previously we had agreed that it'd be good to generate the .egg-info folder automatically using an init task. I realized the other day, though, that we may as well do the same thing for installing node_modules and regenerating static assets too! With this change, we can setup a dev env with a bind-mounted edx-platform in one command.

In the future, I am still interesting in removing the need to reinstall node_modules and regenerate static assets; for now, though, I think that this is an improvement, and its user interface is compatible with the long-term vision.

I did my best to rework the Open edX development docs around the simplified setup. Hopefully they feel simpler now.... very open to feedback here, though.

Finally, I expect that bindmount-canary might be controversial, but I'm not sure of a better solution--open to other suggestions 😄

Description

Before this commit, setting up an edx-platform development environment took multiple steps:

  tutor dev launch
  tutor dev run --mount=/path/to/edx-platform lms bash
  >> pip install -e .
  >> npm clean-install
  >> openedx-assets build --env=dev

This commit moves the steps under run into an init task, which is automatically run by launch. Thus, setup is now one command:

  tutor dev launch --mount=edx-platform

These extra init steps are only applicable when bind-mounting edx-platform (because bind-mounting the repository overrides some important artifacts that exist on the image, which must be re-generated). Thus, the new init tasks exists early if it detects that it is not operating on a bind-mounted repository.

Finally, we try to simplify the Open edX development docs so that it is clearer how bind-mounting fits into the development process.

Part of:

This works around (but does not close) these related issues:

Changed docs

image

@kdmccormick
Copy link
Collaborator Author

Something I just realized but don't have time to confirm right now: If -job services don't bindmount themes, then that'll cause an issue with static asset compilation for custom theme users, right?

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

This is a great step forward, I love it!

docs/dev.rst Outdated Show resolved Hide resolved
docs/dev.rst Outdated Show resolved Hide resolved
docs/dev.rst Show resolved Hide resolved
tutor/commands/jobs.py Show resolved Hide resolved
docs/dev.rst Outdated Show resolved Hide resolved
tutor/templates/build/openedx/Dockerfile Show resolved Hide resolved
npm clean-install

# Regenerate static assets.
openedx-assets build --env=dev
Copy link
Contributor

Choose a reason for hiding this comment

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

These latter two steps will be performed every time we run launch with a bind-mount. I don't have a better alternative, so let's keep it that way, but we should keep it in mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something we could do, eventually, would be to use a hash to ensure that these steps are not run extraneously.

  • node_modules would be easy: upon running npm install, create a node_modules/.package_lock_hash file, containing a hash of package-lock.json. If package-lock.json matches node_modules/.package_lock_hash, then we can skip node_modules installation.
  • assets would be more complicated, but still doable. The scripts/build-assets.sh has the means to a compile a list of all asset input files. Saving the combined hash of those somewhere would allow us to skip asset compilation when assets are unchanged.

Thing is, this is just reimplementing what Docker already does. That's why I'd rather move node_modules and assets outside of the bind-mount if at all possible.

@regisb
Copy link
Contributor

regisb commented Mar 14, 2023

If -job services don't bindmount themes, then that'll cause an issue with static asset compilation for custom theme users, right?

Yes, this is correct. So we should bind-mount the themes folder as well.

But we're not going to be able to solve all issues related to running a custom edx-platform repo. For instance, if the edx-platform python requirements are modified, then in theory we should also run pip install -- but we are not going to, because it would take forever.

@kdmccormick
Copy link
Collaborator Author

Yes, this is correct. So we should bind-mount the themes folder as well.

Sounds good, I will add that to the PR.

in theory we should also run pip install -- but we are not going to, because it would take forever.

Even if it were quick, I'm not sure we would want run pip install, because the changes to /openedx/venv would only exist in ephemeral container memory. The pip updates would be missing in other containers (cms, lms-worker, etc) and they would be blown away from lms whenever the platform got brought down.

So, changes to pip requirements require an image rebuild. I don't think that's a bad thing, I just think it means we need to make sure that developers can change pip requirements and then rebuild as little of the image as possible: openedx-unsupported/wg-developer-experience#162

Before this commit, setting up an edx-platform development environment
took multiple steps:

   tutor dev launch
   tutor dev run --mount=/path/to/edx-platform lms bash
   >> pip install -e .
   >> npm clean-install
   >> openedx-assets build --env=dev

This commit moves the steps under ``run`` into an init task, which
is automatically run by ``launch``. Thus, setup is now one command:

   tutor dev launch --mount=edx-platform

These extra init steps are only applicable when bind-mounting
edx-platform (because bind-mounting the repository overrides
some important artifacts that exist on the image, which must be
re-generated). Thus, the new init tasks exists early if it detects
that it is *not* operating on a bind-mounted repository.

Finally, we try to simplify the Open edX development docs so that
it is clearer how bind-mounting fits into the development process.

Part of: openedx-unsupported/wg-developer-experience#146
Closes: openedx-unsupported/wg-developer-experience#152

This works around (but does not close) these related issues:
* openedx-unsupported/wg-developer-experience#150
* https://github.com/openedx/wg-developer-experience/issues/151
@kdmccormick kdmccormick force-pushed the kdmccormick/mounted-init branch 3 times, most recently from 782797b to a0e7caf Compare March 14, 2023 19:26
@kdmccormick
Copy link
Collaborator Author

Alright @regisb , this is ready for another look.

These bind-mounts:

* ../build/openedx/themes:/openedx/themes
* ../build/openedx/requirements:/openedx/requirements

existed in the dev lms and cms containers, but they did
not exist in the lms-job and cms-job containers.

This means that themes and requirements that were *built into the
image* would exist in the job containers, but live updates to the
themes and requirements would not apply.

To resolve this, we set ``volumes:`` on the lms-job and cms-job
services so that they match the volumes for the normal lms and
cms services.
@kdmccormick kdmccormick force-pushed the kdmccormick/mounted-init branch from 441382e to fd626f5 Compare March 14, 2023 20:27
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

This looks great to me! Do you want to add anything else before we merge?

@kdmccormick
Copy link
Collaborator Author

Nope, merge away 🚀

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.

3 participants