Skip to content

[Backfill corrections] hone AWS publish behavior #1796

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 1, 2023
Merged

[Backfill corrections] hone AWS publish behavior #1796

merged 3 commits into from
Mar 1, 2023

Conversation

nmdefries
Copy link
Contributor

Description

  • Since our AWS bucket has a policy enforcing who can upload and who can view, remove --acl option from the upload command.
  • Only upload prediction files, not coefficient files.
  • Check that AWS upload step uploaded the number of files that we expected; if not, error.

Changelog

  • Makefile

@nmdefries nmdefries requested a review from krivard February 23, 2023 21:14
krivard
krivard approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍 should work

I'm definitely squinting at bash for making this kind of operation awkward though, it seems like tests and pipes should be able to be friends

backfill_corrections/Makefile
Comment on lines +92 to +94
AWS_MSG=`aws s3 cp $(USR_EXPORT_DIR) $(S3_BUCKET)/ --recursive --exclude "*" --include "*/prediction*.csv.gz"`; \
echo "$${AWS_MSG}"; \
[[ `echo "$${AWS_MSG}" | grep "upload:" | wc -l` -eq $$NUM_FILES ]] || ( echo "ERROR: Not all $${NUM_FILES} expected files were uploaded" && exit 74 ); \
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (nonblocking): it really feels like there should be a way to do this with tee or process substitution instead of variables and echo

I played with it for a while and the closest I got was teeing to stderr, which would probably be excluded from the log and that's not what we want... and your way is definitely more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's awkward to have the file upload running as a side effect.

There are some suggestions online to tee to /dev/tty or $(tty) but I can't actually get this to work on my own computer. Something to look into.

Copy link
Contributor

Choose a reason for hiding this comment

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

that could work when running from a terminal, but normal pipeline operations typically run headless alas

@nmdefries
Copy link
Contributor Author

This is ready to merge

@krivard krivard merged commit ec9a30f into main Mar 1, 2023
@krivard krivard deleted the ndefries/backfill/aws-upload-settings branch March 1, 2023 17:19
@krivard krivard mentioned this pull request Mar 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krivard krivard krivard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nmdefries @krivard