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

Remove Docker dependency #349

Merged
merged 18 commits into from Mar 2, 2023
Merged

Remove Docker dependency #349

merged 18 commits into from Mar 2, 2023

Conversation

nmerket
Copy link
Member

@nmerket nmerket commented Feb 16, 2023

Fixes #300.

Pull Request Description

This removes the docker dependency for local runs and the singularity dependency for runs on Eagle. Now that OpenStudio is available for more platforms and easier to get and install, the extra hassle of containerizing all the simulations is not worth it. We will still need docker for AWS because the AWS Batch service runs on Elastic Container Service, but this will simplify even that so there's only the one image that needs to be created for that.

Related to #327 since the integration tests for ComStock were tripping up becase of the custom gems. I intend to get the custom gems stuff working again in #344 after this gets merged. Having the integration tests there will ensure the code works for its intended use case. @asparke2

I would also like to see this supercede run_analysis.rb at some point, so we should add the features that are nice to have there to this, then we can get away with only maintaining the one thing. @joseph-robertson

Checklist

Not all may apply

  • Update LocalDockerBatch to remove docker and change name to LocalBatch
  • Add openstudio to the ci machine
  • Rename stuff and clean up
    • localdocker.py to local.py
    • buildstock_docker -> buildstock_local
    • ContainerRuntime
    • sampler differences based on container or not
  • Update EagleBatch to remove singularity
  • Tests exercising your feature/bug fix (check coverage report on Checks -> BuildStockBatch Tests -> Artifacts)
  • Coverage has increased or at least not decreased. Update minimum_coverage in .github/workflows/ci.yml as necessary.
  • All other unit and integration tests passing
  • Update validation for project config yaml file changes
  • Update existing documentation
  • Run a small batch run on Eagle to make sure it all works if you made changes that will affect Eagle
  • Add to the changelog_dev.rst file and propose migration text in the pull request

@github-actions
Copy link

github-actions bot commented Feb 16, 2023

File Coverage
All files 84%
base.py 89%
eagle.py 74%
exc.py 57%
local.py 50%
postprocessing.py 85%
utils.py 96%
sampler/base.py 79%
sampler/downselect.py 33%
sampler/precomputed.py 93%
sampler/residential_quota.py 61%
test/test_docker.py 33%
test/test_validation.py 97%
workflow_generator/base.py 90%
workflow_generator/commercial.py 53%
workflow_generator/residential.py 96%
workflow_generator/residential_hpxml.py 69%

Minimum allowed coverage is 33%

Generated by 🐒 cobertura-action against 25101eb

@nmerket
Copy link
Member Author

nmerket commented Feb 17, 2023

I'm actually thinking of not removing singularity at this time. ComStock uses OpenStudio 3.4.0 and there's not a build of that for CentOS at this time. Plus this PR is getting kind of unwieldy anyway. Thoughts @asparke2?

@nmerket nmerket changed the title Remove Docker and Singularity dependency Remove Docker dependency Feb 20, 2023
@nmerket nmerket marked this pull request as ready for review February 21, 2023 00:10
Copy link
Contributor

@joseph-robertson joseph-robertson left a comment

Choose a reason for hiding this comment

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

Using resstock to test buildstock_docker -> buildstock_local here: NREL/resstock#1032

Copy link
Contributor

@joseph-robertson joseph-robertson left a comment

Choose a reason for hiding this comment

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

I tried using weather_files_path: c:/OpenStudio/resstock/weather (instead of weather_files_url: https://data.nrel.gov/system/files/156/BuildStock_TMY3_FIPS.zip) and I get PermissionError: [Errno 13] Permission denied: 'c:\\OpenStudio\\resstock\\weather'.

Does weather_files_path need to be a zip? Can we support a folder, or is this a future item to address?

Copy link
Contributor

@joseph-robertson joseph-robertson left a comment

Choose a reason for hiding this comment

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

Based on the resstock test branch, this appears to be working as expected. I do see just a few rounding differences in annual simulation output, but they are very small. I'll leave it up to you whether to address the weather folder issue in my previous comment.

@nmerket
Copy link
Member Author

nmerket commented Feb 27, 2023

I tried using weather_files_path: c:/OpenStudio/resstock/weather (instead of weather_files_url: https://data.nrel.gov/system/files/156/BuildStock_TMY3_FIPS.zip) and I get PermissionError: [Errno 13] Permission denied: 'c:\\OpenStudio\\resstock\\weather'.

Does weather_files_path need to be a zip? Can we support a folder, or is this a future item to address?

Currently the weather files path does need to be a zip file. That's kind of a hold over from the way we do runs elsewhere. I'm open to making that change, but maybe would rather do it as another PR. That could include any other changes we want to port over from run_analysis.rb.

@nmerket nmerket merged commit 54cf01b into develop Mar 2, 2023
@nmerket nmerket deleted the kill-docker-singularity branch March 2, 2023 18:23
@nmerket nmerket added this to the v2023.04.0 milestone Apr 21, 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.

Remove Singularity dependency for Eagle and Docker Dependency for Local Runs
2 participants