Should we continue use of the FEATURES dict in edx-platform?

[robrap]

Ultimately this should result in an ADR and/or a DEPR, but I'm first creating an issue to initiate discussion on the topic.

Background, I've heard in the past that we should stop using the FEATURES dict, but I do not know all the reasons or the recommended alternative. As new settings are added, it would be great if there was clarity around this.

The following ADR on managing django settings mentions FEATURES, but it does not take any stance on this topic.

Open questions:

• Is there any important reason to stop using this dict, or is this just an unnecessary distraction?
• Is there really a problem using the dict, or have we resolved this by enabling overrides for individual settings in this special case dict?
• Should new settings be flattened as FEATURE_XXX?
• Is having feature flags in and outside this dict a consistency problem that we want to correct? Do we already have this problem?

If anyone with history on this topic could shed some additional light, that would be great.

[kdmccormick]

Hah, I recall having this conversation at an edX Arch Lunch when I was an intern in 2017. Nobody could name a reason for keeping a separate FEATURES dict, especially considering that boolean flags already existed outside of FEATURES at that point.

In any event, I'm in favor of flattening, either as FEATURE_XXX or even just as XXX (i.e., name the settings the same as the dict keys) if there weren't any conflicts.

[robrap]

Thanks @kdmccormick. I'll also note that https://docs.openedx.org/projects/edx-platform/en/latest/references/featuretoggles.html is a new feature that helps make it less important how consistent we are.

I like clarity that leads to consistency, so I think it would be useful (through DEPR and/or ADR) to state that the FEATURES dict is deprecated, if that is the decision.

Around naming, I think TOGGLE would be better than FEATURE, and could be an optional or recommended suffix that makes the setting name more clear.

Also, some use ENABLE, ENABLED, DISABLE, or DISABLED in the name. I find it makes reading code confusing, because we separately call an is_enabled() method (or the new is_disabled() method), and my brain has to do less mental gymnastics when I read XXX_TOGGLE.is_enabled() or XXX_TOGGLE.is_disabled().

Maybe the naming recommendations would be better as an edx-toggles how-to (or part of an existing how-to), and here we really just need to clarify whether or not to add to the dict.

[kdmccormick]

I like TOGGLE because it clarifies that the variable a toggle, without the awkwardness of ENABLE_BLAH.is_enabled()

[robrap]

1. I created Fix and improve toggle docs edx-toggles#303 to improve the docs.
2. I think maybe I could just add a comment next to the FEATURES dict about making new toggles top-level and pointing to the toggle docs for naming recommendations.

[robrap]

@kdmccormick: Some thoughts since we last wrote on the topic:

1. I realized that people often use ENABLE_ or DISABLE_ as a prefix to indicate the default value in the name, and I've learned how that can both be very convenient, and sometimes misleading. For example, an ENABLE_XXX toggle is typically defaulted off, and the toggle is used to enable the feature. Using the name TOGGLE_XXX loses this indication, and one would need to rely on docs or code to determine its default. This may be more time consuming, but should also be more accurate. [I'm thinking out loud here, and potentially re-convincing myself of the usefulness of this naming convention.]
2. People continue to add to the FEATURES dict while this issue remains undecided. I may be causing issues for people by bringing it up when no decision has been made. What would it take to make a decision, or should we just forget about this and let people to continue to use it as they wish? [Unfortunately, I don't have the time to take this on, so if it just takes resources to push this through, maybe it will just continue to stay put.]

[kdmccormick]

1. I perused lms/envs/common.py and found perhaps 1/3 of the ENABLE_ settings to default on, so I wouldn't consider that to be a naming convention worth preserving.
2. All it would take to decide is an edx-platform ADR--I can't imagine that there would be controversy. Actually enforcing it and migrating away from FEATURES would be a little tougher, but I bet we can figure something out (FEATURES = vars() 😈 ?)

I also don't have time to carry this out right now. @feanil , is Aximprovements looking for work? If so, is collapsing FEATURES something they'd be interested in?

[kdmccormick]

A little more detail on the migration path I'm imagining... we'd collapse all of FEATURES into the root of common.py. For backwards compatibility, we'd provide a magical FEATURES dict for a few releases that acted as a passthrough to the settings module:

class _Features(dict):
    def __init__(self, pass_through_to):
        self.pass_through_to = pass_through_to
    def __getitem__(self, key):
        return self.pass_through_to[key]
    def __setitem(self, key, value):
        warnings.warn(f"FEATURES is deprecated. Instead of overriding FEATURES['{key}'], just override {key}")
        self.pass_through_to[key] = value

FEATURES = _Features(pass_through_to=vars())

EDIT: Hm, this might not work, because pass_through_to may be bound to the namespace of common.py rather than the namespace of whatever settings module that is reading from and writing to FEATURES.

[robrap]

• With monkey-patching settings as a last resort, is this a solvable problem? I like it better than the linting solution I had in mind, which only stops the leak, but doesn't fix anything.
• If we wish to introduce a naming convention, any automated translation could account for what we wish them to be.
  • I also wonder if _TOGGLE would be better as a suffix to enable alpha sorting with other settings related to a feature area, rather than with all toggles being sorted together?

Examples:

• FEATURES['ENABLE_OAUTH2_PROVIDER'] => OAUTH2_PROVIDER_TOGGLE (drops ENABLE)
• FEATURES['CERTIFICATES_HTML_VIEW'] => CERTIFICATES_HTML_VIEW_TOGGLE

[robrap]

Linting would be simple if we just put an upper bound on the size of the dict. So, it is possible that we could decide, add linting, document a naming convention alternative, and iterate later with better automating the full transition when and if that gets prioritized.

[kdmccormick]

@robrap You're right, linting is totally feasible here just by inspecting the length or keys of the FEATURES dict.

[kdmccormick]

@robrap Regarding naming... I'm looking at common.py to see what other third-party apps do. It seems like the convention is to prefix with feature area, so I like that a lot (most of our own apps already do it too :). However, they don't suffix with _TOGGLE; rather, they just kinda describe the behavior that would occur if they were set to True. Some examples:

• WIKI_ENABLED
• WIKI_ANONYMOUS
• AWS_QUERYSTRING_AUTH
• CSRF_COOKIE_SECURE
• CORS_ORIGIN_ALLOW_ALL
• CORS_ORIGIN_INSECURE

Given that, I lean away from suffixing with _TOGGLE, or with anything else for that matter. That does still leave the awkwardness of MY_APP_ENABLED.is_enabled()... but could we work around that by defining __bool__ on the Toggle base class to equal is_enabled(), allowing all Toggles to be checked with:

if MY_APP_ENABLED:
   ... 

A nice property of this is that all toggles can be checked the same way, regardless of whether they are Django settings, Toggles wrapping Django settings, or Toggles wrapping Waffle Flags. For CourseWaffleFlags, we could even support a nice syntax like:

if MY_COURSE_SENSITIVE_APP_ENABLED.for_context(<course_key>):
   ...

where for_context creates a one-off Toggle instance that is scoped to <course_key>.

[robrap]

@kdmccormick: Good thoughts. I think your proposal is a reasonable alternative, or enhancement, to what is captured in openedx/edx-toggles#290. Yours is a more elegant solution from a readability perspective, as long as people don't get caught up wondering how it actually works.

[kdmccormick]

@robrap Oh, I didn't realize that there was edx-toggles work in flight--nice!

Yours is a more elegant solution from a readability perspective, as long as people don't get caught up wondering how it actually works.

It's the classic tension between Having A Simple API and Having A Simple Implementation :)

Alternatively, in the spirit of There's One Way To Do It, I'm also OK with just having is_enabled(), even if it's a mouthful sometimes, and a brain-twister other times. We could document that avoiding negative setting names is a best practice. For example, ENABLE_LEGACY_EXPERIENCE is superior to DISABLE_NEW_EXPERIENCE.

Anyway, that's a lot of opinions from me, all loosely held. In the end, my only strong opinion is that we need to have fewer layers between what the site operator configures and what the application sees.