fix!: use full import path to safe_lxml

[kdmccormick]

What's this PR do?

Previously, edx-platform's safe_lxml module was imported as:

import safe_lxml

This still works, but will stop working very soon.

This commit updates edx-sga's usage of safe_lxml to the new, correct import path:

import openedx.core.lib.safe_lxml

This change is NOT backwards-compatible with Nutmeg or earlier.
Bumps edx-sga version to 0.19.0.

What are the relevant tickets? / Any background context you want to provide?

• Forum post detailing recent edx-platform import changes: https://discuss.openedx.org/t/breaking-apart-edx-platforms-common-lib-folder/7556
• Jira epic explaining rationale behind changes: https://openedx.atlassian.net/browse/BOM-2579
• Specific Jira ticket describing what changed in edx-platform: https://openedx.atlassian.net/browse/BOM-2583
• edx-platform PR to remove compatibility hack, which this edx-sga PR blocks: refactor!: remove safe_lxml import compatibility hack openedx/edx-platform#30671

How should this be manually tested?

This is probably safe without further manual testing, but anyway here's what I did:

• I ran unit tests via tox and confirmed they passed. Pylint failed, but it failed on master too.
• I made sure this worked with the corresponding branch of edx-platform. From within tutor dev run lms bash -m path/to/my/edx-platform:
  • I installed my version of edx-sga:
    • pip install git+https://github.com/kdmccormick/edx-sga@kdmccormick/fix-safe-lxml-import#egg=edx-sga==0.19.0
  • I ran these tests, which had been failing because of the old safe_lxml import in edx-sga:
    • pytest lms/djangoapps/grades/tests/test_scores.py -k possibly_scored

Where should the reviewer start?

Changes should be pretty non-controversial, but let me know if you have concerns.

One thing: If this commit merges as-is, master of this repo will be incompatible with open-release/nutmeg.master of edx-platform (and earlier). Is that OK? Or does master of this repo need to support older versions of edx-platform?

Screenshots (if appropriate)

N/A

What GIF best describes this PR or how it makes you feel?

I'm just going around breaking stuff:

(just kidding, hopefully)

[kdmccormick]

Hey @pdpinch , could one of you folks take a look?

[pdpinch]

small note: you don't need to bump the version. Our release bot will take care of it.

[kdmccormick]

Gotcha, fixed.

[umarmughal824]

@kdmccormick could you please fix the failing build.

[kdmccormick]

@umarmughal824 pylint is failing on master -- see #325

[kdmccormick]

@umarmughal824 would you be able to fix the pylint build on master so that the test suite can pass on this PR?

[arslanashraf7]

@umarmughal824 would you be able to fix the pylint build on master so that the test suite can pass on this PR?

@kdmccormick Thanks for this PR.

Just wanted to let you know that we have fixed the linting issues in the master branch. A rebase should fix the checks on this PR too.

[kdmccormick]

Thanks @arslanashraf7 ! Rebased & all ready for another round of checks & review.

[arslanashraf7]

The changes look good to me, but since this is a breaking change and incompatible with Nutmeg and earlier releases it would probably be safe to check or do a few things:

1. It's worth adding some compatibility section in the README mentioning the release (0.20.0 This is going to be the next version Supposedly) is compatible only with edX release after Nutmeg.

2. One thing: If this commit merges as-is, master of this repo will be incompatible with open-release/nutmeg.master of edx-platform (and earlier). Is that OK? Or does master of this repo need to support older versions of edx-platform?

   I don't think we install this from the master branch, but in the README we ask the user to install from release branch so merging this in the master is okay until we push a new release out through release branch.

3. Since this breaks Nutmeg and earlier and in edx-platfrom we don't specify the version in base.in and recompiling the requirements might update the version in them and it will start breaking e.g. see this PR that update the versions from base.in.

   • So, If we push a new release we need to think about the possibility of updating the requirement in Nutmeg or older releases to keep them safe from breaking.

Also, Let's hear @pdpinch 's thoughts on this.

[pdpinch]

I don't mind making breaking changes -- we've done it before -- but we should consider the version number more carefully.

It is also a problem that future nutmeg builds would fail. Maybe @kdmccormick has some thoughts on that. Should the nutmeg branch in the version of this xBlock? I think there's some precedence with BTR for that approach.

[kdmccormick]

@pdpinch @arslanashraf7 Looking at base.txt on edx-platform's open-release/nutmeg.master branch, we pin edx-sga to 0.18.0. In fact, now that I've thought about it more, I remember that we pin every direct edx-platform dependency in base.txt, and only we recompile those pins on edx-platform's master branch. The pins in the open-release/* branches are only ever upgaded manually and on a per-need basis.

So, unless someone manually changes the pin, Nutmeg will always install edx-sga==0.18.0. If they tried to change the pin, edx-platform CI tests would fail and prevent them from doing so. Given that, I believe there should be no risk in releasing a new version of this package that's backwards-incompatible.

in the README we ask the user to install from release branch so merging this in the master is okay until we push a new release out through release branch.

These instructions from the README do tell the user to install from the release branch. However, since edx-platform installs edx-sga as part of its base requirements now, I don't think the "Add SGA to the platform requirements" step is necessary any more. If you remove that step, then the reader will always have edx-sga installed at the appropriate compatible version, as specified in edx-platform's base.txt.

[pdpinch]

Thanks @kdmccormick that makes sense to me.

@arslanashraf7 we already have an issue to update the readme (#118), so I think it's OK to merge this and proceed with the release.

[arslanashraf7]

Looks good 👍 Thanks for creating this PR.

remember that we pin every direct edx-platform dependency in base.txt, and only we recompile those pins on edx-platform's master branch. The pins in the open-release/* branches are only ever upgaded manually and on a per-need basis.

@kdmccormick Thanks for your thoughts which seem very reasonable.

[kdmccormick]

Thanks all!