The gist
Code in edx-platform/[lms|cms|common]/djangoapps should now be imported by its full path, relative to the root of edx-platform. For example:
from student import api
should now be:
from common.djangoapps.student import api
If you maintain forks of edx-platform or packages that import edx-platform code, then when updating your forks to Koa, resolve conflicts in favor of the new, full import path.
If you are a committer to edx-platform or to packages that import edx-platform code, use the new, full import path (explicit relative imports with a leading dot, like from .models import MyModel, are also good). The old, abbreviated paths are deprecated and will raise warnings. In some instances, using the old form will lead to errors or bugs.
If you see EdxPlatformDeprecatedImportWarnings kicking around in our code, consider fixing them! There might be a badge in it for you…
The details
For those interested in why we’ve made this change…
Background
In most Python 3 projects, local code is imported from the root of the project. For example, in edX’s ecommerce repo, all local code is imported like from ecommerce.x.y import z. One can also use explicit (dot-prefixed) package-relative imports, a la from .y import z, but Python internally expands those to the full path, so they’re essentially the same thing.
edx-platform has four top-level code folders (lms/, cms/, common/, and openedx/), so one would still expect that all imports start from one of those folders, such as from lms.djangoapps.courseware import urls.
However, in the early days of the platform, it was thought that the Django apps in lms/djangoapps, cms/djangoapps, and common/djangoapps would be broken out into their own projects. So, up until Juniper, those paths were explicitly added to sys.path, making it so all those Django apps were importable as root packages. Some of us call this the “sys.path hack”.
Decision
A few months ago, we decided to remove the sys.path hack. Reasons include:
- Having multiple imports roots is confusing to new (and existing!) edx-platform developers, who are used to local imports starting at the root of a project.
- Previously,
from courseware import viewsandfrom lms.djangoapps.courseware import viewswould instantiate two different instances of the same module, creating all sorts of hard-to-diagnose bugs. - Dev tools (like pylint, isort, mypy) each need to be tweaked to understand the import paths, adding to the platform’s configuration complexity.
- Some static analysis tools that we may be interested in, such as
import-linter, are likely to get confused by the multiple, overlapping import roots.
Instrumenting this involved updating hundreds of references to old import paths.
Backwards-compatibility: Import shims
We knew that switching over to the full import paths and simultaneously dropping support for the old paths would be tough, both for ourselves and for the community.
So, we are maintaining partial and temporary support for the old paths, via import_shims. For both LMS and Studio, the shims replicate the entire deprecated import structure with stub modules that wildcard-import from the corrected import path, also raising a EdxPlatformDeprecatedImportWarning. This will allow imports like from student import views to work for at least a while longer.
Be aware that the import shims don’t work in every instance. Notably, any string references to modules (like @mock.patch('student.views.log')) will break in spite of the shims.
The shims will likely be removed in Lilac.
Potential next steps
common/lib
One oddity that was not addressed in this effort are the packages in edx-platform/common/lib, which are also imported strangely (e.g., from xmodule.tabs import CourseTab). These packages are actually installed into edx-platform as their own projects. Normalizing (or extracting) them was outside of scope of this work for Koa, but I could see this as something to change in the future.
isort
When changing imports in lms/, openedx/,and common/, I resisted the urge to “tidy up” the import blocks at the top of the modules, in order to make the patches easier to apply to forks. So, the imports blocks in edx-platform are pretty messy now.
For Lilac, it may be worth enabling import sorting in edx-platform via the isort tool, which we use in several other repos. Although the first run of isort would make rebases painful, I think it would reduce merge conflicts in the long term, while also making the import blocks easier to mentally parse. I’m interested to hear any opinions on this.
Thanks!
A big thanks goes to @cpennington, who initiated this effort.
Feel free to reply with any questions or concerns.