-
Notifications
You must be signed in to change notification settings - Fork 2
Aws iw pipeline uplift #28
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
Conversation
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.
Looks good to me -- I have suggested a few minor improvements/formatting changes.
docs/workflows/aws.md
Outdated
|
|
||
| ```bash | ||
| docker build -t sar-pipeline -f Docker/Dockerfile . | ||
| docker build -t sar-pipeline:0.2 -f Docker/Dockerfile . |
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.
The Dockerfile itself has version 0.1 in the file. Does the Dockerfile need to be updated, or are these unconnected?
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.
In my testing I was building an image with a new tag (0.2) and was keeping my notes for running. I'll keep consistent at 0.1 and update my docs to be helpful and not just me jotting things
docs/workflows/aws.md
Outdated
| Development in the container | ||
| ```bash | ||
| docker run --env-file env.secret -it --entrypoint /bin/bash -v $(pwd):/home/rtc_user/sar-pipeline -v $(pwd)/scripts:/home/rtc_user/scripts -v /data/working:/home/rtc_user/working sar-pipeline | ||
| docker run --env-file env.secret -it --entrypoint /bin/bash -v $(pwd):/home/rtc_user/sar-pipeline -v $(pwd)/scripts:/home/rtc_user/scripts -v /data/working:/home/rtc_user/working sar-pipeline:0.2 |
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.
Do you have documentation somewhere (or a template file) for what needs to be in env.secret? Can you please add some instructions on what's required for 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.
Will add to docs
| # - scratch : scratch folder for processing | ||
|
|
||
| if make_folders: | ||
| logger.info(f'Making output folders if not existing') |
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.
Since these are now coming in as click paths, you can use Path.mkdir to create them: https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir
| return json.load(file) | ||
|
|
||
| def update_value(self, key_path: str, value): | ||
| def update_value(self, key_path: str, value, separator='.'): |
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.
Please update to have numpy doc strings consistent with rest of repo.
| ref[keys[-1]] = value | ||
|
|
||
| def get_value(self, key_path: str): | ||
| def get_value(self, key_path: str, separator='.'): |
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.
Please update to have numpy doc strings consistent with rest of repo.
|
@caitlinadams changes have been incorporated if you want to have a suss |
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.
Had a quick look -- all looking good! Thanks for making the changes. I'll merge now.
scripts/run_aws_pipeline.sh- process will now exit on failure rather than progress to the next step