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

ResStock-HPXML: HES Workflow Generator #252

Closed
wants to merge 46 commits into from
Closed

Conversation

aspeake
Copy link
Contributor

@aspeake aspeake commented Oct 27, 2021

Fixes #243

Pull Request Description

New workflow generator that translates ResStock-generated xml files to HES json inputs, which are passed to the OS-HEScore workflow to simulate buildings.

This PR adds a new workflow generator that requires a new argument os_hescore_directory to point to a local OpenStudio-HEScore branch. The hescore-hpxml translator is installed as part of the singularity/Docker image (https://github.com/NREL/docker-hescore-hpxml-openstudio/blob/main/Dockerfile). This is accompanied by changes to the resstock repo from where the hescore-hpxml translator will be called (PR is TBA)

Checklist

Not all may apply

  • Code changes (must work)
  • Tests exercising your feature/bug fix (check coverage report on CircleCI build -> Artifacts)
  • All other unit tests passing
  • Update validation for project config yaml file changes
  • Update existing documentation
  • Run a small batch run to make sure it all works (local is fine, unless an Eagle specific feature)
  • Add to the changelog_dev.rst file and propose migration text in the pull request

@aspeake aspeake linked an issue Oct 27, 2021 that may be closed by this pull request
@aspeake aspeake requested a review from nmerket December 10, 2021 23:21
@aspeake
Copy link
Contributor Author

aspeake commented Dec 10, 2021

@nmerket I think this is ready for an initial review. The new workflow generator has been built to work just with local buildstock_docker at the moment, and I plan to have the next round of changes be focused on generalizing for Eagle/AWS.

There are some CI errors right now which appear to spawn from the switch to OS-3.3.0 on the restructure-v3 branch.

Copy link
Member

@nmerket nmerket left a comment

Choose a reason for hiding this comment

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

A lot of these comments are for me. There's definitely some growing pains in buildstockbatch to accommodate HEScore here. It turns out making a fully extensible batch model articulation and simulation platform is kind of hard.

Comment on lines 66 to 69
if self.cfg['workflow_generator']['type'] == 'residential_hpxml_hes':
self.os_hescore_dir = path_rel_to_file('',self.cfg['workflow_generator']['args']['openstudio_hescore']['os_hescore_directory'])
else:
self.os_hescore_dir = None
Copy link
Member

Choose a reason for hiding this comment

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

Having this kind of conditional thing in the base class is concerning. I'm not sure what to do about it, though. Ideally, this would be handled with some elegant inheritance mechanism, but we're pretty far off from that here.

Comment on lines -561 to +565
if self.cfg['workflow_generator']['type'] == 'residential_hpxml':
if 'residential_hpxml' in self.cfg['workflow_generator']['type']:
Copy link
Member

Choose a reason for hiding this comment

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

I learned something today that you can search for a substring using in.

Comment on lines 62 to 64
# FIXME temporary docker image for testing
# return 'nrel/hescore-hpxml-openstudio'
return 'nrel/openstudio:{}'.format(self.os_version)
Copy link
Member

Choose a reason for hiding this comment

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

Need to figure out something more permanent here.

Comment on lines -90 to +98
def run_building(cls, project_dir, buildstock_dir, weather_dir, docker_image, results_dir, measures_only,
n_datapoints, cfg, i, upgrade_idx=None):
def run_building(cls, project_dir, buildstock_dir, weather_dir, os_hescore_dir, docker_image, results_dir,
measures_only, n_datapoints, cfg, i, upgrade_idx=None):
Copy link
Member

Choose a reason for hiding this comment

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

I see this was necessary to get it working, but I don't like it.

Comment on lines 119 to 123
# OS-HEScore measure directories
if os_hescore_dir and os.path.exists(os_hescore_dir):
bind_mounts += [(os.path.join(os_hescore_dir, 'rulesets'), 'OpenStudio-HEScore/rulesets', 'ro'),
(os.path.join(os_hescore_dir, 'hpxml-measures'), 'OpenStudio-HEScore/hpxml-measures', 'ro'),
(os.path.join(os_hescore_dir, 'weather'), 'OpenStudio-HEScore/weather', 'ro')]
Copy link
Member

Choose a reason for hiding this comment

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

Just thinking out loud here: I wonder if we could have the workflow generator define bind mounts in some generic way that could be used by both eagle.py and localdocker.py.

Comment on lines 201 to 224
sim_out_tarfile_name = os.path.join(sim_out_dir, 'simulations_job0.tar.gz')
logger.debug(f'Compressing simulation outputs to {sim_out_tarfile_name}')
with tarfile.open(sim_out_tarfile_name, 'w:gz') as tarf:
for dirname in os.listdir(sim_out_dir):
if re.match(r'up\d+', dirname) and os.path.isdir(os.path.join(sim_out_dir, dirname)):
tarf.add(os.path.join(sim_out_dir, dirname), arcname=dirname)
shutil.rmtree(os.path.join(sim_out_dir, dirname))
# FIXME temporarily comment out for testing
# sim_out_tarfile_name = os.path.join(sim_out_dir, 'simulations_job0.tar.gz')
# logger.debug(f'Compressing simulation outputs to {sim_out_tarfile_name}')
# with tarfile.open(sim_out_tarfile_name, 'w:gz') as tarf:
# for dirname in os.listdir(sim_out_dir):
# if re.match(r'up\d+', dirname) and os.path.isdir(os.path.join(sim_out_dir, dirname)):
# tarf.add(os.path.join(sim_out_dir, dirname), arcname=dirname)
# shutil.rmtree(os.path.join(sim_out_dir, dirname))
Copy link
Member

Choose a reason for hiding this comment

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

Remember to uncomment this at some point and if it breaks something, fix it.

osw['steps'][0]['arguments']['os_hescore_directory'] = '../../OpenStudio-HEScore'

# Add measure path for reporting measure
osw['measure_paths'] = ['OpenStudio-HEScore/hpxml-measures'] + osw['measure_paths']
Copy link
Contributor

Choose a reason for hiding this comment

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

osw['measure_paths'] += ['OpenStudio-HEScore/hpxml-measures']?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpenStudio-HEScore path needs to be first to prioritize the OS-HEScore reporting measure

@aspeake aspeake requested a review from nmerket February 1, 2022 21:26
@aspeake aspeake marked this pull request as ready for review February 8, 2022 20:22
@github-actions
Copy link

github-actions bot commented Feb 17, 2022

File Coverage
All files 85%
base.py 87%
eagle.py 74%
exc.py 57%
localdocker.py 61%
postprocessing.py 84%
utils.py 96%
sampler/base.py 79%
sampler/downselect.py 33%
sampler/precomputed.py 93%
sampler/residential_quota.py 83%
test/test_validation.py 96%
workflow_generator/base.py 90%
workflow_generator/commercial.py 53%
workflow_generator/residential.py 96%
workflow_generator/residential_hpxml.py 71%
workflow_generator/residential_hpxml_hes.py 75%

Minimum allowed coverage is 33%

Generated by 🐒 cobertura-action against d8fa15f

@aspeake
Copy link
Contributor Author

aspeake commented Feb 17, 2022

@nmerket
Copy link
Member

nmerket commented Mar 1, 2022

@aspeake Got it to work with some minor changes. Thanks for getting it to this point.

I'd like to make some more structural changes to buildstockbatch to accommodate this workflow and hopefully provide more flexibility for other workflows such as ComStock. I'd like to run this by @asparke2 and @joseph-robertson to get their thoughts.

Bind Mounts

What directories get mounted where in the container is driven by what workflow you're running. ResStock legacy, ResStock-HPXML, ComStock, and now ResStock-HEScore all have potentially slightly different folders that need to be mounted in. I'm a little murky on how we're handling this in all the implementations we have going already. It's probably a lot of if blocks detecting different workflows. I think we can move these bind mount definitions into the workflow generator classes. It makes sense that they live there. Then in localdocker.py and eagle.py we read them and build the container mount arguments for whatever container platform we're using (docker or singularity).

Custom container image built at runtime

We'd like to use the hescore-hpxml that comes with OpenStudio-HEScore. That way everything is in sync. For it to work well it needs to be installed in the docker image so the right dependencies and module lookups happen correctly. The hescore-hpxml package isn't really meant to be run as a standalone script. The problem is that we don't know the hescore-hpxml version until runtime, so we have a complicated multistep process that's prone to error to build a docker image with the hescore-hpxml package in it.

I think we can make a general capability in buildstockbatch that allows defining the image to run in three ways:

  1. No image provided. Uses default.
  2. User specifies a docker image from Docker Hub or another container registry. This is pretty easy.
  3. User points to a Dockerfile somewhere that is used to build an image before running it.

Another idea is that the docker/singularity image should be defined in the workflow generator class. In general, the workflow you're running is what drives the image needed. Not sure if this is a good idea or bad idea.

Building a singularity image on the fly is a little trickier, but might still be possible. I know ComStock is using custom Singularity images on Eagle. I'm pretty sure they pre-build them. We will probably have to do the same.

Base automatically changed from restructure-v3 to develop March 2, 2022 18:06
@aspeake
Copy link
Contributor Author

aspeake commented Sep 22, 2022

Some minor changes may be needed in the workflow generator to align with the latest approach for in residential_hpxml: #302

@nmerket
Copy link
Member

nmerket commented Sep 22, 2022

Thanks for the heads up @aspeake. Is that something you have time to do?

@aspeake
Copy link
Contributor Author

aspeake commented Sep 22, 2022

@nmerket I can probably find some time in October to do so, however I think it may require updating all the relevant branches so that we can properly test the HES workflow.

@nmerket nmerket mentioned this pull request Jul 7, 2023
8 tasks
nmerket added a commit that referenced this pull request Jul 7, 2023
@nmerket
Copy link
Member

nmerket commented Jul 7, 2023

Replaced with #378

@nmerket nmerket closed this Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResStock-HEScore workflow generator
4 participants