Setting naming convention question for extending list or tuple

There is a setting named CELERY_IMPORTS. It is is set as a tuple, and includes some defaults we don’t want to clobber, but we’d like to be able to add to the list with a new setting. Do we have a naming convention for such a setting? I proposed CELERY_IMPORTS_EXTENDED. Thoughts?

Here is the relevant PR comment:
See fix: celery import upgrade by MaferMazu · Pull Request #29739 · openedx/edx-platform · GitHub

Thank you!

1 Like

I’m not quite sure if I understand what’s being asked, so I’m going to re-state the issue (and please correct me if I get this wrong).

  1. Celery requires a setting called CELERY_IMPORTS that can be any sequence of strings that represent modules to import when the worker starts.
  2. We define a setting called CELERY_IMPORTS as a tuple, and put some modules that must be there in order for the system to work.
  3. We want to define an environment variable in the YAML file that makes it clear that this value is not going to replace CELERY_IMPORTS, but will supplement it with additional modules that should be added for this particular installation. This will be named CELERY_IMPORTS_EXTENDED.

Code snippet:

CELERY_IMPORTS = tuple({*CELERY_IMPORTS, *ENV_TOKENS.get('CELERY_IMPORTS_EXTENDED', ())})

We do use the convention of EXTRA_ in a small number of cases, like EXTRA_MIDDLEWARE_CLASSES or XBLOCK_EXTRA_MIXINS. One small suggestion I would make is to make CELERY_IMPORTS a list and not a tuple, so that the code can read like:

CELERY_IMPORTS.extend(ENV_TOKENS.get('CELERY_IMPORTS_EXTENDED', []))

Another possibility would be to do something like:

CELERY_IMPORTS += ENV_TOKENS.get('CELERY_IMPORTS_EXTENDED`, (,))

We probably don’t want to splat it into an intermediate set though, because we would lose ordering guarantees, which might bite us in weird and unexpected ways in the future.

1 Like

Thanks @dave.

  1. Yes - you understood correctly.
  2. EXTRA sounds good. I think I like CELERY_EXTRA_IMPORTS. It allows CELERY to still be the leading word used in alphabetical ordering, and still reads ok grammatically. Let me know if you disagree. Thanks.
  3. Hopefully switch from tuple to list is simple enough, if this is the only usage: Search · CELERY_IMPORTS · GitHub

Thanks again.

1 Like

CELERY_EXTRA_IMPORTS sounds good to me. Good luck!

1 Like

What a good thread! :star_struck:

Is there a place where this stuff (or can be) documented for future situations?

1 Like

Good point. I don’t actually know where a tidbit like this would go. Comments in the settings file? @feanil: Thoughts?

There’s currently no place where a settings naming conventions are recorded. I would suggest a comment in the production.py settings file for now. The other place I was thinking of was a README.rst in the lms/envs folder.

@robrap any chance you can add a bit of doc in while you’re in that general vicinity?

  1. This is for an OSPR. I know the README.rst request is simple, but I personally probably won’t get to it soon.
  2. If anyone were to take this up, it would also be useful to point to code-annotations/documenting_django_settings.rst at master · openedx/code-annotations · GitHub.
  3. If anyone wants to move any bits of this discussion to the OSPR as an additional request to the author, that would also work. I don’t think they are in the forums.

PR for Review to add the documentation: docs: Add documentation for how to extend a setting. by feanil · Pull Request #30222 · openedx/edx-platform · GitHub

2 Likes

Thank you @robrap for starting this conversation and thank you all for your comments. I’m going to edit the PR. I’m glad this contributed more than I expected.

Have a great day :sun_behind_small_cloud: