Skip to content

Restructure#75

Merged
marcosmilzo merged 12 commits into
devfrom
restructure
May 26, 2018
Merged

Restructure#75
marcosmilzo merged 12 commits into
devfrom
restructure

Conversation

@mkao006

@mkao006 mkao006 commented May 17, 2018

Copy link
Copy Markdown
Contributor
  • Moved all the controller package to the thereadingmachine directory.
  • Added sentiment traffic light
  • Minor modification to the environment and configurations

mkao006 added 12 commits April 26, 2018 04:20
* added a sentiment outlook to the dashboard
* Changed the color pallete
* added computation of the last sentiment percentile
* moved all controller to thereadingmachine/
* added path to thereadingmachine for python
* collected all variables and parameters in parameter.py and environment.py
* added a sentiment outlook to the dashboard
* Changed the color pallete
* added computation of the last sentiment percentile
* moved all controller to thereadingmachine/
* added path to thereadingmachine for python
* collected all variables and parameters in parameter.py and environment.py
This eliminates the manula update everytime we do testing and avoid resetting the database.
@mkao006 mkao006 requested review from marcosmilzo and mrpozzi May 17, 2018 10:27

@mrpozzi mrpozzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple of comment, but all in all much cleaner, good job

Comment thread deploy.md
```

You can view the repo on [Docker Hub](https://hub.docker.com/r/thereadingmachine/thereadingmachine/)
You can view the repo on [Docker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unwanted newline

Comment thread set_env_var.sh
export WEBAPP_DIR=$PROJECT_HOME/webapp
export WEBAPP_PLOT_DIR=$WEBAPP_DIR/templates/static/plotly
export PYTHONPATH=$PROJECT_HOME:$PYTHONPATH
export AIRFLOW_START_DATE=`date +%Y-%m-%d`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setting up the start date as today's date may have some unwanted effect: namely if you schedule tasks that take a long time the dag may never actually run. It's not encouraged practice in Airflow. Consider using a fixed start date or adding a buffer (like 1/2 days before today's date)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Not sure what you meant by it will never run. Why would the dag never run if a process takes a long time?
  2. It is true that the start date should not be dynamic in the dag file, however, the set_env_var.sh file is merely a configuration which can be changed. Generally, you will have multiple configuration file for testing, staging and deployment. This avoids having to change the dag file every single time we deploy, which I have done multiple time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a matter of code quality, besides having a variable parameter impacts reproducibility.

@@ -3,55 +3,28 @@
import pandas as pd

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Didn't we decide to drop the lstm process?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Dropped from the process (i.e. pipeline) but we don't have to drop it from the code.

@marcosmilzo marcosmilzo merged commit 54123d2 into dev May 26, 2018
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