Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Make Most Dependencies Optional #911

Closed
swainn opened this issue Jan 20, 2023 · 5 comments · Fixed by #967
Closed

[FEATURE] Make Most Dependencies Optional #911

swainn opened this issue Jan 20, 2023 · 5 comments · Fixed by #967

Comments

@swainn
Copy link
Member

swainn commented Jan 20, 2023

Is your feature request related to a problem? Please describe.
Tethys has a lot of dependencies and most are not used in every installation. Provide a way to handle optional dependencies so we can create a "MiniTethys"

Describe the solution you'd like
Develop a function that can be used to import optional dependencies and handle failure gracefully when not installed. Provide a standardized error message.

@sdc50
Copy link
Member

sdc50 commented May 15, 2023

I've come up with a couple of approaches for this, and I'm interested in some feedback.

Approach one:

Create a optional_import function that will either return the module (if it's available to import) or a dummy object that stores the import error. The import error wouldn't be raised until some attribute of the module is accessed.

Example:

bcrypt = optional_import('bcrypt')

def generate_salt_string():
    salt = bcrypt.gensalt()
    return salt

generate_salt_string()
MissingOptionalDependency: Optional dependency "bcrypt" was not able to be imported because of the following error:
No module named 'bcrypt'.

Implementation Details:

class MissingOptionalDependency(ImportError):
    pass


class FailedImport:
    def __init__(self, module, import_error):
        self.module_name = module
        self.error = import_error

    def __getattr__(self, item):
        raise MissingOptionalDependency(
                f'Optional dependency "{self.module_name}" was not able to be imported because of the following error:\n'
                f'{self.error}.'
            )


def optional_import(module, error_message=None):
    try:
        return import_module(module)
    except ImportError as e:
        return FailedImport(module, e)

Approach 2:

Create a lazy_import function that will return a function to call to get the module or raise an error if it's not available. Note: The name is somewhat misleading because the module is still imported when lazy_import is called. The difference here is that when the module is needed, a context specific message can be passed to the function that is called to get the module.

Example:

get_bcrypt = lazy_import('bcrypt')

def generate_salt_string():
    bcrypt = get_bcrypt('Generating salt strings requires that the optional dependency "bcrypt" be installed.')
    salt = bcrypt.gensalt()
    return salt

generate_salt_string()
MissingOptionalDependency: Generating salt strings requires that the optional dependency "bcrypt" be installed.'

Implementation Details:

from importlib import import_module


class MissingOptionalDependency(ImportError):
    pass

class OptionalModule:
    def __init__(self, module_name):
        self.module_name = module_name
        self.module = None
        self.error = None
        try:
            self.module = import_module(self.module_name)
        except ImportError as e:
            self.error = e

    def __call__(self, error_message=None):
        if self.module:
            return self.module
        else:
            error_message = error_message or (
                f'Optional dependency "{self.module_name}" was not able to be imported because of the '
                f'following error:\n{self.error}.'
            )
            raise MissingOptionalDependency(error_message)


def lazy_import(module_name):
    return OptionalModule(module_name)

Approach 1 is a simpler approach that requires fewer code changes, but doesn't have the ability to provide context specific errors about what specific features are not available due to a missing dependency.

Approach 2 allows for more specific feedback to users, but would require a lot more code changes, and would be harder to enforce/maintain.

I'm leaning toward approach 1. @swainn @shawncrawley thoughts?

@sdc50
Copy link
Member

sdc50 commented May 15, 2023

This modification would allow importing specific attributes from a module (i.e. from x import y).

def optional_import(module, from_module=None, error_message=None):
    try:
        if from_module:
            return getattr(import_module(from_module), module)
        return import_module(module)
    except ImportError as e:
        return FailedImport(module, e)

y = optional_import('y', from_module='x')

@sdc50
Copy link
Member

sdc50 commented May 22, 2023

After discussion at scrum we decided to follow the first approach to make it as least intrusive as possible, but to also add a utility function to handle the exception and raise a custom exception.

bcrypt = optional_import('bcrypt')

def generate_salt_string():
    verify_import(bcrypt, error_message='Generating salt strings requires that the optional dependency "bcrypt" be installed.')
    salt = bcrypt.gensalt()
    return salt

generate_salt_string()

@sdc50
Copy link
Member

sdc50 commented Jun 6, 2023

Some dependencies (e.g. tethys_dataset_services) are used by some of the database models and impact the migrations. For example:

VALID_ENGINES = optional_import("VALID_ENGINES", from_module="tethys_dataset_services.valid_engines")

class DatasetService(models.Model):
    """
    ORM for Dataset Service settings.
    """

    # Define default values for engine choices
    CKAN = VALID_ENGINES["ckan"]
    HYDROSHARE = VALID_ENGINES["hydroshare"]

    # Define default choices for engine selection
    ENGINE_CHOICES = ((CKAN, "CKAN"), (HYDROSHARE, "HydroShare"))

    name = models.CharField(max_length=30, unique=True)
    engine = models.CharField(max_length=200, choices=ENGINE_CHOICES, default=CKAN)
...

I'm inclined to hardcode the values if the module is not available so that the migrations can be run. The only potential problem is if someone initially sets up a database without the dependency, and then later wants to add it (and it happens to have different values from what is hardcoded).

@sdc50
Copy link
Member

sdc50 commented Jun 8, 2023

I've gone through all of our dependencies and made as many as possible optional. This is the remaining list of dependencies that I consider to be required by Tethys:

dependencies:
  - python

  # system dependencies
  - pyopenssl
  - openssl

  # core dependencies
  - django=3.2.*
  - channels=3.*
  - daphne=3.*
  - setuptools_scm
  - pip
  - pyyaml
  - jinja2
  - requests # required by lots of things
  - bcrypt  # also required by channels, docker, daphne, condorpy

  # django plugin dependencies
  - django-bootstrap5
  - django-model-utils
  - django-guardian

@swainn swainn linked a pull request Jun 16, 2023 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants