-
Notifications
You must be signed in to change notification settings - Fork 12
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
Changes from all commits
d7e5a7a
8dd02c2
38a4726
49e208d
04bb47a
390694e
bbaeb14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
|
@@ -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)) | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ def run_tests(self): | |
'yamale', | ||
'ruamel.yaml', | ||
'awsretry', | ||
'lxml' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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': [ | ||
|
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.
Unrelated to this issue?
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.
Yes, unrelated. I missed it on a previous PR.