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

adding validation of the number of options in the ApplyUpgrade measure #330

Merged
merged 7 commits into from Nov 30, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 58 additions & 0 deletions buildstockbatch/base.py
Expand Up @@ -14,8 +14,10 @@
import difflib
from fsspec.implementations.local import LocalFileSystem
import logging
from lxml import objectify
import os
import pandas as pd
import re
import requests
import shutil
import tempfile
Expand Down Expand Up @@ -261,6 +263,7 @@ def validate_project(project_file):
assert BuildStockBatchBase.validate_postprocessing_spec(project_file)
assert BuildStockBatchBase.validate_resstock_or_comstock_version(project_file)
assert BuildStockBatchBase.validate_openstudio_version(project_file)
assert BuildStockBatchBase.validate_number_of_options(project_file)
logger.info('Base Validation Successful')
return True

Expand Down Expand Up @@ -702,6 +705,61 @@ def validate_resstock_or_comstock_version(project_file):

return True

@staticmethod
def validate_number_of_options(project_file):
"""Checks that there aren't more options than are allowed in the ApplyUpgrade measure.

:param project_file: path to project file
:type project_file: str
:raises ValidationError: if there are more options defined than there are in the measure.
:return: whether validation passes or not
:rtype: bool
"""
cfg = get_project_configuration(project_file)
measure_xml_filename = os.path.join(
cfg["buildstock_directory"], "measures", "ApplyUpgrade", "measure.xml"
)
Comment on lines +719 to +721
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using the measure.xml because it exists in the same place in both ResStock and ComStock, and it's more straightforward and less error prone to parse xml from python than ruby code.

if os.path.exists(measure_xml_filename):
measure_xml_tree = objectify.parse(measure_xml_filename)
measure_xml = measure_xml_tree.getroot()
n_options_in_measure = 0
n_costs_per_option_in_measure = 0
for argument in measure_xml.arguments.argument:
m_option = re.match(r"^option_(\d+)$", str(argument.name))
if m_option:
option_number = int(m_option.group(1))
n_options_in_measure = max(option_number, n_options_in_measure)
m_costs = re.match(r"^option_(\d+)_cost_(\d+)_value", str(argument.name))
if m_costs:
cost_number = int(m_costs.group(2))
n_costs_per_option_in_measure = max(cost_number, n_costs_per_option_in_measure)
Comment on lines +722 to +735
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I'm counting up the number of options and costs per option in the ApplyUpgrade measure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it speed things up to not check every option's number of costs (since we know it's the same number for every option)?

Copy link
Member Author

Choose a reason for hiding this comment

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

This already runs pretty fast enough. It seems like an unnecessary optimization to me. Since we have to loop over all the arguments anyway, it's just as easy and simpler to check on each one.

n_options_in_cfg = 0
n_costs_in_cfg = 0
for upgrade in cfg.get("upgrades", []):
options = upgrade.get("options", [])
n_options = len(options)
n_costs = max(len(option.get("costs", [])) for option in options)
n_options_in_cfg = max(n_options, n_options_in_cfg)
n_costs_in_cfg = max(n_costs, n_costs_in_cfg)
Comment on lines +736 to +743
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I count up the number of options and costs per option required in the project file.

err_msgs = []
if n_options_in_cfg > n_options_in_measure:
err_msgs.append(
f"There are {n_options_in_cfg} options in an upgrade in your project file and only "
f"{n_options_in_measure} in the ApplyUpgrade measure."
)
if n_costs_in_cfg > n_costs_per_option_in_measure:
err_msgs.append(
f"There are {n_costs_in_cfg} costs on an option in your project file and only "
f"{n_costs_per_option_in_measure} in the ApplyUpgrade measure."
)
Comment on lines +744 to +754
Copy link
Member Author

Choose a reason for hiding this comment

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

Write some errors that tell them what's going on if there's going to be a problem.

if err_msgs:
err_msgs.append(
"See https://github.com/NREL/buildstockbatch/wiki/Adding-Options-to-the-ApplyUpgrade-measure"
)
raise ValidationError("\n".join(err_msgs))
Comment on lines +755 to +759
Copy link
Member Author

Choose a reason for hiding this comment

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

Send them to a page on the wiki explaining how to fix the problem.


return True

@staticmethod
def validate_openstudio_version(project_file):
"""
Expand Down
21 changes: 20 additions & 1 deletion buildstockbatch/test/test_integration.py
Expand Up @@ -3,8 +3,12 @@
import pathlib
import pytest
import shutil
import tempfile
import json

from buildstockbatch.localdocker import LocalDockerBatch
from buildstockbatch.utils import get_project_configuration
from buildstockbatch.exc import ValidationError

resstock_directory = pathlib.Path(
os.environ.get("RESSTOCK_DIR", pathlib.Path(__file__).resolve().parent.parent.parent.parent / "resstock")
Expand All @@ -22,7 +26,7 @@
resstock_directory / "project_testing" / "testing_upgrades.yml",
], ids=lambda x: x.stem)
@resstock_required
def test_resstock_local_batch(project_filename, mocker):
def test_resstock_local_batch(project_filename):
LocalDockerBatch.validate_project(str(project_filename))
batch = LocalDockerBatch(str(project_filename))

Expand Down Expand Up @@ -81,3 +85,18 @@ def test_resstock_local_batch(project_filename, mocker):
assert (ts_pq_path / "_metadata").exists()

shutil.rmtree(out_path)


@resstock_required
def test_number_of_options_apply_upgrade():
proj_filename = resstock_directory / "project_national" / "national_upgrades.yml"
cfg = get_project_configuration(str(proj_filename))
cfg["upgrades"][-1]["options"] = cfg["upgrades"][-1]["options"] * 10
cfg["upgrades"][0]["options"][0]["costs"] = cfg["upgrades"][0]["options"][0]["costs"] * 5
Comment on lines +93 to +95
Copy link
Member Author

Choose a reason for hiding this comment

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

In the test I add a lot more options so we bump into the validation error.

with tempfile.TemporaryDirectory() as tmpdir:
tmppath = pathlib.Path(tmpdir)
new_proj_filename = tmppath / "project.yml"
with open(new_proj_filename, "w") as f:
json.dump(cfg, f)
with pytest.raises(ValidationError):
LocalDockerBatch.validate_number_of_options(str(new_proj_filename))
12 changes: 11 additions & 1 deletion docs/changelog/changelog_dev.rst
Expand Up @@ -25,4 +25,14 @@ Development Changelog
:tags: general
:pullreq: 326

Adds some integration tests with the lastest ResStock.
Adds some integration tests with the lastest ResStock.
Copy link
Contributor

Choose a reason for hiding this comment

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

latest


.. change::
:tags: validaton
Copy link
Contributor

Choose a reason for hiding this comment

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

validation

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Ha. Thanks for catching all my hasty typos.

:pullreq: 330
:tickets: 329

Adds a validation step to verify the ApplyUpgrade measure has enough
options specified to support the upgrades in the current project file.
Instructs the user how to add more options to the ApplyUpgrade measure
if not.
1 change: 1 addition & 0 deletions setup.py
Expand Up @@ -68,6 +68,7 @@ def run_tests(self):
'yamale',
'ruamel.yaml',
'awsretry',
'lxml'
Copy link
Member Author

Choose a reason for hiding this comment

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

We need lxml to parse the xml. If adding yet another kind of heavy dependency gives anybody heartburn, I could begrudgingly rewrite this to just use element tree parser in the standard library.

],
extras_require={
'dev': [
Expand Down