upload-buildstockcsv-to-s3

[yingli-NREL]

Modified postprocessing.py def upload_results() to upload buildstockcsv to s3

Checklist

Not all may apply

• Code changes (must work)
• 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]

File	Coverage
All files	84%	✅
base.py	89%	✅
eagle.py	79%	✅
exc.py	57%	✅
local.py	50%	✅
postprocessing.py	84%	✅
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_hpxml.py	84%	✅

Minimum allowed coverage is 33%

Generated by 🐒 cobertura-action against 621e574

[nmerket]

@yingli-NREL I added the pull request checklist back in. What you need to do is make sure you've done each item in that list (or verified that it wasn't applicable). If you're not sure what some of those mean, @rajee or I can help.

[yingli-NREL]

create one folder named "buildstock_csv" in the S3 results directory and add buildstock.csv in the folder "buildstock_csv"

[nmerket]

I apologize for taking so long to properly review this. Have you run a small batch to verify this is working? This could use some unit testing, which is kind of tricky with s3 stuff because you'll probably need to mock things. There's some existing tests I linked to that could be added to. A couple other notes below. I know it is a lot, but I've been thorough and somewhat critical hopefully as a teaching tool. If it would help, we can meet to talk through some of this.

[nmerket]

When I look at the coverage report, this doesn't seem to be getting called. It's not clear to me why.

It would make sense to add something to this test to check if the file got uploaded. It's using mock to not actually upload file to s3 but to see that it was called successfully.

[nmerket]

One more thought on this, could we not have upload_buildstock_csv here? It seems to mostly be a copy of the more generic upload_file but puts the file somewhere else. Maybe there could be a second, optional argument to upload_file where you could specify the s3 location when it's different than the default and then just use it for buildstock.csv.

[yingli-NREL]

I created a local environment in my HPC account and used that environment to run a small batch. It works. The results are upload in Amazon S3>Buckets>eulp>buildstock_csv_to_s3/>test/>test10/

[nmerket]

I see why you made this a list so you could use map below, but that's not necessary. You can just pull the filename here. We should also only upload it if it exists. I think ComStock might put this file somewhere different, so it may not always be where you're looking for it.

Suggested change

	buildstock_csv = []
	for file in buildstock_dir.glob('buildstock.csv'):
	buildstock_csv.append(file.relative_to(buildstock_dir))
	buildstock_csv = buildstock_dir / 'buildstock.csv'

I haven't tested that this works. I might be missing some normalization or something.

[nmerket]

It's best to gather all the work you want to do into one list of tasks and then send that to dask.compute once. You don't have to call dask.delayed with a map, it was just convenient in this case. See dask.delayed documentation for more details. This change depends on buildstock_csv being a filename, not a list with just one item in it.

Suggested change

	dask.compute(map(dask.delayed(upload_file), all_files))
	dask.compute(map(dask.delayed(upload_buildstock_csv), buildstock_csv))
	tasks = list(map(dask.delayed(upload_file), all_files))
	tasks.append(dask.delayed(upload_buildstock_csv)(buildstock_csv))
	dask.compute(tasks)

[nmerket]

The buildstock.csv is in a different place depending on whether you're running buildstock_eagle or buildstock_local. When I run buildstock_local and have the output_directory not in the default location, it fails to upload:

INFO:2023-10-06 17:06:19:buildstockbatch.postprocessing:Uploading the parquet files to s3
2023-10-06 17:06:20,315 - distributed.worker - WARNING - Compute Failed
Key:       upload_buildstock_csv-3a9eafb5-350b-4d0d-ad8d-efae94791c46
Function:  upload_buildstock_csv
args:      ('')
kwargs:    {}
Exception: "FileNotFoundError(2, 'No such file or directory')"

I can take a look at why next week.

[nmerket]

A few more notes. I'll try to get this working early next week.

[nmerket]

I think this is the problem right here. It's assuming that file is in the usual place it is on Eagle. I think the sampler returns the buildstock.csv location. We should catch that and use that file. Could be a bit tricky because the sample is called way before the postprocessing on a different node and everything.

[nmerket]

Also, this is a little bizarre to use a for loop and glob to look for one file.

[nmerket]

I confirmed it's working for both the local version and on Eagle.