New project initialization #1

dongbohu · 2019-09-12T18:38:54Z

This PR includes the initialization files for adage backend in Python 3. Most of the files involved are boilerplate code. The files that should be reviewed include:

├── adage
│   ├── adage
│   │   ├── config_template.yml: template config file
│   │   ├── settings.py: django settings
│   │   ├── urls.py: URLs supported by current project  
│   ├── analyze
│   │   ├── models.py: copied from "adage" repo, converted to Python 3
│   │   ├── serializers.py: a simple DJango REST framework serialization demo
│   │   └── views.py: a simple DJango REST framework serialization demo
│   ├── genes
│   │   ├── models.py: copied from `django-genes`, converted to Python 3
│   ├── organisms
│   │   ├── models.py: copied from `django-organisms`, converted to Python 3
│   ├── requirements.txt: PyPI packages
├── deployment
│   ├── nginx.conf: a simple Nginx config file 
│   ├── run.sh: a summary of deployment steps
│   └── supervisor-adage.conf: config file for gunicorn daemon
├── .circleci
│   ├── ci_django.yml: Django config file for CircleCI 
│   ├── config.yml: CircleCI config

Here is a demo site that I built on AWS based on this PR:
https://py3-adage.greenelab.com/api/v1/

Right now it only supports machine learning model API (enabled by Django REST framework):
https://py3-adage.greenelab.com/api/v1/mlmodels/

dongbohu · 2019-09-13T15:37:53Z

@mhuyck: By the way, I copied genes and organisms from django-genes and django-organisms because the PyPI packages are still being used by tribe, which is running Python 2. When Tribe and Adage are both migrated to Python 3, we can factor out genes and organisms as PyPI packages that support Python 3.

mhuyck

I have lots to say here. You can maybe see the evolution of my thinking as I gradually remember what all of these parts are.

I think the majority of my notes are questions to help make sure I'm on the same page about where we are headed and how we plan to do deployment in this version. I think there are a relatively small number of specific code changes I'm suggesting and only one or two outright errors that I caught.

This is great work. Thank you for bringing us into the Python 3 and Django 2.2 future, @dongbohu!

mhuyck · 2019-09-19T20:02:04Z

adage/adage/settings.py

+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)


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.

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.)

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.

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.

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

adage/adage/config_template.yml

mhuyck · 2019-09-24T18:02:40Z

adage/adage/settings.py

+    'corsheaders',
+    'organisms',
+    'genes',
+    'analyze',


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.

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?

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. 😭

@cgreene: Yes, you are right.

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.

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.

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?

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.

adage/adage/settings.py

adage/analyze/models.py

.circleci/config.yml

dongbohu

@mhuyck: I think I've addressed most of your comments. To keep this PR from growing too big, I will add new issues to keep track of some questions that you raised. Please let me know if you have any other comments. Thanks.

adage/adage/config_template.yml

adage/adage/settings.py

mhuyck

I think we still need to decide how we're going to proceed with the analyze "app". We should either fold that code into the adage project now or document it as an issue to be fixed in an upcoming PR.

I'm also not clear on the resolution for where to put those regex lines I noted in adage/genes/models.py.

Everything else seems to be addressed.

dongbohu · 2019-10-01T18:08:39Z

@mhuyck: I have created a few new issues to keep track of the two questions that I haven't addressed:
https://github.com/greenelab/py3-adage-backend/issues
Please feel free to comment on them (and add new ones if necessary).

dongbohu · 2019-10-01T18:10:59Z

I also added a new line in settings.py because I realized that I missed the line of CORS middleware.

mhuyck · 2019-10-01T18:21:44Z

Looks good. Thanks @dongbohu!

dongbohu added 13 commits September 5, 2019 16:36

Initialize django 2.2 projects and apps

f4d4978

Add DRF files

451451f

Add static dir config and a deployment script

9e3e684

Add deployment script and config files

154c56c

Beautify yaml config file

9ffa230

Rename config file

8b809f7

circleci config

4cb022e

test ci

b7afe24

Add venv steps

7350d32

tweak django steps

b957f72

fix path

7815bee

fix django config filename

c01515f

update pip

d2a1295

dongbohu requested a review from mhuyck September 13, 2019 15:34

mhuyck requested changes Sep 27, 2019

View reviewed changes

dongbohu commented Sep 30, 2019

View reviewed changes

Address PR comments

12e9202

dongbohu mentioned this pull request Sep 30, 2019

Migrate "organisms" and "genes" apps to Python 3 #2

Open

mhuyck reviewed Oct 1, 2019

View reviewed changes

adage/adage/config_template.yml Show resolved Hide resolved

mhuyck reviewed Oct 1, 2019

View reviewed changes

adage/adage/settings.py Show resolved Hide resolved

mhuyck reviewed Oct 1, 2019

View reviewed changes

adage/adage/settings.py Show resolved Hide resolved

mhuyck reviewed Oct 1, 2019

View reviewed changes

dongbohu mentioned this pull request Oct 1, 2019

Rename (and divide) "analyze" django app #6

Closed

Add cors middleware

fdf6154

mhuyck approved these changes Oct 1, 2019

View reviewed changes

dongbohu merged commit 177bb38 into master Oct 1, 2019

dongbohu deleted the dhu/init branch October 1, 2019 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New project initialization #1

New project initialization #1

dongbohu commented Sep 12, 2019 •

edited

Loading

dongbohu commented Sep 13, 2019

mhuyck left a comment

mhuyck Sep 19, 2019

mhuyck Sep 19, 2019

dongbohu Sep 27, 2019

mhuyck Oct 1, 2019

dongbohu Oct 1, 2019

mhuyck Sep 24, 2019

dongbohu Sep 27, 2019

cgreene Sep 27, 2019

dongbohu Sep 27, 2019

cgreene Sep 28, 2019

dongbohu Sep 29, 2019

mhuyck Oct 1, 2019

mhuyck Oct 1, 2019

dongbohu left a comment

mhuyck left a comment

dongbohu commented Oct 1, 2019

dongbohu commented Oct 1, 2019

mhuyck commented Oct 1, 2019

New project initialization #1

New project initialization #1

Conversation

dongbohu commented Sep 12, 2019 • edited Loading

dongbohu commented Sep 13, 2019

mhuyck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongbohu left a comment

Choose a reason for hiding this comment

mhuyck left a comment

Choose a reason for hiding this comment

dongbohu commented Oct 1, 2019

dongbohu commented Oct 1, 2019

mhuyck commented Oct 1, 2019

dongbohu commented Sep 12, 2019 •

edited

Loading