Skip to content

Conversation

@ErinWeisbart
Copy link
Member

Correct credential handling to enable AWS IAM role usage.

@ErinWeisbart
Copy link
Member Author

Closes #132 by changing load_data_csv access to download.
Closes #114.

@ErinWeisbart ErinWeisbart changed the title correct role handling correct role handling and S3FS bypass Aug 5, 2022
@ErinWeisbart
Copy link
Member Author

Noting that S3FS bypass appears to be working fine and I had several tests of all the files downloading correctly and then apparently stochastically one run threw:
OSError: [Errno 30] Read-only file system: '/home/ubuntu/local_input/path/to/image/cdp2bioactives_a22_s4_w33236d730-2143-4738-bfac-685f917a717d.tif.2FE98647'

@ErinWeisbart
Copy link
Member Author

Full S3FS bypass is working at this point.
Still haven't finished necessary changes to run-worker.sh to make it mount S3 bucket using new credential handling.

@bethac07 bethac07 linked an issue Sep 20, 2022 that may be closed by this pull request
@ErinWeisbart ErinWeisbart marked this pull request as ready for review January 6, 2023 23:01
@ErinWeisbart ErinWeisbart requested a review from bethac07 January 6, 2023 23:01
@ErinWeisbart
Copy link
Member Author

ErinWeisbart commented Jan 6, 2023

I have this branch running on a dataset now and it works (!), successfully completely bypassing S3FS and using a role.
(I don't know if it mounts S3 using a role, but I think it will be helpful to consider this a complete unit and troubleshoot S3 mounting with a role if necessary in a separate PR).

Copy link
Collaborator

@bethac07 bethac07 left a comment

Choose a reason for hiding this comment

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

Overall, this seems great, and super exciting that it's working! Thank you so much

For future, tiiiiiny nitpick - would be great to not mix in a ton of stylistic changes (like "->' in a PR like this already making big behavior changes, or at least make those an explicit commit, just to a) keep the size and readability of the PR reasonable and b) assuming commits aren't squashed, by having them be an explicit commit, you see that that's why a line was changed ("oh, that was done for stylistic reasons" as opposed to "wait, what does this have to do with role handling?"). For this one, there are a lot of commits, so up to you if you'd like us to squash or not, but just something small for the future.

Congrats on this, it was a huge achievement!

@bethac07 bethac07 mentioned this pull request Jan 20, 2023
fix variable names
@ErinWeisbart
Copy link
Member Author

Thanks @bethac07 !
Sorry, this PR kinda turned into a chaos of commits in all the troubleshooting. I'm not honestly sure how a lot of stylistic changes ended up being mixed in, but I will certainly try harder to keep them to their own commit in the future.

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.

DOWNLOAD_FILES doesn't fully bypass S3FS Make DCP use roles better, not just passing around credentials

3 participants