girder / girder

A data management platform for the web, developed by Kitware
http://girder.readthedocs.io
Apache License 2.0
429 stars 177 forks source link

Remove side effects from Girder imports #2386

Open mgrauer opened 7 years ago

mgrauer commented 7 years ago

Girder.events is a good example

danlamanna commented 7 years ago

This is related to the plugin refactor @jbeezley is working on.

mgrauer commented 6 years ago

Question of what to do with decorators that have side effects, e.g. validation decorators.

Needs further clarification.

jeffbaumes commented 6 years ago

@mgrauer @danlamanna do we consider this resolved with pip installable plugins? If there is more to do, needs a concrete scope and champion or it may need to be removed from the 3.0 release milestone.

zachmullen commented 5 years ago

TODO: enumerate all the places we want to squash them out before we can consider this closed

kotfic commented 5 years ago

With the following script parse.py

#!/usr/bin/env python
import ast
import sys

def is_import(expr):
    return type(expr) in [
        ast.Import,
        ast.ImportFrom]

def is_def(expr):
    return type(expr) in [
        ast.FunctionDef,
        ast.ClassDef]

def is_top_level_comment(expr):
    return type(expr) == ast.Expr and \
        type(expr.value) == ast.Str

def is_simple_assignment(expr):
    if type(expr) == ast.Assign:
        try:
            # Try to execute the assignment, if it throws a name error
            # it isn't a simple assignment.
            eval(compile(ast.Module(body=[expr]), '', 'exec'))
            return True
        except NameError:
            pass
    return False

predicates = [
    is_import,
    is_def,
    is_top_level_comment,
    is_simple_assignment
]

if __name__ == '__main__':
    with open(sys.argv[1], 'r') as fh:
        code = fh.read()

    tree = ast.parse(code)
    code = code.split('\n')

    for expr in tree.body:
        def is_sideeffect():
            for f in predicates:
                if f(expr):
                    return False
            return True

        if is_sideeffect():
            print('{}|{}|{}|{}'.format(sys.argv[1], expr.lineno, type(expr).__name__, code[expr.lineno -1]))

I have run this command in the girder/girder directory:

find . -type f -name "*.py" | parallel /tmp/parse.py

To generate this list of top level side effects. Some of these are harmless:

file name lineno type code
./events.py 337 Assign daemon = ForegroundEventsDaemon()
./utility/filesystem_assetstore_adapter.py 44 Assign DEFAULT_PERMS = stat.S_IRUSR stat.S_IWUSR
./external/mongodb_proxy.py 29 Try try:
./external/mongodb_proxy.py 35 Assign EXECUTABLE_MONGO_METHODS = get_methods(pymongo.collection.Collection,
./wsgi.py 24 Expr cherrypy.config.update({'engine.autoreload.on': False,
./wsgi.py 26 Expr cherrypy.config['server'].update({'cherrypy_server': False,
./wsgi.py 30 Assign girder._quiet = True # This means we won't duplicate messages to stdout/stderr
./wsgi.py 31 Assign _formatter = girder.LogFormatter('[%(asctime)s] %(levelname)s: %(message)s')
./wsgi.py 32 Assign _handler = cherrypy._cplogging.WSGIErrorHandler()
./wsgi.py 33 Expr _handler.setFormatter(_formatter)
./wsgi.py 34 Expr girder.logger.addHandler(_handler)
./wsgi.py 37 Expr server.setup()
./wsgi.py 38 Assign application = cherrypy.tree
./wsgi.py 40 Expr cherrypy.server.unsubscribe()
./wsgi.py 41 Expr cherrypy.engine.start()
./utility/mail_utils.py 192 Assign _templateDir = os.path.join(PACKAGE_DIR, 'mail_templates')
./utility/mail_utils.py 193 Assign _templateLookup = TemplateLookup(directories=[_templateDir], collection_size=50)
./utility/mail_utils.py 194 Expr events.bind('_sendmail', 'core.email', _sendmail)
./utility/ziputil.py 42 Try try:
./utility/assetstore_utilities.py 27 Assign _assetstoreTable = {
./utility/search.py 100 Expr addSearchMode('text', partial(_commonSearchModeHandler, mode='text'))
./utility/search.py 101 Expr addSearchMode('prefix', partial(_commonSearchModeHandler, mode='prefix'))
./utility/hash_state.py 31 Assign _HASHLIB_INLINE_EVP_STRUCT = _ver < (2, 7, 13) or (_ver >= (3,) and _ver < (3, 5, 3))
./utility/hash_state.py 104 Expr _HashInfo(
./utility/hash_state.py 111 Expr _HashInfo(
./utility/hash_state.py 118 Expr _HashInfo(
./utility/hash_state.py 125 Expr _HashInfo(
./utility/hash_state.py 132 Expr _HashInfo(
./utility/hash_state.py 139 Expr _HashInfo(
./utility/system.py 215 Assign cherrypy.tools.status = StatusMonitor()
./utility/system.py 216 Expr cherrypy.config.update({"tools.status.on": True})
./utility/progress.py 104 Assign noProgress = ProgressContext(False)
./utility/_cache.py 25 Expr register_backend('cherrypy_request', 'girder.utility._cache', 'CherrypyRequestBackend')
./utility/_cache.py 30 Assign cache = make_region(name='girder.cache').configure(backend='dogpile.cache.null')
./utility/_cache.py 31 Assign requestCache = make_region(name='girder.request').configure(backend='dogpile.cache.null')
./utility/_cache.py 36 Assign rateLimitBuffer = make_region(name='girder.rate_limit')
./utility/server.py 34 With with open(os.path.join(os.path.dirname(file), 'error.mako')) as f:
./utility/path.py 32 Assign NotFoundException = ResourcePathNotFound
./utility/init.py 34 Try try:
./cli/build.py 34 If if not six.PY3:
./cli/build.py 37 Assign _GIRDER_BUILD_ASSETS_PATH = resource_filename('girder', 'web_client')
./init.py 37 Assign VERSION['apiVersion'] = version
./init.py 41 Assign auditLogger = logging.getLogger('girder_audit')
./init.py 42 Expr auditLogger.setLevel(logging.INFO)
./init.py 43 Assign logger = logging.getLogger('girder')
./init.py 44 Expr logger.setLevel(logging.DEBUG) # Pass everything; let filters handle level-based filtering
./init.py 45 Expr config.loadConfig() # Populate the config info at import time
./init.py 291 Assign logprint.info = functools.partial(logprint, level=logging.INFO, color='info')
./init.py 292 Assign logprint.warning = functools.partial(
./init.py 294 Assign logprint.error = functools.partial(
./init.py 296 Assign logprint.success = functools.partial(
./init.py 298 Assign logprint.critical = functools.partial(
./init.py 300 Assign logprint.debug = logprint
./init.py 301 Assign logprint.exception = functools.partial(
./api/describe.py 41 If if six.PY3:
./api/describe.py 53 Assign API_VERSION = constants.VERSION['apiVersion']
./api/rest.py 53 Assign _MONGO_CURSOR_TYPES = (MongoProxy, pymongo.cursor.Cursor, pymongo.command_cursor.CommandCursor)
./api/rest.py 1258 Assign _sharedContext = Resource()
./api/v1/system.py 44 Assign ModuleStartTime = datetime.datetime.utcnow()
./api/docs.py 25 Assign models = collections.defaultdict(dict)
./api/docs.py 28 Assign routes = collections.defaultdict(
./main.py 23 Try try:
./main.py 37 If if name == 'main':
./constants.py 27 Assign PACKAGE_DIR = os.path.dirname(os.path.abspath(file))
./constants.py 28 Assign ROOT_DIR = os.path.dirname(PACKAGE_DIR)
./constants.py 29 Assign LOG_ROOT = os.path.join(os.path.expanduser('~'), '.girder', 'logs')
./constants.py 51 Try try:
./constants.py 58 Assign STATIC_PREFIX = os.path.join(sys.prefix, 'share', 'girder')
./constants.py 59 Assign STATIC_ROOT_DIR = os.path.join(STATIC_PREFIX, 'static')
./constants.py 304 Expr TokenScope.describeScope(
./constants.py 308 Expr TokenScope.describeScope(
./constants.py 311 Expr TokenScope.describeScope(
./constants.py 315 Expr TokenScope.describeScope(
./constants.py 320 Expr TokenScope.describeScope(
./constants.py 323 Expr TokenScope.describeScope(
./constants.py 326 Expr TokenScope.describeScope(
./constants.py 329 Expr TokenScope.describeScope(
./constants.py 332 Expr TokenScope.describeScope(
danlamanna commented 5 years ago

The side effects in _cache.py are creating dumb objects that do all their real initialization (network/disk IO) at app start time, rather than import time - so I think they can stay.

wsgi.py is intentionally designed for the side effect of running a WSGI app, so it can also stay imo.

jbeezley commented 5 years ago

That doesn't catch side-effects resulting from decorators though. For example, anything using @validator is registering symbols on a module level global in setting_utilities.py.