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

Feature/benchmarks on windows #184

Merged
merged 17 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 25 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ name: CI
on:
# Triggers the workflow on push or pull request events but only for the main branch
push:
branches: [main]
branches: [ main ]
pull_request:
branches: [main]
branches: [ main ]

# Allows you to run this workflow manually from the Actions tab
workflow_dispatch:
Expand Down Expand Up @@ -69,6 +69,9 @@ jobs:
# ---------- Install GAMS

- name: Install GAMS
env:
GAMS_LICENSE: ${{ secrets.GAMS_LICENSE }}
if: ${{ env.GAMS_LICENSE != '' }}
run: |
curl https://d37drm4t2jghv5.cloudfront.net/distributions/44.1.0/linux/linux_x64_64_sfx.exe -o linux_x64_64_sfx.exe
chmod +x linux_x64_64_sfx.exe
Expand All @@ -81,17 +84,18 @@ jobs:
mkdir -p $HOME/.local/share/GAMS
echo "$GAMS_LICENSE" > $HOME/.local/share/GAMS/gamslice.txt
ls -l $HOME/.local/share/GAMS/
env:
GAMS_LICENSE: ${{ secrets.GAMS_LICENSE }}


# ---------- Run tool, check for regressions

- name: Run tool on all benchmarks
env:
GAMS_LICENSE: ${{ secrets.GAMS_LICENSE }}
if: ${{ env.GAMS_LICENSE != '' }}
working-directory: xl2times
# Use tee to also save the output to out.txt so that the summary table can be
# printed again in the next step.
# Save the return code to retcode.txt so that the next step can fail the action
# if run_benchmarks.py failed.
run: |
source .venv/bin/activate
export PATH=$PATH:$GITHUB_WORKSPACE/GAMS/gams44.1_linux_x64_64_sfx
Expand All @@ -101,6 +105,22 @@ jobs:
| tee out.txt; \
echo ${PIPESTATUS[0]} > retcode.txt)

- name: Run CSV-only regression tests (no GAMS license)
SamRWest marked this conversation as resolved.
Show resolved Hide resolved
env:
GAMS_LICENSE: ${{ secrets.GAMS_LICENSE }}
if: ${{ env.GAMS_LICENSE == '' }}
working-directory: xl2times
# Run without --dd flag if GAMS license secret doesn't exist.
# Useful for testing for (CSV) regressions in forks before creating PRs.
run: |
source .venv/bin/activate
export PATH=$PATH:$GITHUB_WORKSPACE/GAMS/gams44.1_linux_x64_64_sfx
(python utils/run_benchmarks.py benchmarks.yml \
--times_dir $GITHUB_WORKSPACE/TIMES_model \
--verbose \
| tee out.txt; \
echo ${PIPESTATUS[0]} > retcode.txt)

- name: Print summary
working-directory: xl2times
run: |
Expand Down
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ ground_truth/*
*.pyproj.*
speedscope.json
*.pkl
.venv/
.venv*/
benchmarks/
.idea/
.python-version
docs/_build/
docs/api/
40 changes: 40 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,46 @@ git commit --no-verify

See our GitHub Actions CI `.github/workflows/ci.yml` and the utility script `utils/run_benchmarks.py` to see how to run the tool on the DemoS models.

In short, use the commands below to clone the benchmarks data into your local `benchmarks` dir.
Note that this assumes you have access to all these repositories (some are private and
you'll have to request access) - if not, comment out the inaccessible benchmarks from `benchmakrs.yml` before running.

```bash
mkdir benchmarks
# Get VEDA example models and reference DD files
# XLSX files are in private repo for licensing reasons, please request access or replace with your own licensed VEDA example files.
git clone [email protected]:olejandro/demos-xlsx.git benchmarks/xlsx/
git clone [email protected]:olejandro/demos-dd.git benchmarks/dd/

# Get Ireland model and reference DD files
git clone [email protected]:esma-cgep/tim.git benchmarks/xlsx/Ireland
git clone [email protected]:esma-cgep/tim-gams.git benchmarks/dd/Ireland
```
Then to run the benchmarks:
```bash
# Run a only a single benchmark by name (see benchmarks.yml for name list)
python utils/run_benchmarks.py benchmarks.yml --verbose --run DemoS_001-all | tee out.txt

# Run all benchmarks (without GAMS run, just comparing CSV data)
python utils/run_benchmarks.py benchmarks.yml --verbose | tee out.txt
SamRWest marked this conversation as resolved.
Show resolved Hide resolved


# Run benchmarks with regression tests vs main branch
git branch feature/your_new_changes --checkout
# ... make your code changes here ...
git commit -a -m "your commit message" # code must be committed for comparison to `main` branch to run.
python utils/run_benchmarks.py benchmarks.yml --verbose | tee out.txt
```
At this point, if you haven't broken anything you should see something like:
```
Change in runtime: +2.97s
Change in correct rows: +0
Change in additional rows: +0
No regressions. You're awesome!
```
If you have a large increase in runtime, a decrease in correct rows or fewer rows being produced, then you've broken something and will need to figure out how to fix it.

SamRWest marked this conversation as resolved.
Show resolved Hide resolved

### Debugging Regressions

If your change is causing regressions on one of the benchmarks, a useful way to debug and find the difference is to run the tool in verbose mode and compare the intermediate tables. For example, if your branch has regressions on Demo 1:
Expand Down
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ requires = ["setuptools>=61.0.0", "wheel"]
build-backend = "setuptools.build_meta"

[tool.setuptools]
packages = ["xl2times"]
packages = ["xl2times", "utils"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this also install a utils package to the python (virtual)env if someone does pip install . in the root directory? Wondering if that might result in name clashes.

I was thinking of utils more as a collection of scripts for benchmarking/developing, and not as a package to be distributed. Do we need to include it in pyproject.toml? Happy to go with the best practice here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah apologies, this was a fix for a ModuleNotFound from a from utils.dd_to_csv import in run_benchmarks I added, but the right solution was to import dd_to_csv directly.
I'll revert this change in #186

But yes, this would have included utils in built wheels etc. I didn't realise this was meant to not be included.


[project]
name = "xl2times"
Expand All @@ -24,6 +24,7 @@ dependencies = [
"more-itertools",
"openpyxl >= 3.0, < 3.1",
"pandas >= 2.1",
"pyarrow"
SamRWest marked this conversation as resolved.
Show resolved Hide resolved
]

[project.optional-dependencies]
Expand Down
Empty file added utils/__init__.py
Empty file.
8 changes: 6 additions & 2 deletions utils/dd_to_csv.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,13 +216,17 @@ def convert_dd_to_tabular(
return


if __name__ == "__main__":
def main(arg_list: None | list[str] = None):
SamRWest marked this conversation as resolved.
Show resolved Hide resolved
args_parser = argparse.ArgumentParser()
args_parser.add_argument(
"input_dir", type=str, help="Input directory containing .dd files."
)
args_parser.add_argument(
"output_dir", type=str, help="Output directory to save the .csv files in."
)
args = args_parser.parse_args()
args = args_parser.parse_args(arg_list)
convert_dd_to_tabular(args.input_dir, args.output_dir, generate_headers_by_attr())


if __name__ == "__main__":
main()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we not need to pass in an arg_list here? (I haven't seen this idiom before)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sam, do you need to make this change here too?

Suggested change
main()
main(sys.argv[1:])

Loading
Loading