Unify smaller apps into one openedx_content app

[ormsbee]

This is a wacky proposal that I've been kicking around in my head since I started working seriously on #452, and ran into issues with renaming the app and/or moving models around.

What is this?

This PR refactors the repo to combine all authoring apps (publishing, components, content, collections, etc.) into a single openedx_content app from Django's point of view. But the boundaries previously set up by the apps still exist in openedx_learning.apps.openedx_content.applets.* which has different packages for components, collections, and all the rest.

Each of these sub-apps still have their own models.py and admin.py files, though these are all stitched together by the higher level openedx_content files. So for instance, openedx.apps.openedx_content.models imports everything from the sub-apps. We could make utility wrappers to make this more convenient later. (Edit: Introspection magic breaks code autocomplete.)

It would look like:

openedx_learning/apps/openedx_content
├── __init__.py
├── admin.py
├── api.py
├── applets
│   ├── __init__.py
│   ├── backup_restore
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   ├── models.py
│   │   ├── serializers.py
│   │   ├── toml.py
│   │   └── zipper.py
│   ├── collections
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   ├── models.py
│   │   └── readme.rst
│   ├── components
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   ├── models.py
│   │   └── readme.rst
│   ├── contents
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   └── models.py
│   ├── publishing
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   ├── contextmanagers.py
│   │   ├── models
│   │   │   ├── __init__.py
│   │   │   ├── container.py
│   │   │   ├── draft_log.py
│   │   │   ├── entity_list.py
│   │   │   ├── learning_package.py
│   │   │   ├── publish_log.py
│   │   │   └── publishable_entity.py
│   │   └── readme.rst
│   ├── sections
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   └── models.py
│   ├── subsections
│   │   ├── __init__.py
│   │   ├── admin.py
│   │   ├── api.py
│   │   └── models.py
│   └── units
│       ├── __init__.py
│       ├── admin.py
│       ├── api.py
│       └── models.py
├── apps.py
├── backcompat
│   ├── __init__.py
│   ├── backup_restore
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   │       └── __init__.py
│   ├── collections
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   ├── components
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   ├── contents
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   ├── publishing
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   ├── readme.rst
│   ├── sections
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   ├── subsections
│   │   ├── __init__.py
│   │   ├── apps.py
│   │   └── migrations
│   └── units
│       ├── __init__.py
│       ├── apps.py
│       └── migrations
├── management
│   ├── __init__.py
│   └── commands
│       ├── __init__.py
│       ├── add_assets_to_component.py
│       ├── lp_dump.py
│       └── lp_load.py
├── migrations
│   ├── 0001_initial.py
│   ├── 0002_rename_tables_to_openedx_content.py
│   └── __init__.py
└── models.py

Why do this?

I still believe that having small, discrete app-like things is useful for controlling complexity, and I don't want to give that up. That being said, refactoring is made much harder when we want to try to either change the names of real Django apps (e.g. contents to media) or if we want to move models around (e.g. Container/ContainerVersion leaving the publishing app to go to a new containers app). Having all the models be in one namespace will make shifting the boundaries between them much easier.

This will have a few other minor benefits. We can do a top level api.py file for openedx_content in a way that's consistent with other apps in the system. It sort of sets up the umbrella openedx_content app as the holder of the public interface. It also makes it less cumbersome to enter in the list of apps.

On the downside, there's less consistency in terms of what goes where. Management commands, migrations, and app initialization code must all go in the root openedx_content app.

How does this work?

See the details at: https://github.com/ormsbee/openedx-learning/blob/big-authoring/docs/decisions/0020-authoring-as-one-app.rst

[ormsbee]

After this transition, I think I'd want to eventually refactor the applets package to look something more like:

applets/
        collections/
        components/
        containers/
                   sections/
                   subsections/
                   units/
        media/
        publishing/

[ormsbee]

I'm killing this auto-discovery code from the PR because I don't think the convenience is worth the loss of code introspection in editors and CI, but I want to preserve it here in case we need to write some applet-traversing code again:

"""
This was an attempt to make cute and clever code to dynamically discover and
import all applet modules of a certain type for aggregation purposes (e.g. the
authoring/models.py file importing from all applet models.py files). This code
actually does work, but I only realized after buildling it that it breaks
editor introspection/autocomplete, which makes the cost far too high for this
convenience.
"""

import functools
import importlib
import inspect
import pathlib


def auto_import(module_name):
    """Auto-import all modules with a given name in subdirs of applets."""
    caller_frame_info = inspect.stack()[1]
    caller_module = inspect.getmodule(caller_frame_info[0])
    caller_module_name = caller_module.__name__

    # converts openedx_learning.authoring.models -> openedx_learning.authoring
    import_base = ".".join(caller_module_name.split(".")[:-1])

    caller_filepath = caller_frame_info.filename
    caller_dir = pathlib.Path(caller_filepath).resolve().parent
    applets_dir = caller_dir / "applets"
    module_paths = applets_dir.rglob(f"{module_name}.py")
    relative_paths = [
        module_path.relative_to(caller_dir) for module_path in module_paths
    ]
    all_modules_dict = {}
    for relative_path in sorted(relative_paths):
        module_import_name = f"{import_base}." + ".".join(relative_path.parts)[:-3]
        module = importlib.import_module(module_import_name)
        module_dict = vars(module)
        if '__all__' in module_dict:
            module_dict_to_add = {
                key: module_dict[key]
                for key in module_dict['__all__']
            }
        else:
            module_dict_to_add = {
                key: val
                for (key, val) in module_dict.items()
                if not key.startswith('_')
            }
        all_modules_dict.update(module_dict_to_add)

    return all_modules_dict


auto_import_models = functools.partial(auto_import, "models")
auto_import_api = functools.partial(auto_import, "api")
auto_import_admin = functools.partial(auto_import, "admin")

[bradenmacdonald]

The authoring app's initial migration tries to be smart–it will create a whole new set of tables if it's a new database, but if it detects an up-to-date Ulmo install of app migrations, it will take over the model state for all those models without running any database operations.

Could this be done using Django's "squashed migration" feature instead of custom logic?

I'm currently going with authoring being an "umbrella app", and the small things being "applets"... but I'm open to suggestions.

I'm not a fan of "applet", so I'd suggest "[sub]modules". Or perhaps better, continue to call them "apps" and just rename the larger thing - "umbrella app", "super-app", "domain", etc.

[ormsbee]

I'm not a fan of "applet", so I'd suggest "[sub]modules". Or perhaps better, continue to call them "apps" and just rename the larger thing - "umbrella app", "super-app", "domain", etc.

My first stab at this was to shove them into a modules sub-directory, and then I was briefly floating the idea of calling authoring a "modular app". I backed away from it because I thought that overloading the use of the word "module" was bad, given its common Python meaning.

For the same reason, I don't want to call the small things "apps" because "apps" really means something in Django, and I don't want to overload that term. One of the reasons I went for "applet" was because it did not collide with any existing terminology that I know of in the Python/Django sphere. (I mean yes, there's the Java connotation, but Java applets have been dead for almost a decade now, and they had waned in popularity years before that.)

In our meeting, "subapps" was suggested, which I'm okay with. I honestly still prefer "applets" though, because I think it more clearly conveys that these things are not actually real Django apps, but instead are more diminutive, app-like things. If I see a subapps package, it still makes me think that the things inside it are actual Django apps.

[ormsbee]

@bradenmacdonald: I may be missing something, but I don't really see how to make squashed migrations work without keeping the shells of the old apps around in their old locations (at least enough for a apps.py and migrations folder), and that seems like it would be more confusing than it's worth. If the squashed migration happens only in the authoring app, it also doesn't work because deciding whether something is migrated or not doesn't depend on the state of the schema but on the entries in the django_migrations table, and that table will have no entries for oel_authoring regardless.

FWIW, the schema changes were auto-generated in the sense that I generated them from looking at the current state of the model files and then indented them in one level to wrap them in my SeparateDatabaseAndState subclass.

[bradenmacdonald]

keeping the shells of the old apps around in their old locations (at least enough for a apps.py and migrations folder)

I see. I guess that's the price that we'd have to pay to allow migrations from any point in time rather than Ulmo exactly.

[ormsbee]

I see. I guess that's the price that we'd have to pay to allow migrations from any point in time rather than Ulmo exactly.

Actually, I may have an idea around this. The two important things are the labels and getting all the apps into edx-platform's INSTALLED_APPS. I think I can shove the migration remnants into a corner somewhere, make a function to return the app locations that edx-platform needs, and hook up the migrations to be sequential so this all works out okay.

[ormsbee]

@kdmccormick, @bradenmacdonald: This should be ready for review. The corresponding edx-platform changes are in openedx/openedx-platform#37924 if you want to run it locally.

After thinking about it more, I'd still like to keep the term "applet" because:

1. It conveys that these things are not actual Django apps (which subapps implies).
2. It implies that they are smaller than full blown apps.
3. It does not conflict with common Django or Python terminology, like module or context would.
4. Java applets have been dead for a while.

If "applet" is a no-go for you folks, would something like "micro-app" or "mini-app" work?

[ormsbee]

I had to make a complete copy of the API code we currently use to do this migration, bur re-orient it to use the historical models rather than live ones. This is really what I should have done in the first place, to make the migration more robust—but now I simply have to, in order to keep this from breaking.

[bradenmacdonald]

I think that's worth explaining in the docstring at the top of this migration file.

[kdmccormick]

Code looks great! I'm fine with applets, you make a good case.

I checked this out with your edx-platform branch to test. Even after re-pip-installing openedx-learning, running DJANGO_SETTINGS_MODULE=cms.envs.tutor.development ./manage.py cms migrate keeps giving me:

ValueError: The field content_libraries.ContentLibrary.learning_package was declared with a lazy reference to 'oel_authoring.learningpackage', but app 'oel_authoring' isn't installed.
The field contentstore.ComponentLink.upstream_block was declared with a lazy reference to 'oel_authoring.component', but app 'oel_authoring' isn't installed.
The field contentstore.ContainerLink.upstream_container was declared with a lazy reference to 'oel_authoring.container', but app 'oel_authoring' isn't installed.
The field modulestore_migrator.ModulestoreBlockMigration.change_log_record was declared with a lazy reference to 'oel_authoring.draftchangelogrecord', but app 'oel_authoring' isn't installed.
The field modulestore_migrator.ModulestoreBlockMigration.target was declared with a lazy reference to 'oel_authoring.publishableentity', but app 'oel_authoring' isn't installed.
The field modulestore_migrator.ModulestoreMigration.change_log was declared with a lazy reference to 'oel_authoring.draftchangelog', but app 'oel_authoring' isn't installed.
The field modulestore_migrator.ModulestoreMigration.target was declared with a lazy reference to 'oel_authoring.learningpackage', but app 'oel_authoring' isn't installed.
The field modulestore_migrator.ModulestoreMigration.target_collection was declared with a lazy reference to 'oel_authoring.collection', but app 'oel_authoring' isn't installed.

I can poke at it more later, but let me know if this is something you ran into while testing.

[kdmccormick]

Suggested change

	# path('__debug__/', include('debug_toolbar.urls')),
	path('__debug__/', include('debug_toolbar.urls')),

[kdmccormick]

Suggested change

	Module for parts of the Learning Core API that exist to make it easier to use in
	Django projects.
	"""
	def learning_core_apps_to_install():
	Module for parts of the Learning Core API that exist to make it easier to use in
	Django projects.
	"""
	def openedx_learning_apps_to_install():

we don't call it learning_core anywhere else in the code, so I'd air towards consistency here.

[bradenmacdonald]

Though I have to point out, with your proposed name, "learning apps" sounds to me like "not authoring apps", and the only apps in here are for authoring, not learning. So I kinda wish we'd go the other way and use learning_core more in the code where we refer to this package. But maybe that ship has sailed.

In other words, openedx_learning_apps_to_install can be read as "openedx_learning apps to install", which makes sense, but also read as "Open edX learning apps to install" which doesn't.

[ormsbee]

I checked this out with your edx-platform branch to test. Even after re-pip-installing openedx-learning, running DJANGO_SETTINGS_MODULE=cms.envs.tutor.development ./manage.py cms migrate keeps giving me:
...

Huh. Yeah, I see it now as well. I ran into a variant of this last week, and I thought I had fixed it. I did somehow manage to get it to the fully migrated state. But now that I try starting from scratch again, I get the same error. I'll look into it. Thank you.

[ormsbee]

Oh, I get it. The migration adjustments I made on the edx-platform side were before I kept all the old apps around, so the references need to be moved back to oel_publishing for things like this. I'll push up the changes later tonight.

[bradenmacdonald]

Code looks good. Let me know when you've made those changes to fix the migrations, and I can test it on my devstack.

[bradenmacdonald]

I think that's worth explaining in the docstring at the top of this migration file.

[ormsbee]

@bradenmacdonald, @kdmccormick: This is ready for another review round. I'll update the openedx-platform PR that points to it later tonight.

[bradenmacdonald]

I think I'm missing something here. None of the Platform migrations actually manipulate the database or depend on its actual state in any way. So why is the ordering so important? What happens if one runs all the Learning Core migrations first, then the platform migrations? Does Django block the 0002_rename_tables_to_openedx_content migration because it detects that the foreign keys in its State tracking will be out of sync with the database?

[ormsbee]

Even though the platform migrations don't touch database state, they do alter the tracked model state. So if openedx-learning migrations all run first, then Django gives an error when trying to later run the platform migrations because as far as it's concerned, the backcompat models no longer exist (which results in an error saying that the backcompat app itself does not exist).

Or to put in other words, the platform migration wants to switch a foreign key reference from oel_publishing.LearningPackage to openedx_content.LearningPackage, but when it comes to compute what that actually means, it will detect that oel_publishing.LearningPackage doesn't exist because oel_publishing : 0011_remove_all_field_state_for_move_to_applet has already dropped those logical models (even if the tables are still there).

If people always run CMS migrations before LMS migrations, this wouldn't be a problem. It hasn't been a problem in the past because the dependencies strictly went one way: openedx-learning could do whatever it wanted, and the openedx-platform apps would catch up later. It's different now because:

1. The final backcompat app migrations are destructive in terms of model state, as they all get deleted.
2. We want to force the platform migrations to point to the openedx_content models before we rename the tables under those models.

If it weren't for (2), we might be able to get away with leaving the backcompat app models around. I'm not sure about what kind of issues that might cause down the road though.

[bradenmacdonald]

OK, this is getting way too convoluted, but could we create a final migration in openedx_content (not backcompat) which re-creates the "old" tables like oel_publishing.LearningPackage as an alias of the new tables, strictly in migration state? Then when the platform migration runs, Django will see both the old and the new model, see that they point to the same table, and run the migration as a no-op. We want to get rid of the old table references in the code, but I don't see a problem having them live on in the migration state indefinitely, unless we anticipate reusing those table namespaces.

Alternately, just don't drop those old logical tables or another year or so.

If people always run CMS migrations before LMS migrations

Which one does tutor run first? If tutor runs CMS first, then I'd say it's not a huge deal, as most people won't hit the error and anyone who does can just --fake the migration and move on.

[ormsbee]

It's not really about the table, it's the logical model that's at the app level. In the Django migration model state, the platform app models don't have key references to the table oel_publishing_learningpackage, they have have references to the oel_publishing app model LearningPackage, which happens to map to that table. But there's no way that I know of to make openedx_content take over the app namespaces of all the apps it's replacing.

Alternately, just don't drop those old logical tables or another year or so.

This could work. We could remove the backcompat migrations that drop all the model state. We keep the platform migrations that switch over the field references, but we make those happen after the rename of the tables. That way the ordering is always going to be consistent regardless of whether the LC migrations are run first in isolation, or if they're all mixed together. So it looks like:

1. New openedx_content models are created.
2. Underlying oel_* tables are renamed to openedx_content
3. Platform models switch their references over. I think this can be a zero-database-operation migration, but in the worst case we re-build some foreign key constraint indexes.

So then the backcompat models basically become phantoms with no backing tables. But that's the major downside to it—we'd have to keep those backcompat model modules around, with big fat warnings everywhere to not use or touch them, because any changes will prompt migrations to be created for them, and those migrations will always fail.

Which one does tutor run first? If tutor runs CMS first, then I'd say it's not a huge deal, as most people won't hit the error and anyone who does can just --fake the migration and move on.

It looks like LMS runs first.

[bradenmacdonald]

If we later move openedx_learning.apps.openedx_content -> openedx_core.apps.openedx_content or openedx_content (top level package) as discussed in the arch sync, will that be a trivial change or complex like this?

Edit: See #468 which tracks this follow-up work.

[ormsbee]

It should be trivial. The references in openedx-platform need to be updated, and in particular things like the field definitions (like this example).

But those field definitions don't mess up the migrations, because they're not really recorded in the migration table. So just updating all the references should be fine as long as the code remains the same.