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
Add Integration Tests #326
Conversation
Minimum allowed coverage is Generated by 🐒 cobertura-action against 563b9e7 |
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.
My comments on what I did below. Merge #323 before this one.
- uses: actions/checkout@v3 | ||
with: | ||
path: buildstockbatch | ||
- uses: actions/checkout@v3 | ||
with: | ||
repository: NREL/resstock | ||
path: resstock | ||
ref: develop |
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.
Checking out both repos
repository: NREL/resstock | ||
path: resstock | ||
ref: develop | ||
- uses: actions/setup-python@v4 |
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 generally tried to update to the latest versions of all the actions.
on: [pull_request] | ||
on: | ||
push: | ||
branches: | ||
- develop | ||
pull_request: | ||
types: | ||
- synchronize | ||
- opened |
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.
Doing these checks on merge into develop as well on @shorowit's recommendation.
- name: Download weather | ||
run: | | ||
mkdir weather | ||
cd weather | ||
wget --quiet https://data.nrel.gov/system/files/156/BuildStock_TMY3_FIPS.zip |
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.
All the testing files use the same batch of weather files. This keeps it from being downloaded 4 times.
@pytest.mark.parametrize("project_filename", [ | ||
resstock_directory / "project_national" / "national_baseline.yml", | ||
resstock_directory / "project_national" / "national_upgrades.yml", | ||
resstock_directory / "project_testing" / "testing_baseline.yml", | ||
resstock_directory / "project_testing" / "testing_upgrades.yml", | ||
], ids=lambda x: x.stem) |
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 testing each of the project yaml files in resstock.
# Get the number of upgrades | ||
n_upgrades = len(batch.cfg.get("upgrades", [])) | ||
# Limit the number of upgrades to 2 to reduce simulation time | ||
if n_upgrades > 2: | ||
batch.cfg["upgrades"] = batch.cfg["upgrades"][0:2] | ||
n_upgrades = 2 | ||
|
||
# Modify the number of datapoints so we're not here all day. | ||
if n_upgrades == 0: | ||
n_datapoints = 4 | ||
else: | ||
n_datapoints = 2 | ||
batch.cfg["sampler"]["args"]["n_datapoints"] = n_datapoints |
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 don't want to spend forever running these ResStock runs. I just want to make sure the machinery works, so I'm limiting the number of upgrades and datapoints to keep the CI running quickly.
local_weather_file = resstock_directory.parent / "weather" / batch.cfg["weather_files_url"].split("/")[-1] | ||
if local_weather_file.exists(): | ||
del batch.cfg["weather_files_url"] | ||
batch.cfg["weather_files_path"] = str(local_weather_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.
It'll check if the weather is found locally and swap that in instead. Since we downloaded it earlier to the appropriate place it should just be able to unzip it.
create_eagle_env.sh
Outdated
conda create -y --prefix "$MY_CONDA_PREFIX" -c conda-forge "pyarrow>=7.0.0" "python=3.9" "numpy>=1.20.0" "pandas>=1.0.0,!=1.0.4" "dask>=2022.10.0" "distributed>=2021.5" ruby | ||
source deactivate | ||
source activate "$MY_CONDA_PREFIX" | ||
mamba create -y --prefix "$MY_CONDA_PREFIX" -c conda-forge "python=3.10" "pyarrow" "numpy" "pandas" "dask>=2022.10.0" "distributed" "ruby" | ||
conda deactivate | ||
conda activate "$MY_CONDA_PREFIX" | ||
which pip | ||
if [ $DEV -eq 1 ] | ||
then | ||
pip install --ignore-installed -e . | ||
pip install --no-cache-dir --ignore-installed -e . | ||
else | ||
pip install --ignore-installed . | ||
pip install --no-cache-dir --ignore-installed . |
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.
These are changes from #323. I branched from that branch for this work. They will disappear once that is merged.
assert (out_path / "results_csvs" / f"results_up{upgrade_id:02d}.csv.gz").exists() | ||
assert (ts_pq_path / "_common_metadata").exists() | ||
assert (ts_pq_path / "_metadata").exists() | ||
|
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.
Do all these asserts check to see if all the simulations failed? The machinery might work, but there should be some real results.
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. It does check that timeseries files exist for each simulation, but I will also add something to open the results parquet file and ensure they all succeeded.
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.
Checking to see "all" succeeded seems robust, but we might accidentally sample a building that will fail in OpenStudio-HPXML. The current failure rate seems low (~0.1%) enough. At least 1 successful run might be okay.
CI time goes from about 3 min to 15 min here? |
That's what happens when you actually start running simulations. I'm doing this for all versions of python we support. I may consider just doing it for the latest version, but it all happens in parallel so I don't see a problem with it. |
df.to_csv(gf, index=True, line_terminator='\n') | ||
df.to_csv(gf, index=True, lineterminator='\n') |
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.
👍
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.
@nmerket : It looks like the tests are working. It makes sense that the CI time increased a little. I do like the parallelization. Unless you want to do anything else with this PR, I am happy with the functionality.
Fixes #318
Pull Request Description
This adds some integration tests where we checkout the lastest ResStock and run small versions of each of the testing projects in ResStock.
Checklist
Not all may apply
minimum_coverage
in.github/workflows/ci.yml
as necessary.Update validation for project config yaml file changesUpdate existing documentation