BOM-2575: Delete common/lib

[UsamaSadiq]

Description

• PR to fix the final remaining issue BOM-2575 under the epic Dissolve Sub-projects under edx-platform

Major Changes

• Common/lib folder removed from edx-platform.
• commonlib-tests test section renamed to pavelib-tests since now it only runs the tests for pavelib/paver_tests.

[kdmccormick]

Thanks @UsamaSadiq . I'm excited that we're almost done with this epic.

Please include links to the public version of the tickets. Nobody outside of 2U can access the 2u-internal.atlassian.net links. If you must include private links in addition to public links, you can use the Private-ref: commit footer. Please ask your team to do this for all openedx repositories.

Lastly, just a reminder that you may need to update edX/2U's Ansible playbooks as I mentioned here. I've already proposed the corresponding Tutor change.

[kdmccormick]

Looks great, thanks @UsamaSadiq !

[kdmccormick]

Having this PR merged will help me make several Tutor improvements. Let me know if there is any way I can help @UsamaSadiq.

[aht007]

@kdmccormick Usama is on leave for the whole week. Let me know if it's urgent and you want to get it merged before next week so I will try to work on it

[kdmccormick]

@aht007 I would appreciate it if you folks were able to merge it this week. If you are not able to, let me know, and I can pick it up myself.

[awais786]

Sandbox creation failed with this PR so we are investigating it now.

[kdmccormick]

Indented version:

  .run()
File \"/edx/app/edxapp/edx-platform/lms/startup.py\", line 20, in run
  django.setup()
File \"/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/__init__.py\", line 24,
  in setup apps.populate(settings.INSTALLED_APPS)
File \"/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/apps/registry.py\", line 114, in populate
  app_config.import_models()
File \"/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/apps/config.py\", line 301, in import_models
  self.models_module = import_module(models_module_name)
File \"/usr/lib/python3.8/importlib/__init__.py\", line 127, in import_module
 return _bootstrap._gcd_import(name[level:], package, level)
File \"<frozen importlib._bootstrap>\", line 1014, in _gcd_import
File \"<frozen importlib._bootstrap>\", line 991, in _find_and_load
File \"<frozen importlib._bootstrap>\", line 975, in _find_and_load_unlocked
File \"<frozen importlib._bootstrap>\", line 671, in _load_unlocked
File \"<frozen importlib._bootstrap_external>\", line 848, in exec_module
File \"<frozen importlib._bootstrap>\", line 219, in _call_with_frames_removed
File \"/edx/app/edxapp/edx-platform/lms/djangoapps/bulk_email/models.py\", line 22, in <module>
  from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name
File \"/edx/app/edxapp/edx-platform/openedx/core/djangoapps/course_groups/cohorts.py\", line 22, in <mod

[kdmccormick]

I think some of the stack trace is missing. If I had to guess from what's there, though, I would bet that the Ansible provisioning scripts need to have pip install -e . added wherever they install requirements.

[kdmccormick]

Specifically, I think a task that runs pip install -e . may need to be added right after this task: https://github.com/openedx/configuration/blob/master/playbooks/roles/edxapp/tasks/deploy.yml#L151-L165

[awais786]

I think some of the stack trace is missing. If I had to guess from what's there, though, I would bet that the Ansible provisioning scripts need to have pip install -e . added wherever they install requirements.

https://gist.github.com/awais786/568516a3645fd4704acf3ad2d85ade66

[kdmccormick]

Thanks @awais786 , this is what I was looking for:

File \"/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content_libraries/models.py\", line 81, in <module>", "
  class ContentLibrary(models.Model)
File \"/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py\", line 113, in __new__", "    
  raise RuntimeError(
    "RuntimeError: Model class openedx.core.djangoapps.content_libraries.models.ContentLibrary
    doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS."
    )

This is happening because your sandboxes are not running pip install -e ., so edx-platform's setup.py entrypoints are not getting set up. Entrypoints are how Django Plugins get installed, and content_libraries is a Django plugin, which is why we see Django complaining about it here.

[UsamaSadiq]

Hi @kdmccormick,
Thanks for helping us out on this issue.
For the ansible changes, as you highlighted, we'll need to add a new step to run pip install -e . after installing the requirement files in the ansible playbooks. Creating a fix PR in confiiguration to upgrade the ansible playbooks accordingly will hopefully resolve this issue.

Plan of action

• I'll spin up a PR in configuration to add new command target within edxapp role playbooks specifically in the deploy.yml.
• Use the created configuration branch to spin up a custom sandbox to test these changes again.
• Merge both the configutation PR & this current PR together so they can both go out in the same deployment cycle.

Concern/Question

• According to my understanding, since the same playbook is used to install requirements within sandbox and prod, it'll go smoothly after this change but please let us know if we need to look out for some other aspects as well.
• Would it perhaps not work if we add the output of pip install -e . in the base.txt file directly within the make upgrade command as it was being done previously using local.in as a medium? That would perhaps save us from updating all the points to explicitly install -e . and make the process simpler. [I'm not sure if this approach will work since that was the reason we had the packages mentioned explicitly in the local.in file before too]

[kdmccormick]

@UsamaSadiq Your plan sounds great to me 👍🏻

According to my understanding, since the same playbook is used to install requirements within sandbox and prod, it'll go smoothly after this change but please let us know if we need to look out for some other aspects as well.

I believe that your understanding is correct, but without edX-internal access I cannot be 100% sure. I'm confident, though, that if any issues slip through, they would be caught by edX's Stage and thus shouldn't impact Prod.

Would it perhaps not work if we add the output of pip install -e . in the base.txt file directly within the make upgrade command as it was being done previously using local.in as a medium? That would perhaps save us from updating all the points to explicitly install -e . and make the process simpler. [I'm not sure if this approach will work since that was the reason we had the packages mentioned explicitly in the local.in file before too]

I'm glad you asked this. The reason we don't want base.txt to contain -e . is because it makes our build tooling less flexible and optimizable.

As a specific example, in Tutor's Dockerfile, we currently must do this:

COPY ./edx-platform /openedx/edx-platform     # Layer 1: Copy in all of edx-platform
RUN pip install -r requirements/edx/base.txt  # Layer 2: Install requirements
...                                                   

This is bad, because every time any part of edx-platform is modified, we will need to re-build Layer 2, even if no requirements have changed.

Instead, we'd like to write our Dockerfile like this:

COPY ./edx-platform/requirements \
     /openedx/edx-platform/requirements       # Layer 1: Copy in *only* requirements files
RUN pip install -r requirements/edx/base.txt  # Layer 2: Install requirements  

COPY ./edx-platform /openedx/edx-platform     # Layer N: Copy in rest of repository                                                  

Here, Docker can use the cached version of Layers 1 and 2 as long as the requirements files have not change. This would allow developers to modify code in edx-platform without having to re-build Layers 2-N every single time.

This brings us to the issue: If base.txt contains -e ., then we cannot install requirements without the entire code of edx-platform being present. So, one of the motivations behind all this common/lib work was to remove -e . from base.txt, unlocking a major optimization for Tutor's Dockerfile (as well as edx-platform's official Dockerfile, and other build tooling potentially).

[UsamaSadiq]

Thanks a lot for the detailed explanation @kdmccormick.
I'll attempt the discussed solution and will get back to you once we've tested the changes with the updated configuration playbooks.

[kdmccormick]

Thanks Usama.

[UsamaSadiq]

Refactoring the codejail github dependency link had caused a failure in all the Python/Capa problems on multiple client ends. To resolve the immediate issue, some of the changes were rolled back from production 3f14733...f6965b9 but since a revert PR or fix hasn't been merged yet so the pipeline is still paused.
We'll have to hold our merge and deployment until the issue gets resolved properly, production is working fine and the pipeline gets unpaused.

FYI @kdmccormick

[kdmccormick]

🤦🏻 Welp, I did that one to myself.

Thanks for the heads up Usama. By the way, I will be out until 9/27.

[kdmccormick]

Regarding the codejail failure, I opened #31021 in case it helps.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[kdmccormick]

Thank you @UsamaSadiq and @aht007 ! I appreciate all your team's work on getting rid of common/lib.