Prevent XBlock 1.3.1 from being installed arbitrarily by EDXAPP_EXTRA_REQUIREMENTS

OmarIthawi commented 4 years ago

:warning: This a big change to how we modify our requirements :warning:

Background

We found that EDXAPP_EXTRA_REQUIREMENTS is installing the latest XBlock all the time and a couple of days ago this broke our Tahoe Staging and would also break Tahoe Production if we don't fix it.

We pushed a hack https://github.com/appsembler/edx-configs/pull/854 to force ping the XBlock to 1.2.2 but it's just a hack.

I think the correct solution is to either:

Easy: Merge this PR.
Hard: Bake all requirements into the edx-platform itself and use pip-tools to help us find and fix conflicting requirements. While pip-tools have been the standard way to mange requirements for Open edX, we never used it as it's mostly impossible to use for a fork that's behind the upstream master. Long story here. So I don't recommend this route.

The Fix

Previously: Any requirement in EDXAPP_EXTRA_REQUIREMENTS gets to install all its dependencies without restrictions. This was often helpful to avoid listing too much dependencies within EDXAPP_EXTRA_REQUIREMENTS.
Proposed: EDXAPP_EXTRA_REQUIREMENTS packages are no longer able to install extra requirements which could break Open edX for new installations. We need to explicitly specifiy the required dependencies within EDXAPP_EXTRA_REQUIREMENTS in addition to their own dependencies.

How to Test this PR

For now: Push it to production with https://github.com/appsembler/edx-configs/pull/854 reverted. It should fix the issue of XBlock.
Pretty soon: Create a fresh edxapp server and deploy, it'll probably break and we need to fix it.

thraxil commented 4 years ago

I'm going to do a test deploy with this on the sandbox since I've reproduced the problem there.

thraxil commented 4 years ago

deploying this for the sandbox, i hit this error during migrate:


Traceback (most recent call last):
  File \"manage.py\", line 118, in <module>
    startup.run()
  File \"/edx/app/edxapp/edx-platform/lms/startup.py\", line 19, in run
    django.setup()
  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/__init__.py\", line 27, in setup
    apps.populate(settings.INSTALLED_APPS)
  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/apps/registry.py\", line 108, in populate
    app_config.import_models()
  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/apps/config.py\", line 202, in import_models
    self.models_module = import_module(models_module_name)
  File \"/usr/lib/python2.7/importlib/__init__.py\", line 37, in import_module
    __import__(name)
  File \"/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/course_access_groups/models.py\", line 12, in <module>
    from organizations.models import Organization, UserOrganizationMapping
ImportError: cannot import name UserOrganizationMapping

Which is at least different from the XBlock error. Since it's import-related, I'm guessing that it has something to do with the same problem though. (or maybe it's something separate and related to the recent CAG work?)

OmarIthawi commented 4 years ago

@thraxil that means we're not installing our own edx-oragnization fork, or worse, we're uninstalling it in favor of the edx fork. UserOrganizationMapping is something we did and not available (yet) in upstream.

@melvinsoft would you mind taking over this PR? I need to focus on Mutli-Tenant Emails.

thraxil commented 4 years ago

FWIW, the sandbox's server-vars.yml is here: https://github.com/appsembler/edx-configs/blob/master/appsembler/sandbox/hawthorn/prod/files/server-vars.yml. It should mostly be sticking with default packages/requirements/etc defined for the hawthorn base but it does specify course-access-groups==0.3.0: https://github.com/appsembler/edx-configs/blob/master/appsembler/sandbox/hawthorn/prod/files/server-vars.yml#L59

thraxil commented 4 years ago

Does anyone else have input on this? When I tested it, I ran into the issue I mention above. That's on a vanilla hawthorn config that doesn't install any EDXAPP_EXTRA_REQUIREMENTS. Previous deploys had worked and nothing changed other than using this PR.

Looking at what this code does, it seems like it shouldn't factor in and the organizations stuff ought to be unrelated (but why did that come up all of a sudden?), but there's so much complexity around requirements and dependencies in edx that I certainly wouldn't say that with any confidence.

johnbaldwin commented 4 years ago

On the surface, it seems reasonable, but as you say, Anders, there's so much complexity around requirements and dependencies in edx.

My feeling is that we need to define clear test criteria. Do we install this fix on staging and compare installed versions of python packages against what is expected?

johnbaldwin commented 4 years ago

Thanks Anders. @OmarIthawi Can we get this tested on Staging before we do a production push with it? I'll approve with caveat too

OmarIthawi commented 4 years ago

Thanks Anders and John. I've made the code a bit more backward compatible until we are sure it fixes the issue on staging.

bryanlandia commented 4 years ago

@OmarIthawi what about private requirements? We have more control over those but not necessarily on their dependencies

OmarIthawi commented 4 years ago

@OmarIthawi what about private requirements? We have more control over those but not necessarily on their dependencies

Good question.

Sorry @bryanlandia. I don't have the bandwidth to follow up with this. I'll close and please you or @melvinsoft feel free to reopen in another PR.

appsembler / configuration