Skip to content

Conversation

@c-w
Copy link
Contributor

@c-w c-w commented Aug 7, 2018

The interface to the algorithm Docker is as follows:

  • The container is directly invoked docker run the_container ...
  • The first and only argument to the container is the path to the image to be analyzed.
  • The container prints a JSON object {"faceVectors":[[...],[...],...]} to stdout.

See #3

from sqlalchemy.orm import sessionmaker
from sqlalchemy import create_engine
from ..log import get_logger
from ..settings import (MYSQL_USER, MYSQL_PASSWORD, MYSQL_DATABASE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In previous PRs, ..imports were replaced with module.module.module. Which should we choose for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix up all the imports in a follow up PR, keeping local consistency for now. In general, absolute imports are preferred since they are more explicit.

as Serializer, BadSignature, SignatureExpired)
from .database_manager import DatabaseManager
from .database_manager import get_database_manager
from ..settings import TOKEN_EXPIRATION, TOKEN_SECRET_KEY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In previous PRs, ..imports were replaced with module.module.module. Which should we choose for consistency?

self.engine.dispose()


@lru_cache(maxsize=1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the purpose of this decorator (maxsize=1, on a function with no arguments) to treat the DatabaseManager class as a singleton? If so, is this a better way than explicitly making the class a singleton, so it is not accidentally misused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the standard/stdlib way to create a singleton.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I foresee with this method is that I could still make as many different instantiations of DatabaseManager as I would like. If we made the class an actual singleton (https://www.python-course.eu/python3_metaclasses.php) (could use metaclasses as the link describes, but not totally necessary), then this problem would be alleviated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In dynamic languages you can always shoot yourself in the foot if you want to... it's just about making "doing the right thing" easy. Ideally the design would be changed to avoid the need for the singleton (e.g. currently there are lots of nested sessions etc flying around) but that's a bigger change than I'm willing to make right now.

@c-w c-w merged commit 9a2693b into master Aug 8, 2018
@c-w c-w deleted the docker branch August 8, 2018 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants