-
Notifications
You must be signed in to change notification settings - Fork 14
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
Two small fixes #424
Two small fixes #424
Conversation
Minimum allowed coverage is Generated by 🐒 cobertura-action against 1b60710 |
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.
One question.
cls.run_simulations(cfg, jobs_d, job_id, sim_dir, S3FileSystem(), bucket, prefix) | ||
cls.run_simulations(cfg, jobs_d, job_id, sim_dir, S3FileSystem(), f"{bucket}/{prefix}") |
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.
I see that in the run_simulations
function definition there's just an output_path
argument. Does that mean this wasn't already working?
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.
I changed the function params in response to this comment, but missed updating this line.
Meanwhile, I'm also looking into why the test didn't catch this.
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.
Added some comments to test_docker_base.py to at least explain why the test there won't catch issues like the one in docker_base.py.
We should try to get that test to test things more thoroughly, but that's more involved so I'll save it for a separate PR.
Fixes two small issues from #422
Pull Request Description
Checklist
Not all may apply
minimum_coverage
in.github/workflows/coverage.yml
as necessary.Update validation for project config yaml file changesUpdate existing documentationRun a small batch run on Kestrel/Eagle to make sure it all works if you made changes that will affect Kestrel/EagleAdd to the changelog_dev.rst file and propose migration text in the pull request