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.

Sign up for GitHub

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

cruft removal attempt #1308

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
+51 −146
Open

cruft removal attempt #1308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ecfa304
cruft removal attempt #1
melange396 Oct 11, 2023
1d1f36a
move setup script contents inside Dockerfile (or at least try to)
melange396 Oct 11, 2023
6016c24
missed a pip\!
melange396 Oct 11, 2023
10c30cd
stealing a trick from the dev Makefile, hoping it works
melange396 Oct 11, 2023
bbdb34c
syntax error fix?
melange396 Oct 11, 2023
3ba08e4
documentation updates
melange396 Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 8 additions & 29 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,6 @@ jobs:
build:
runs-on: ubuntu-latest
steps:
- run: mkdir -p repos/undefx
- name: Checkout undefx/py3tester
uses: actions/checkout@v2
with:
repository: undefx/py3tester
path: repos/undefx/py3tester
- name: Checkout undefx/undef-analysis
uses: actions/checkout@v2
with:
repository: undefx/undef-analysis
path: repos/undefx/undef-analysis

- run: mkdir -p repos/delphi

- name: Checkoutcmu-delphi/operations
Expand All @@ -29,25 +17,10 @@ jobs:
with:
repository: cmu-delphi/utils
path: repos/delphi/utils
- name: Checkout cmu-delphi/github-deploy-repo
uses: actions/checkout@v2
with:
repository: cmu-delphi/github-deploy-repo
path: repos/delphi/github-deploy-repo
- name: Checkout THIS REPO
uses: actions/checkout@v2
with:
path: repos/delphi/delphi-epidata
- name: Checkout cmu-delphi/flu-contest
uses: actions/checkout@v2
with:
repository: cmu-delphi/flu-contest
path: repos/delphi/flu-contest
- name: Checkout cmu-delphi/nowcast
uses: actions/checkout@v2
with:
repository: cmu-delphi/nowcast
path: repos/delphi/nowcast

- name: Build docker images
run: |
Expand Down Expand Up @@ -80,11 +53,17 @@ jobs:

- name: Run Unit Tests
run: |
docker run --rm --network delphi-net --env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/tests
docker run --rm --network delphi-net \
--mount type=bind,source=./repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \
--mount type=bind,source=./repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment (non-blocking, probably be separate issue): we should move to having CI use the Makefile commands. It would help keep local testing identical with CI and would remove code duplication.

--env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/tests

- name: Run Integration Tests
run: |
docker run --rm --network delphi-net delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/integrations
docker run --rm --network delphi-net \
--mount type=bind,source=./repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \
--mount type=bind,source=./repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \
Copy link
Contributor

Choose a reason for hiding this comment

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

Question (non-blocking, separate issue): any reason why unit tests and integrations are separated? In the Makefile both are run together, using that command here would remove this Docker command duplication.

delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/integrations

- name: Clean Up
run: |
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/performance-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ jobs:
cd driver/repos/delphi
git clone https://github.com/cmu-delphi/operations
git clone https://github.com/cmu-delphi/utils
git clone https://github.com/cmu-delphi/flu-contest
git clone https://github.com/cmu-delphi/nowcast
cd ../../

cd ..
Expand Down
25 changes: 11 additions & 14 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,17 @@ $ curl "https://raw.githubusercontent.com/cmu-delphi/delphi-epidata/dev/dev/loca
You should now have the following directory structure:

```sh
├── driver
│ ├── .dockerignore -> repos/delphi/delphi-epidata/dev/local/.dockerignore
│ ├── Makefile -> repos/delphi/delphi-epidata/dev/local/Makefile
│ ├── repos
│ │ ├── pyproject.toml -> delphi/delphi-epidata/dev/local/pyproject.toml
│ │ ├── setup.cfg -> delphi/delphi-epidata/dev/local/setup.cfg
│ │ └── delphi
│ │ ├── delphi-epidata
│ │ ├── flu-contest
│ │ ├── github-deploy-repo
│ │ ├── nowcast
│ │ ├── operations
│ │ └── utils
```
└── driver
├── .dockerignore -> repos/delphi/delphi-epidata/dev/local/.dockerignore
├── Makefile -> repos/delphi/delphi-epidata/dev/local/Makefile
└── repos
├── pyproject.toml -> delphi/delphi-epidata/dev/local/pyproject.toml
├── setup.cfg -> delphi/delphi-epidata/dev/local/setup.cfg
└── delphi
├── delphi-epidata
├── operations
└── utils
```

and you should now be in the `driver` directory.
You can now execute make commands
Expand Down
7 changes: 7 additions & 0 deletions TODO
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
- for #1129 :
- grep for:
- py3test
- test_target
- and remove from:
- tests/*
- integrations/*
5 changes: 1 addition & 4 deletions deploy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,7 @@
"dst": "[[stasis]]/integrations/",
"match": "^.*\\.(py)$",
"recursive": true
},

"// run unit and coverage tests",
{"type": "py3test"}
Comment on lines -246 to -247
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removing this will prevent github-deploy-repo from running local tests during its deployment process

}

]
}
15 changes: 9 additions & 6 deletions dev/docker/python/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ RUN apt-get update && apt-get install -y r-base && Rscript -e "install.packages(

WORKDIR /usr/src/app

COPY repos repos
COPY repos/delphi/delphi-epidata/dev/docker/python/setup.sh .

RUN ln -s -f /usr/share/zoneinfo/America/New_York /etc/localtime && \
chmod -R o+r repos/ && \
bash setup.sh && \
pip install --no-cache-dir -r repos/delphi/delphi-epidata/requirements.api.txt -r repos/delphi/delphi-epidata/requirements.dev.txt
mkdir delphi && \
chmod -R o+r delphi/

COPY repos/delphi/operations/src delphi/operations
COPY repos/delphi/utils/src delphi/utils
COPY repos/delphi/delphi-epidata/src delphi/epidata
Copy link
Contributor

@dshemetov dshemetov Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Comment (non-blocking): this COPY command is later overwritten by a runtime mount between the same folders we run tests. Consider getting rid of this.

Thinking out loud (non-blocking): looks like we're dropping the top-level repos dir within the Dockerfile. Want to make sure we think through the consequences of this. AFAICT, previously we copied the entire local repos folder over into /usr/src/app/repos and then moved pieces over to /usr/src/app/delphi/ and /usr/src/app/undefx/. So it looks like we're skipping the intermediate step. So is there anything that still relies on that repos folder? The only thing I can find are the Docker testing commands that end with e.g. python -m pytest --import-mode importlib repos/delphi/delphi-epidata/integrations. But that is handled by the first mount command in those Docker commands:

--mount type=bind,source=./repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \

So the edit to my first sentence from this sequence of thoughts is: we're not dropping the repos dir, it just gets added by a mount at runtime.

Comment (non-blocking, separate issue): it's tough to mentally imagine the directory structure of Dockerfile when runtime mounts are involved (due to non-locality between Docker run command and the Dockerfile). Maybe we should add a comment at the end of this Dockerfile saying something like "at runtime, these additional directories are present".

Comment (non-blocking, separate issue): a future future solution to this non-locality is to make a unified docker-compose.yml file.

Copy link
Contributor

@dshemetov dshemetov Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Most of my review suggestions are addressed by #1315


COPY repos/delphi/delphi-epidata/requirements.*.txt .
RUN pip install --no-cache-dir -r requirements.api.txt -r requirements.dev.txt
27 changes: 0 additions & 27 deletions dev/docker/python/setup.sh
View file
Open in desktop

This file was deleted.

15 changes: 0 additions & 15 deletions dev/local/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@
# operations/
# delphi-epidata/
# utils/
# flu-contest/
# nowcast/
# github-deploy-repo/
# undefx/
# py3tester/
# undef-analysis/
#
# Leaves you in driver, the main workdir.
#
Expand All @@ -33,15 +27,6 @@ cd driver/repos/delphi
git clone https://github.com/cmu-delphi/operations
git clone https://github.com/cmu-delphi/delphi-epidata
git clone https://github.com/cmu-delphi/utils
git clone https://github.com/cmu-delphi/flu-contest
git clone https://github.com/cmu-delphi/nowcast
git clone https://github.com/cmu-delphi/github-deploy-repo
cd ../../

mkdir -p repos/undefx
cd repos/undefx
git clone https://github.com/undefx/py3tester
git clone https://github.com/undefx/undef-analysis
cd ../../

ln -s repos/delphi/delphi-epidata/dev/local/Makefile
Expand Down
25 changes: 0 additions & 25 deletions dev/local/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,40 +32,15 @@ packages =
delphi.epidata.server.endpoints
delphi.epidata.server.endpoints.covidcast_utils
delphi.epidata.server.utils
delphi.flu_contest
delphi.flu_contest
delphi.flu_contest.archefilter
delphi.flu_contest.covid
delphi.flu_contest.epicast
delphi.flu_contest.forecasters
delphi.flu_contest.hosp
delphi.flu_contest.main
delphi.flu_contest.uploads
delphi.flu_contest.utils
delphi.github_deploy_repo
delphi.github_deploy_repo.actions
delphi.nowcast
delphi.nowcast.experiments
delphi.nowcast.fusion
delphi.nowcast.obsolete
delphi.nowcast.sensors
delphi.nowcast.util
delphi.operations
delphi.operations.database_metrics
delphi.operations.screenshots
delphi.operations.screenshots.covidcast
delphi.utils
delphi.utils.geo
delphi.utils.obsolete
undefx.py3tester
undefx.undef_analysis

package_dir =
delphi.epidata = delphi/delphi-epidata/src
delphi.flu_contest = delphi/flu-contest/src
delphi.github_deploy_repo = delphi/github-deploy-repo/src
delphi.nowcast = delphi/nowcast/src
delphi.operations = delphi/operations/src
delphi.utils = delphi/utils/src
undefx.py3tester = undefx/py3tester/src
undefx.undef_analysis = undefx/undef-analysis
25 changes: 11 additions & 14 deletions docs/epidata_development.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,14 @@ $ curl "https://raw.githubusercontent.com/cmu-delphi/delphi-epidata/dev/dev/loca
You should now have the following directory structure:

```sh
├── driver
│ ├── .dockerignore -> repos/delphi/delphi-epidata/dev/local/.dockerignore
│ ├── Makefile -> repos/delphi/delphi-epidata/dev/local/Makefile
│ ├── repos
│ │ └── delphi
│ │ ├── delphi-epidata
│ │ ├── flu-contest
│ │ ├── github-deploy-repo
│ │ ├── nowcast
│ │ ├── operations
│ │ └── utils
└── driver
├── .dockerignore -> repos/delphi/delphi-epidata/dev/local/.dockerignore
├── Makefile -> repos/delphi/delphi-epidata/dev/local/Makefile
└── repos
└── delphi
├── delphi-epidata
├── operations
└── utils
```

and you should now be in the `driver` directory.
Expand Down Expand Up @@ -173,7 +170,7 @@ Then run the tests in a container based on that image:

```bash
docker run --rm delphi_python \
python3 -m undefx.py3tester.py3tester --color \
python3 -m pytest \
repos/delphi/delphi-epidata/tests
```

Expand Down Expand Up @@ -315,7 +312,7 @@ More concretely, you can run Epidata API integration tests like this:

```bash
docker run --rm --network delphi-net delphi_python \
python3 -m undefx.py3tester.py3tester --color \
python3 -m pytest \
repos/delphi/delphi-epidata/integrations
```

Expand Down Expand Up @@ -377,7 +374,7 @@ docker run --rm --network delphi-net \
--mount type=bind,source="$(pwd)"/repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \
--mount type=bind,source="$(pwd)"/repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \
delphi_python \
python3 -m undefx.py3tester.py3tester --color \
python3 -m pytest \
repos/delphi/delphi-epidata/integrations
```

Expand Down
14 changes: 4 additions & 10 deletions docs/new_endpoint_tutorial.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,10 @@ tree -L 3 .
```
.
└── repos
   ├── delphi
   │   ├── delphi-epidata
   │   ├── flu-contest
   │   ├── github-deploy-repo
   │   ├── nowcast
   │   ├── operations
   │   └── utils
   └── undefx
   ├── py3tester
   └── undef-analysis
   └── delphi
      ├── delphi-epidata
      ├── operations
      └── utils
```

# the data
Expand Down