Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New project initialization #1
Changes from all commits
f4d4978
451451f
9e3e684
154c56c
9ffa230
8b809f7
4cb022e
b7afe24
7350d32
b957f72
7815bee
c01515f
d2a1295
12e9202
fdf6154
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
inadage-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.)There was a problem hiding this comment.
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 bydjango-admin startproject
command), this section was: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 modifysettings.py
directly. That is why I put this option inconfig.yml
now.This is also the reason why the default is
True
here, because I try to keep it consistent with the original setting.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 formanalysis
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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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. 😭
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 withExperiment
, and all of theSampleAnnotation
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?
There was a problem hiding this comment.
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 likegenes
andorganisms
, but the bulk of the Adage API is probably inseparable from the rest of the back end code.