NREL / resstock

Highly granular modeling of residential building stocks at national, regional, and local scales using OpenStudio/EnergyPlus.
https://resstock.nrel.gov
Other
107 stars 54 forks source link

Fix #1130 - Can't run resstock in parallel #1131

Closed jmarrec closed 5 months ago

jmarrec commented 1 year ago

Pull Request Description

I know the BuildExistingModel is in the OpenStudio-HPXML repo and changes should probably be made there, but I would like to make sure this has a chance of being included in ResStock before I make the changes over there as well.

Having the ability to run ResStock in parallel is something I actively need.

Running 125 "baseline" simulations on my 8 CPU / 16 cores machine:

I ran 3125 simulations, in paralle, in 2h30 minutes. And I plan to run a lot more. This would have taken somewhere between 17 and 24 hours serially, rendering it completely unpractical

I'd very much like if this could be upstreamed in ResStock, as it may help other people and would reduce the burden of keeping my fork up to date.

Checklist

Not all may apply:

joseph-robertson commented 1 year ago

@jmarrec So the basic idea here is making the path of the buildstock.csv file variable (instead of fixed at lib/housing_characteristics) so that you can use run_analysis.rb in parallel?

Couple of questions. (1) How can we get this branch running on CI? (2) If we did, I'd assume we'd see problems with the integration-tests job that uses buildstockbatch (where you haven't yet updated its workflow generator for the BuildExistingModel update). Is there a way to make the new buildstock_csv_path argument optional (and default to the original lib/housing_characteristics) so we wouldn't need to make any workflow generator changes? Edit: looks like above you are already noting we could do this šŸ‘ .

jmarrec commented 1 year ago

@jmarrec So the basic idea here is making the path of the buildstock.csv file variable (instead of fixed at lib/housing_characteristics) so that you can use run_analysis.rb in parallel?

Yes, and don't wipe the lib folder if it's already there and up to date.

Couple of questions.

(1) How can we get this branch running on CI?

Your CI rules seem to protect against running on a branch from a fork... That's a bad setting IMHO. You should set it so workflows coming from a fork don't run automatically but required a maintainer to approve the run.

Anyways, I'm cloning this PR onto my forked repo so it runs there: https://github.com/jmarrec/resstock/pull/1

(2) If we did, I'd assume we'd see problems with the integration-tests job that uses buildstockbatch (where you haven't yet updated its workflow generator for the BuildExistingModel update). Is there a way to make the new buildstock_csv_path argument optional (and default to the original lib/housing_characteristics) so we wouldn't need to make any workflow generator changes? Edit: looks like above you are already noting we could do this šŸ‘ .

Yes we can. We'll see if it's necessary.

joseph-robertson commented 5 months ago

@jmarrec Should we still consider getting this PR into a state to be merged?

jmarrec commented 5 months ago

@joseph-robertson I think the change makes sense, but as a maintainer you're always entitled to reject a pull request as something you don't want to maintain or don't see the point of. Up to you really. (I won't be mad if that's the case, just close it)

The conflicts are trivial to fix and I can resolve them if needed.

joseph-robertson commented 5 months ago

Superseded by https://github.com/NREL/resstock/pull/1230.