Skip to content
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

Add boto3 dependency, add venv/.venv to .gitignore #239

Merged
merged 8 commits into from
Jul 11, 2024

Conversation

TimothyWillard
Copy link
Contributor

Should address #238.

saraloo
saraloo previously approved these changes Jun 21, 2024
@jcblemai
Copy link
Collaborator

Thanks, Tim, very good catch. Few thoughts:

This originates from #218 which makes me wonder if we should add botocores as well or if it is included in boto.

I'm also hesitant to force the user to install boto3 (which is not that easy in some conda environments) just for some batch functions that are not often used. Most of our users won't have an AWS CLI set up.

Perhaps we should import boto just in the aws batch functions that need it (like it's done in batch/inference_job: you can run it locally without a CLI, only with the --aws flag it imports the modules). We can also add it to an extra install target (batch, aws?)? I'm hesitant about the last item because it is also complex to have several targets.

jcblemai
jcblemai previously approved these changes Jun 22, 2024
emprzy
emprzy previously approved these changes Jun 24, 2024
Copy link
Collaborator

@emprzy emprzy left a comment

Choose a reason for hiding this comment

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

Don't have much/any input, but thank you for looping me in! Good for my edification to see stuff like this!

@TimothyWillard TimothyWillard dismissed stale reviews from emprzy, jcblemai, and saraloo via 7a83279 June 25, 2024 19:57
@TimothyWillard
Copy link
Contributor Author

This originates from #218 which makes me wonder if we should add botocores as well or if it is included in boto.

boto3 is tightly coupled to botocore, but I went ahead and added it as an install dependency anyways since we explicitly import from botocore.

I'm also hesitant to force the user to install boto3 (which is not that easy in some conda environments) just for some batch functions that are not often used. Most of our users won't have an AWS CLI set up.

I went ahead and made it an optional install target, my main concern is being able to successfully install and run the package following the quick start documentation and these dependencies are used in only one function. So if making this a required dependency for the package causes more headaches let's just go with the path of least resistance.

saraloo
saraloo previously approved these changes Jul 10, 2024
Had to resolve merge conflicts related to the import of boto3 and
botocore.
@jcblemai
Copy link
Collaborator

Thanks, that looks good to me. It's a bit unfortunate that the python setup tools do not support "negative" targets (e.g. a pip install gempyor with everything, and a pip install gemypor[light] with a stripped down version). But it has been requested for a long time.

PR all good to merge, ty Tim

@saraloo saraloo self-requested a review July 11, 2024 19:27
@TimothyWillard TimothyWillard merged commit ad08cce into main Jul 11, 2024
1 check passed
@TimothyWillard TimothyWillard deleted the bug/GH-238-missing-boto3 branch July 11, 2024 20:07
@TimothyWillard
Copy link
Contributor Author

Thanks for reviewing y'all! Merging this one in, I'm quite excited to clean up my git status.

@TimothyWillard TimothyWillard restored the bug/GH-238-missing-boto3 branch July 11, 2024 20:08
@jcblemai jcblemai deleted the bug/GH-238-missing-boto3 branch July 12, 2024 08:11
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.

None yet

4 participants