[BB-933] Add pluggable_override

[Agrendalath]

Description:

This adds a new extension point - a pluggable_override decorator that allows overriding any function or method by pointing to its alternative implementation in settings.

This ticket is a part of edx/edx-platform#21433 and should be merged before that one.

JIRA:

OSPR-3801

Dependencies:

edx/edx-platform#21433

Merge deadline:

Before edx/edx-platform#21433.

Installation instructions:

Listed in edx/edx-platform#21433.

Testing instructions:

Listed in edx/edx-platform#21433.

Reviewers:

• @bradenmacdonald
• @nasthagiri
• @nedbat

Merge checklist:

• All reviewers approved
• CI build is green
• Version bumped
• Changelog record added
• Documentation updated (not only docstrings)
• Commits are squashed

Post merge:

• Create a tag
• Check new version is pushed to PyPi after tag-triggered build is
  finished.
• Delete working branch (if not needed anymore)

[openedx-webhooks]

Thanks for the pull request, @Agrendalath! I've created OSPR-5033 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[regisb]

Hi @Agrendalath! While I understand the point of this PR, it makes me worried about its consequences. I'm afraid it will add an extra easy-to-add but hard-to-maintain extension point everywhere @pluggable_override is being used.

If I understand correctly, using the @pluggable_override decorator on a function would be similar to the following pattern:

from django.conf import settings
from django.utils.module_loading import import_string

custom_func = settings.get("SOMESETTINGNAME", default_func)
if isinstance(custom_func, str):
    custom_func = import_string(custom_func)

(Ironically, I recently wrote this code for an xblock feature)

The main difference with this piece of code is that the custom_func would receive the default_func as its first argument -- which is kind of a strange design pattern (IMHO). In OOP, a more conventional approach would be to use inheritance and call the original function with super(). Basically, you are implementing here function-based inheritance, which I find weird.

Of course, the alternative would be to use monkey-patching, which comes with its own set of issues.

To summarize, I'm not opposed to this, but I'd like to make sure we are aware of the consequences: adding @pluggable_override anywhere introduces an extension point at the level of the Python API. This decorator will make it extremely easy to add extension points, but then these extension points will have to be documented and maintained.

In any case, if we do decide to move forward with this, the @pluggable_override decorator will have to be documented in edx-django-utils.

[bradenmacdonald]

@regisb

I'm afraid it will add an extra easy-to-add but hard-to-maintain extension point

This decorator will make it extremely easy to add extension points, but then these extension points will have to be documented and maintained.

I have some of the same concerns, but don't really know of a better alternative for allowing operators to customize more obscure parts of the platform. (The use cases we at OpenCraft have had for this PR so far are things like customizing XBlock unit icons seen in the LMS, and requiring login to view certificates - things that we're fairly sure are idosyncratic requirements.) We have a few choices: refuse such customizations entirely, fork the platform and maintain code drift (we avoid this), merge these idiosyncratic features into the codebase (makes the code more complex for all, benefits few), or use this approach so that at least there is an API to support such customizations without requiring operators to fork or modify the platform.

To me, this seems like the best option, giving people options for customization via a straighforward API, and keeping the custom code out of the core platform. If you have any suggestions on better ways to make the platform more customizable and flexible like this, please share.

One last note: as you can see in #21433 where we add this to the extension_points.rst documention, we consider this a "Trial" so see how effective and maintainable this sort of extension point is.

these extension points will have to be documented

Yes, I introduced extension_points.rst for exactly such a reason - we can include a list in there of all the supported pluggable_override points.

the custom_func would receive the default_func as its first argument -- which is kind of a strange design pattern (IMHO). In OOP, a more conventional approach would be to use inheritance and call the original function with super(). Basically, you are implementing here function-based inheritance, which I find weird.

Is it really a strange design pattern? It's conceptually pretty similar to a regular python method decorator, just used without the @ syntax. The general pattern is called Advice. (We could even adjust it so that it uses the exact same form as python decorators, returning a function instead of a value, and accepting the arguments via the returned function rather than alongside the default_func. I'm not sure that's any better though.)

This design allows chaining of multiple overrides, allows the custom function to defer to the original implementation / modify the arguments passed to the original implementation / modify the return value of the original implementation, while avoiding monkey patching. I think this approach is much better than monkey patching because one can clearly see the pluggable_override decorator in the code indicating that a function's behavior may be changed by a plugin and can see the corresponding django setting override, whereas monkey patching leaves no visible hint that the code is modified unless you know about the monkey patch or are running a debugger.

[regisb]

I have some of the same concerns, but don't really know of a better alternative for allowing operators to customize more obscure parts of the platform.

Me neither, so I agree this is probably the best possible solution. However, I'll insist again on the documentation of pluggable overrides:

1. Usage of the decorator should be documented in the docs/ of edx-django-utils. Eventually, I hope these docs will find their way to https://edx-django-utils.readthedocs.io/ (which is currently a 404).
2. We should find a way to automatically generate documentation for existing pluggable overrides. If they are supported, then they must be both maintained and documented.

I suggest you create a code annotation to document those entrypoints. This is the mechanism that we use to document feature toggles and settings in edx-platform: https://code-annotations.readthedocs.io/en/latest/getting_started.html

The annotation format would probably look like this:

# .. pluggable_override_setting: SOMESETTING
# .. pluggable_override_description: Describe here what this function does. 
@pluggable_override('SOMESETTING')
def some_function(*args, **kwargs):
    ...  

Those annotations would then be collected by an ad-hoc Sphinx extension: https://code-annotations.readthedocs.io/en/latest/sphinx_extensions.html Finally, these entrypoints would find their way to the edx-platform docs, similarly to the setting docs: https://github.com/edx/edx-platform/blob/master/docs/technical/settings.rst

If this sounds like a good approach to you, please let me know if I can help you getting familiar with code annotations.

Is it really a strange design pattern? It's conceptually pretty similar to a regular python method decorator, just used without the @ syntax. The general pattern is called Advice.

I didn't know this was a design pattern. The way I see it, a pluggable override is different from a python decorator in the sense that it requires the overridden function to be passed as argument. With pluggable overrides, I can imagine many cases where the overridden function will be an unused argument. For instance, the get_icon function from https://github.com/edx/edx-platform/pull/21433/files#diff-25e7d8c5c06c52d5218793104fcd5631 will probably not need calling its "parent". After all, the "child" function could just as well import its "parent" manually if it needs it: from foo.bar import parent_func.

I'm not hell-bent on this issue though -- much less than on the documentation. If other developers agree with your approach then I'm fine with it.

[natabene]

@Agrendalath I am late to the party, but thank you for your contribution. Please let me know once this is ready for our review, unless @regisb wants to review as a core committer.

[Agrendalath]

@natabene, since @nasthagiri and @nedbat were already looking into it on edx/edx-platform#21433 (from which this was extracted), would it be reasonable to get a second look from them?

[natabene]

@nedbat @nasthagiri Would you like to review this?

[nasthagiri]

@nedbat had offered to do so.

[Agrendalath]

@nedbat, did you have a moment to take a look at this?

[natabene]

@nedbat Do you think you could find some time to review this in the next 2 weeks?

[robrap]

@regisb: FYI: I am not going to do any doc updates at this time, but I did publish to readthedocs if anyone wants to clean up the docs: https://edx.readthedocs.io/projects/edx-django-utils/en/latest/. Note: we don't have a published standard for how to organize the docs, but edx-toggles index takes a stab at this that includes renaming some of the items in the left-hand nav to make it more clear.

[nedbat]

I don't understand why you use functools.partial here? Now prev_fn will behave exactly like f, so what is gained?

[Agrendalath]

That's odd - I cannot find any references to why we have added this, so I've just removed it and tested that it works correctly in a few different scenarios.

[nedbat]

This code could be simplified with:

override_functions = getattr(settings, override, ())

and then letting the for impl in override_functions: loop do nothing.

[Agrendalath]

Good idea, changed.

[nedbat]

Sorry it has taken me long to get to this.

@regisb does the latest annotation work have something for us to use to document these new "entrypoints"?

[nedbat]

@Agrendalath this will need to be rebased to move from Travis to GitHub Actions.

[nedbat]

@Agrendalath thanks for the detailed docstring. We should make sure it ends up in the published docs.

[regisb]

does the latest annotation work have something for us to use to document these new "entrypoints"?

The following steps need to be performed to document these entrypoints:

1. Define a new annotation format, in code_annotations/contrib/config.
2. Create a Sphinx extension that will collect these annotations, based on the featuretoggles and settings extensions.
3. Add a new documentation page in the edx-platform technical docs that will use this Sphinx extension.

[Agrendalath]

@nedbat, thank you for reviewing this. We have addressed your comments. Regarding the automated generation of the documentation, would it be reasonable to do this once the status of this feature is changed from "Trial" to "Adopted", and we have more overrides available? Currently, we have only one such override, proposed in edx/edx-platform#21433.
cc: @regisb, @bradenmacdonald

[nedbat]

Thanks for making the changes, and sorry for my slow pace. I definitely want to get this into the annotation toolchain, but we can merge this and keep making progress.

[openedx-webhooks]

@Agrendalath 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.