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

New project initialization #1

Merged
merged 15 commits into from
Oct 1, 2019
6 changes: 6 additions & 0 deletions .circleci/ci_django.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
db:
name: circleci_db
user: circleci
password: ''
host: localhost
port: 5432
46 changes: 46 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
version: 2
mhuyck marked this conversation as resolved.
Show resolved Hide resolved

jobs:
tests:
docker:
# Primary docker container where the tests will be run
- image: circleci/python:3.6.8

# Secondary docker container for database service
- image: postgres:11.4
environment:
POSTGRES_USER: circleci

working_directory: ~/py3-adage-backend

steps:
- checkout
- run:
name: Set up Postgres
command: |
sudo apt-get update --quiet
sudo apt-get upgrade --yes --quiet
sudo apt-get install postgresql-client --yes
createdb --host=localhost --username=circleci circleci_db
cp .circleci/ci_django.yml adage/adage/config.yml
- run:
name: Set up venv
command: |
mkdir -p ~/.venv
python3 -m venv ~/.venv/adage
- run:
name: Backend tests
working_directory: ~/py3-adage-backend/adage
command: |
source ~/.venv/adage/bin/activate
pip install pip --upgrade
pip install -r requirements.txt
python3 manage.py makemigrations
python3 manage.py migrate
python3 manage.py test

workflows:
version: 2
test:
jobs:
- tests
13 changes: 13 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
*~
*.pyc
*.sqlite3
*.json_cache

# Django config file
adage/adage/config.yml

# Adage-specific data files
data/gene_history
data/*.gene_info
data/bootstrap_cache_*
data/*_pickled_genesets
Empty file added adage/adage/__init__.py
Empty file.
22 changes: 22 additions & 0 deletions adage/adage/config_template.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Database configuration (required)
db:
name: database_name
user: db_user
password: not_secure
host: database_hostname
port: database_port_number
mhuyck marked this conversation as resolved.
Show resolved Hide resolved

# Django secret key (required for production server).
# Optional for dev server, default: "django_secret_key"
key: 'django_secret_key'

# Debug flag (optional, default: True)
debug: True

# Allowed hosts (optional, default: blank list)
allowed_hosts:
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
- localhost
- 127.0.0.1

# Directory that hosts static files (optional, default: "<BASE_DIR>/static"
static_root: '/home/ubuntu/www/static/'
148 changes: 148 additions & 0 deletions adage/adage/settings.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
"""
Django settings for adage project.

Generated by 'django-admin startproject' using Django 2.2.5.

For more information on this file, see
https://docs.djangoproject.com/en/2.2/topics/settings/

For the full list of settings and their values, see
https://docs.djangoproject.com/en/2.2/ref/settings/
"""

import os
import yaml

# Build paths inside the project like this: os.path.join(BASE_DIR, ...)
BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))

# Read secrets from YAML file
path = os.path.join(BASE_DIR, 'adage', 'config.yml')
with open(path) as read_file:
config = yaml.full_load(read_file)

# Quick-start development settings - unsuitable for production
# See https://docs.djangoproject.com/en/2.2/howto/deployment/checklist/

# SECURITY WARNING: keep the secret key used in production secret!
SECRET_KEY = config.get('secret_key', 'django_secret_key')

# SECURITY WARNING: don't run with debug turned on in production!
DEBUG = config.get('debug', True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: instead of a stern SECURITY WARNING comment in the code that nobody but a developer will see, should we make the codebase secure by default and make DEBUG default to False? To make things a little easier for developers, we could put a comment about setting the 'debug' config parameter to True in the config_template.yml file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, I realize this is boilerplate... just wondering if and when we want to uphold certain standards of deployability.

(Also: I know we had this hard-coded to True in adage-server. I'm trying to think through the steps toward our end goal explicitly and make sure we improve our final product this time based upon what we learned with our first effort.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In the default settings.py (generated by django-admin startproject command), this section was:

# SECURITY WARNING: don't run with debug turned on in production!
DEBUG = True

which is probably where the lines in adage came from. I found it a little annoying when deploying the production server, because I had to modify settings.py directly. That is why I put this option in config.yml now.

This is also the reason why the default is True here, because I try to keep it consistent with the original setting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm totally fine with defaulting it to True for now. It will be a while before we come to the point of deploying this in production. At the same time, that time delay is why I was thinking it might be good to change the default now so we don't forget. After the code review is done we tend to not revisit questions like this.

Is there a place for us to keep notes about things we want to remember to do before production deployment? If you prefer to leave the development default for now then maybe we should have a list of reminders about this and other things we know we need to come back to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhuyck, I created issue #5 to keep track of the config of production server. Please feel free to comment on it.


ALLOWED_HOSTS = config.get('allowed_hosts', [])

# Application definition

INSTALLED_APPS = [
'django.contrib.admin',
'django.contrib.auth',
'django.contrib.contenttypes',
'django.contrib.sessions',
'django.contrib.messages',
'django.contrib.staticfiles',
'rest_framework',
'django_filters',
'corsheaders',
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
'organisms',
'genes',
'analyze',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rename this unless you foresee any major trouble arising from that. What I didn't understand when building models initially is that these things are all objects, so the names should all be nouns. Also, analyze or its noun form analysis isn't necessarily the most useful name. I'm going to look through how this is used and see if I can think of something better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I simply copied this line from original Adage settings. analysis is one option, but it seems a little too generic. @cgreene, do you have any preference on the name of this Django app?

Copy link
Member

Choose a reason for hiding this comment

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

Is this app essentially the place where most of the endpoints live? It's too generic for me to remember what goes in it now. 😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgreene: Yes, you are right.

Copy link
Member

Choose a reason for hiding this comment

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

In a new django project, would this just be called adage? I think the django devs might have changed how apps were named by default a bit after this project was created.

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 Django's jargon, adage is the project's name, analyze, genes, organisms are App's name. The app's name doesn't really matter to the frontend. It only matters to backend development. So we can name it anything, but still we want it to be a reasonable one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really glad we're doing this review. This is the first time for this particular naming decision because the first time around actually predates the GreeneLab code review policy!

I jabbed the original developer in the ribs and asked "what were you thinking?" (We go way back.) He mumbled something about it seeming good at the time because we didn't really know what the data models were going to look like.

The naming is indeed generic because it encompasses everything. I wonder if it makes sense to split the backend into two Django "apps" because there is something of a logical split between the annotated "source data" (Samples, essentially, which we pulled from ArrayExpress queries, along with Experiment, and all of the SampleAnnotation models) and "ADAGE model data" (MLModel, Signature, Activity, and the rest). That separation of concerns would help our system design by giving a stronger core purpose to the two parts of the backend.

We should also keep in mind that the Django framework was not originally written for single page apps. Our goal here should be to make the best use of the capabilities it provides without getting too hung up on parts that are not relevant to us as we aim to build a solid REST API.

Maybe we are wandering outside the scope of this pull request. Does it make sense to resolve this question via an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Update: I suggest we follow the default Django 2.2 project layout @cgreene points out and dispense with the idea of a separate "app" (currently named analyze). The possibility of separate app modules that Django offers are very useful as reusable modules and they make sense for something portable like genes and organisms, but the bulk of the Adage API is probably inseparable from the rest of the back end code.

]

REST_FRAMEWORK = {
'PAGE_SIZE': 25,
'DEFAULT_PAGINATION_CLASS': 'rest_framework.pagination.LimitOffsetPagination',
'DEFAULT_FILTER_BACKENDS': ('django_filters.rest_framework.DjangoFilterBackend',),
}

MIDDLEWARE = [
'django.middleware.security.SecurityMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
'corsheaders.middleware.CorsMiddleware',
'django.middleware.common.CommonMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'django.middleware.clickjacking.XFrameOptionsMiddleware',
]

ROOT_URLCONF = 'adage.urls'

TEMPLATES = [
{
'BACKEND': 'django.template.backends.django.DjangoTemplates',
'DIRS': [],
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
'APP_DIRS': True,
'OPTIONS': {
'context_processors': [
'django.template.context_processors.debug',
'django.template.context_processors.request',
'django.contrib.auth.context_processors.auth',
'django.contrib.messages.context_processors.messages',
],
},
},
]

WSGI_APPLICATION = 'adage.wsgi.application'


# Database
# https://docs.djangoproject.com/en/2.2/ref/settings/#databases

DATABASES = {
'default': {
'ENGINE': 'django.db.backends.postgresql_psycopg2',
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
'NAME': config['db']['name'],
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
'USER': config['db']['user'],
'PASSWORD': config['db']['password'],
'HOST': config['db']['host'],
'PORT': config['db']['port'],
}
}


# Password validation
# https://docs.djangoproject.com/en/2.2/ref/settings/#auth-password-validators

AUTH_PASSWORD_VALIDATORS = [
{
'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator',
},
{
'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator',
},
{
'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator',
},
{
'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator',
},
]
mhuyck marked this conversation as resolved.
Show resolved Hide resolved


# Internationalization
# https://docs.djangoproject.com/en/2.2/topics/i18n/

LANGUAGE_CODE = 'en-us'

TIME_ZONE = 'America/New_York'
mhuyck marked this conversation as resolved.
Show resolved Hide resolved

USE_I18N = True

USE_L10N = True

USE_TZ = True


# Static files (CSS, JavaScript, Images)
# https://docs.djangoproject.com/en/2.2/howto/static-files/
# Use "python manage.py collectstatic" command to populate static directory
STATIC_ROOT = config.get('static_root', os.path.join(BASE_DIR, 'static'))
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
STATIC_URL = '/static/'

# CORS config
# https://pypi.org/project/django-cors-headers/
CORS_ORIGIN_ALLOW_ALL = True
CORS_URLS_REGEX = r'^/api/v1/.*$'
CORS_ALLOW_METHODS = ('GET', )
15 changes: 15 additions & 0 deletions adage/adage/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
"""adage URL Configuration"""

from django.contrib import admin
from django.urls import include, path
from rest_framework import routers
from analyze import views

router = routers.DefaultRouter()
router.register(r"mlmodels", views.MLModelViewSet)

urlpatterns = [
path('admin/', admin.site.urls),
path('api/v1/', views.api_root),
path('api/v1/', include(router.urls)),
mhuyck marked this conversation as resolved.
Show resolved Hide resolved
]
16 changes: 16 additions & 0 deletions adage/adage/wsgi.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
"""
WSGI config for adage project.

It exposes the WSGI callable as a module-level variable named ``application``.

For more information on this file, see
https://docs.djangoproject.com/en/2.2/howto/deployment/wsgi/
"""

import os

from django.core.wsgi import get_wsgi_application

os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'adage.settings')

application = get_wsgi_application()
Empty file added adage/analyze/__init__.py
Empty file.
3 changes: 3 additions & 0 deletions adage/analyze/admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from django.contrib import admin

# Register your models here.
5 changes: 5 additions & 0 deletions adage/analyze/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from django.apps import AppConfig


class AnalyzeConfig(AppConfig):
name = 'analyze'
Loading