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 script for setting up an AMI #11

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Add script for setting up an AMI #11

wants to merge 15 commits into from

Conversation

aboucaud
Copy link
Contributor

This PR adds both a bash script and a conda environment file to store the generic RAMP dependencies.

The idea is to add to every kit a file named ami_environment.yml containing the conda dependencies (channels, dependencies & pip-only requirements) so an AMI can easily be setup for this particular kit with three simple shell commands

git clone https://github.com/paris-saclay-cds/ramp-backend.git
cd ramp-backend
./scripts/ami_setup.sh <kit_name> scripts/ramp_environment.yml

@kegl
Copy link

kegl commented May 18, 2018

Great, just let's not call it "ami", it should be generic setup for any (ubuntu) backend.

@aboucaud
Copy link
Contributor Author

Name changed to ubuntu_setup.sh..

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

let's not call it "ami",

It's maybe more a comment for the autism repo, but shouldn't we then call it there differently as well? (eg ramp_environment.yml as the file here)

source .profile

conda update --yes --quiet conda
pip install --upgrade pip
Copy link
Member

Choose a reason for hiding this comment

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

conda update pip ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last time I tried, the conda version of pip was an old one, and pip was complaining there was a newer version to be install with this command.

I guess that was a lazy choice to avoid a warning.

#################################################################
update_conda_env() {
environment_file=$1
conda env update --name base --file $environment_file
Copy link
Member

Choose a reason for hiding this comment

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

Should check, but I think you could even leave out the --name base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True only if the environment file does not have a different name specified.
This is for corner cases.

@@ -0,0 +1,119 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

the file is called .py, but should this be .sh or something similar ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups.. 😄

@@ -0,0 +1,17 @@
name: ramp
Copy link
Member

Choose a reason for hiding this comment

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

What is this file used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still had in mind we should separate the "backend" requirements for the moment.

Since we install ramp-backend and ramp-workflow via pip, all of their dependencies are also installed via pip by default. If we specify their dependencies in this file, we then can use conda for most of these dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, missed it in the script that you do the conda env update twice, with this one and the one of the kit

- sqlalchemy
- boto3
- pip:
- https://api.github.com/repos/paris-saclay-cds/ramp-workflow/zipball/master
Copy link
Member

Choose a reason for hiding this comment

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

I think ramp-workflow is not a dependency of ramp-backend? So maybe leave it here to only ramp-backend, so we have less duplication between this env file and the one in the starting kit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it not better the other way around ?
keep ramp-workflow here and remove it from the ami_env.yml

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not sure. Keeping it in the starting kit makes that more complete for people who want to check or replicate the same environment for the challenge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then

- jupyter
- matplotlib
- scikit-learn
- psycopg2
Copy link
Member

Choose a reason for hiding this comment

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

this will not really work as desired I think since you defined psycopg2-binary as the dependency in setup.py (of ramp-backend). But I think it is actually better to change setup.py to use just psycopg2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching the mismatch.

I switched from psycopg2 to psycopg2-binary because it was advised in a Warning message. I have to check if both exist within conda though.

Copy link
Member

Choose a reason for hiding this comment

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

conda already provides binary packages, so there should not exist such a thing (It's rather strange practice of the psycopg2 devs to do this I think)

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.

3 participants