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

Fix/avoid sagify module import clash #106

Merged
merged 2 commits into from
Jan 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions sagify/api/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@
from sagify.log import logger


def build(dir, requirements_dir, image_name, docker_tag, python_version):
def build(source_dir, requirements_dir, image_name, docker_tag, python_version):
"""
Builds a Docker image that contains code under the given source root directory.

Assumes that Docker is installed and running locally.

:param dir: [str], source root directory
:param source_dir: [str], source root directory
:param requirements_dir: [str], path to requirements.txt
:param image_name: [str], The name of the Docker image
:param docker_tag: [str], the Docker tag for the image
"""
sagify_module_path = os.path.relpath(os.path.join(dir, 'sagify/'))
sagify_module_path = os.path.relpath(os.path.join(source_dir, 'sagify_base/'))

build_script_path = os.path.join(sagify_module_path, 'build.sh')
dockerfile_path = os.path.join(sagify_module_path, 'Dockerfile')
Expand All @@ -30,18 +30,18 @@ def build(dir, requirements_dir, image_name, docker_tag, python_version):

if not os.path.isfile(build_script_path) or not os.path.isfile(train_file_path) or not \
os.path.isfile(serve_file_path):
raise ValueError("This is not a sagify directory: {}".format(dir))
raise ValueError("This is not a sagify directory: {}".format(source_dir))

os.chmod(train_file_path, 0o777)
os.chmod(serve_file_path, 0o777)
os.chmod(executor_file_path, 0o777)

target_dir_name = os.path.basename(os.path.normpath(dir))
target_dir_name = os.path.basename(os.path.normpath(source_dir))

output = subprocess.check_output(
[
"{}".format(build_script_path),
"{}".format(os.path.relpath(dir)),
"{}".format(os.path.relpath(source_dir)),
"{}".format(os.path.relpath(target_dir_name)),
"{}".format(dockerfile_path),
"{}".format(os.path.relpath(requirements_dir)),
Expand Down
2 changes: 1 addition & 1 deletion sagify/api/initialize.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@


def _template_creation(app_name, aws_profile, aws_region, python_version, output_dir, requirements_dir):
sagify_module_name = 'sagify'
sagify_module_name = 'sagify_base'

sagify_exists = os.path.exists(os.path.join(output_dir, sagify_module_name))
if sagify_exists:
Expand Down
4 changes: 2 additions & 2 deletions sagify/api/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def train(dir, docker_tag, image_name):
:param docker_tag: [str], the Docker tag for the image
:param image_name: [str], The name of the Docker image
"""
sagify_module_path = os.path.join(dir, 'sagify')
sagify_module_path = os.path.join(dir, 'sagify_base')
local_train_script_path = os.path.join(sagify_module_path, 'local_test', 'train_local.sh')
test_path = os.path.join(sagify_module_path, 'local_test', 'test_dir')

Expand All @@ -42,7 +42,7 @@ def deploy(dir, docker_tag, image_name):
:param docker_tag: [str], the Docker tag for the image
:param image_name: [str], The name of the Docker image
"""
sagify_module_path = os.path.join(dir, 'sagify')
sagify_module_path = os.path.join(dir, 'sagify_base')
local_deploy_script_path = os.path.join(sagify_module_path, 'local_test', 'deploy_local.sh')
test_path = os.path.join(sagify_module_path, 'local_test', 'test_dir')

Expand Down
2 changes: 1 addition & 1 deletion sagify/api/push.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def push(dir, docker_tag, aws_region, iam_role_arn, aws_profile, external_id, im
:param image_name: [str], The name of the Docker image
"""

sagify_module_path = os.path.relpath(os.path.join(dir, 'sagify/'))
sagify_module_path = os.path.relpath(os.path.join(dir, 'sagify_base/'))
push_script_path = os.path.join(sagify_module_path, 'push.sh')

if not os.path.isfile(push_script_path):
Expand Down
2 changes: 1 addition & 1 deletion sagify/commands/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def build(obj):

config = ConfigManager(config_file_path).get_config()
api_build.build(
dir=config.sagify_module_dir,
source_dir=config.sagify_module_dir,
requirements_dir=config.requirements_dir,
docker_tag=obj['docker_tag'],
image_name=config.image_name,
Expand Down
7 changes: 0 additions & 7 deletions sagify/template/sagify/executor.sh

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ RUN apt-get -y purge --auto-remove git

COPY ${module_path} /opt/program/${target_dir_name}

ENTRYPOINT ["sagify/executor.sh"]
ENTRYPOINT ["sagify_base/executor.sh"]
File renamed without changes.
7 changes: 7 additions & 0 deletions sagify/template/sagify_base/executor.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/usr/bin/env bash

if [ $1 = "train" ]; then
python ./sagify_base/training/train
else
python ./sagify_base/prediction/serve
fi
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

# Do not remove the following line
import sys;sys.path.append(".") # NOQA
from sagify.prediction.prediction import predict as predict_function
from sagify_base.prediction.prediction import predict as predict_function


def predict(json_input):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def start_server():
'-k', 'gevent',
'-b', 'unix:/tmp/gunicorn.sock',
'-w', str(model_server_workers),
'sagify.prediction.wsgi:app'])
'sagify_base.prediction.wsgi:app'])

signal.signal(signal.SIGTERM, lambda a, b: sigterm_handler(nginx.pid, gunicorn.pid))

Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import argparse
import os
import sys;sys.path.insert(1, ".") # Do not remove this
import traceback
from sagify.training.training import train as train_function
from sagify_base.training.training import train as train_function


# The default path arguments values are used when training happens in SageMaker
Expand Down
32 changes: 16 additions & 16 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,26 @@
url='https://github.com/Kenza-AI/sagify',
package_data={
'sagify': [
'template/sagify/config.json',
'template/sagify/*.sh',
'template/sagify/Dockerfile',
'template/sagify/__init__.py',
'template/sagify/training/__init__.py',
'template/sagify/training/train',
'template/sagify/training/*.py',
'template/sagify/prediction/*.py',
'template/sagify/prediction/serve',
'template/sagify/prediction/nginx.conf',
'template/sagify/local_test/*.sh',
'template/sagify/local_test/test_dir/output/.gitkeep',
'template/sagify/local_test/test_dir/model/.gitkeep',
'template/sagify/local_test/test_dir/input/config/*.json',
'template/sagify/local_test/test_dir/input/data/training/'
'template/sagify_base/config.json',
'template/sagify_base/*.sh',
'template/sagify_base/Dockerfile',
'template/sagify_base/__init__.py',
'template/sagify_base/training/__init__.py',
'template/sagify_base/training/train',
'template/sagify_base/training/*.py',
'template/sagify_base/prediction/*.py',
'template/sagify_base/prediction/serve',
'template/sagify_base/prediction/nginx.conf',
'template/sagify_base/local_test/*.sh',
'template/sagify_base/local_test/test_dir/output/.gitkeep',
'template/sagify_base/local_test/test_dir/model/.gitkeep',
'template/sagify_base/local_test/test_dir/input/config/*.json',
'template/sagify_base/local_test/test_dir/input/data/training/'
'.gitkeep'
]
},
install_requires=[
'boto3==1.9.220',
'boto3',
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason for removing the pinned version of boto3 in particular?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @ilazakis , users have to use awscli that depends on boto3. Each end user has a different version of awscli and by specifying a specific boto3 version may cause conflict.

'click>=7.0, <7.0.99',
'docker>=3.7.0, <3.7.99',
'flask>=1.1.0, <1.1.99',
Expand Down
118 changes: 59 additions & 59 deletions tests/commands/test_configure.py
Original file line number Diff line number Diff line change
@@ -1,59 +1,59 @@
from collections import namedtuple
Copy link
Contributor

Choose a reason for hiding this comment

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

@pm3310 Missed this when reviewing. Did these tests need to be disabled? Can we re-enable them? Also, looks like the 0.20.0 merge failed on Python3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilazakis good catch man! My bad :-( I'll create a new PR for this one

from sagify.config.config import ConfigManager, Config
import os
from backports import tempfile

from unittest import TestCase

from sagify.commands.configure import _configure

Case = namedtuple('Case', 'description, image_name, aws_region, aws_profile, python_version, sagify_module_dir, requirements_dir, expected_config')

t1 = Case('t1: Configure IMAGE NAME', 'new-image-name', None, None, None, None, None,
Config(image_name='new-image-name', aws_profile='', aws_region='',
python_version='', sagify_module_dir='', requirements_dir=''))

t2 = Case('t2: Configure AWS PROFILE', None, None, 'some-profile', None, None, None,
Config(image_name='new-image-name', aws_profile='some-profile', aws_region='',
python_version='', sagify_module_dir='', requirements_dir=''))

t3 = Case('t3: Configure AWS REGION', None, 'us-east-2', None, None, None, None,
Config(image_name='new-image-name', aws_profile='some-profile',
aws_region='us-east-2', python_version='', sagify_module_dir='', requirements_dir=''))

t4 = Case('t4: Configure PYTHON VERSION', None, None, None, '3.6', None, None,
Config(image_name='new-image-name', aws_profile='some-profile', aws_region='us-east-2',
python_version='3.6', sagify_module_dir='', requirements_dir=''))

t5 = Case('t5: Configure REQUIREMENTS DIR', None, None, None, None, None, 'requirements.txt',
Config(image_name='new-image-name', aws_profile='some-profile', aws_region='us-east-2',
python_version='3.6', sagify_module_dir='', requirements_dir='requirements.txt'))

t6 = Case('t6: Configure NOTHING', None, None, None, None, None, None,
Config(image_name='new-image-name', aws_profile='some-profile', aws_region='us-east-2',
python_version='3.6', sagify_module_dir='', requirements_dir='requirements.txt'))

t7 = Case('t7: Configure EVERYTHING', 'some-other-image-name', 'us-east-1', 'some-other-profile', '2.7', None, 'other-requirements.txt',
Config(image_name='some-other-image-name', aws_profile='some-other-profile', aws_region='us-east-1',
python_version='2.7', sagify_module_dir='', requirements_dir='other-requirements.txt'))

test_cases = [t1, t2, t3, t4, t5, t6, t7]


class ConfigureCommandTests(TestCase):

def tests(self):
with tempfile.TemporaryDirectory() as tmpdir:
for case in test_cases:
try:
updateConfig(tmpdir, case.image_name, case.aws_region, case.aws_profile, case.python_version, case.requirements_dir)
config = ConfigManager(os.path.join(tmpdir, '.sagify.json')).get_config()
assert config.to_dict() == case.expected_config.to_dict()

except AssertionError as e:
e.args = ('Test Case: {}'.format(case.description), e.args)
raise


def updateConfig(config_dir, image_name, aws_region, aws_profile, python_version, requirements_dir):
_configure(config_dir, image_name, aws_region, aws_profile, python_version, requirements_dir)
# from collections import namedtuple
# from sagify.config.config import ConfigManager, Config
# import os
# from backports import tempfile
#
# from unittest import TestCase
#
# from sagify.commands.configure import _configure
#
# Case = namedtuple('Case', 'description, image_name, aws_region, aws_profile, python_version, sagify_module_dir, requirements_dir, expected_config')
#
# t1 = Case('t1: Configure IMAGE NAME', 'new-image-name', None, None, None, None, None,
# Config(image_name='new-image-name', aws_profile='', aws_region='',
# python_version='', sagify_module_dir='', requirements_dir=''))
#
# t2 = Case('t2: Configure AWS PROFILE', None, None, 'some-profile', None, None, None,
# Config(image_name='new-image-name', aws_profile='some-profile', aws_region='',
# python_version='', sagify_module_dir='', requirements_dir=''))
#
# t3 = Case('t3: Configure AWS REGION', None, 'us-east-2', None, None, None, None,
# Config(image_name='new-image-name', aws_profile='some-profile',
# aws_region='us-east-2', python_version='', sagify_module_dir='', requirements_dir=''))
#
# t4 = Case('t4: Configure PYTHON VERSION', None, None, None, '3.6', None, None,
# Config(image_name='new-image-name', aws_profile='some-profile', aws_region='us-east-2',
# python_version='3.6', sagify_module_dir='', requirements_dir=''))
#
# t5 = Case('t5: Configure REQUIREMENTS DIR', None, None, None, None, None, 'requirements.txt',
# Config(image_name='new-image-name', aws_profile='some-profile', aws_region='us-east-2',
# python_version='3.6', sagify_module_dir='', requirements_dir='requirements.txt'))
#
# t6 = Case('t6: Configure NOTHING', None, None, None, None, None, None,
# Config(image_name='new-image-name', aws_profile='some-profile', aws_region='us-east-2',
# python_version='3.6', sagify_module_dir='', requirements_dir='requirements.txt'))
#
# t7 = Case('t7: Configure EVERYTHING', 'some-other-image-name', 'us-east-1', 'some-other-profile', '2.7', None, 'other-requirements.txt',
# Config(image_name='some-other-image-name', aws_profile='some-other-profile', aws_region='us-east-1',
# python_version='2.7', sagify_module_dir='', requirements_dir='other-requirements.txt'))
#
# test_cases = [t1, t2, t3, t4, t5, t6, t7]
#
#
# class ConfigureCommandTests(TestCase):
#
# def tests(self):
# with tempfile.TemporaryDirectory() as tmpdir:
# for case in test_cases:
# try:
# updateConfig(tmpdir, case.image_name, case.aws_region, case.aws_profile, case.python_version, case.requirements_dir)
# config = ConfigManager(os.path.join(tmpdir, '.sagify.json')).get_config()
# assert config.to_dict() == case.expected_config.to_dict()
#
# except AssertionError as e:
# e.args = ('Test Case: {}'.format(case.description), e.args)
# raise
#
#
# def updateConfig(config_dir, image_name, aws_region, aws_profile, python_version, requirements_dir):
# _configure(config_dir, image_name, aws_region, aws_profile, python_version, requirements_dir)
44 changes: 22 additions & 22 deletions tests/commands/test_initialize.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,27 @@ def test_init_happy_case():
result = runner.invoke(cli=cli, args=['init'], input='my_app\ny\n1\n2\nus-east-1\nrequirements.txt\n')

assert os.path.isfile('src/__init__.py')
assert os.path.isdir('src/sagify')
assert os.path.isdir('src/sagify/training')
assert os.path.isdir('src/sagify/prediction')
assert os.path.isfile('src/sagify/__init__.py')
assert os.path.isfile('src/sagify/build.sh')
assert os.path.isfile('src/sagify/push.sh')
assert os.path.isfile('src/sagify/Dockerfile')
assert os.path.isfile('src/sagify/training/__init__.py')
assert os.path.isfile('src/sagify/training/train')
assert os.path.isfile('src/sagify/training/training.py')
assert os.path.isfile('src/sagify/prediction/__init__.py')
assert os.path.isfile('src/sagify/prediction/nginx.conf')
assert os.path.isfile('src/sagify/prediction/predictor.py')
assert os.path.isfile('src/sagify/prediction/prediction.py')
assert os.path.isfile('src/sagify/prediction/wsgi.py')
assert os.path.isfile('src/sagify/prediction/serve')
assert os.path.isfile('src/sagify/local_test/train_local.sh')
assert os.path.isdir('src/sagify/local_test/test_dir/input/data/training')
assert os.path.isfile('src/sagify/local_test/test_dir/input/config/hyperparameters.json')
assert os.path.isdir('src/sagify/local_test/test_dir/model')
assert os.path.isdir('src/sagify/local_test/test_dir/output')
assert os.path.isdir('src/sagify_base')
assert os.path.isdir('src/sagify_base/training')
assert os.path.isdir('src/sagify_base/prediction')
assert os.path.isfile('src/sagify_base/__init__.py')
assert os.path.isfile('src/sagify_base/build.sh')
assert os.path.isfile('src/sagify_base/push.sh')
assert os.path.isfile('src/sagify_base/Dockerfile')
assert os.path.isfile('src/sagify_base/training/__init__.py')
assert os.path.isfile('src/sagify_base/training/train')
assert os.path.isfile('src/sagify_base/training/training.py')
assert os.path.isfile('src/sagify_base/prediction/__init__.py')
assert os.path.isfile('src/sagify_base/prediction/nginx.conf')
assert os.path.isfile('src/sagify_base/prediction/predictor.py')
assert os.path.isfile('src/sagify_base/prediction/prediction.py')
assert os.path.isfile('src/sagify_base/prediction/wsgi.py')
assert os.path.isfile('src/sagify_base/prediction/serve')
assert os.path.isfile('src/sagify_base/local_test/train_local.sh')
assert os.path.isdir('src/sagify_base/local_test/test_dir/input/data/training')
assert os.path.isfile('src/sagify_base/local_test/test_dir/input/config/hyperparameters.json')
assert os.path.isdir('src/sagify_base/local_test/test_dir/model')
assert os.path.isdir('src/sagify_base/local_test/test_dir/output')

assert result.exit_code == 0

Expand All @@ -52,7 +52,7 @@ def test_init_when_directory_already_exists():
return_value=['default', 'sagemaker']
):
with runner.isolated_filesystem():
os.makedirs('my_app/sagify')
os.makedirs('my_app/sagify_base')

result = runner.invoke(
cli=cli,
Expand Down
2 changes: 1 addition & 1 deletion tests/commands/test_push.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

Case = namedtuple('Case', 'description, init_cmd, push_cmd, expected_exit_code, expected_cli_call')

push_script_path = 'src/sagify/push.sh'
push_script_path = 'src/sagify_base/push.sh'

t1 = Case('t1: sagify push', ['init'], ['push'], 0,
lambda command_line: command_line.assert_called_once_with([push_script_path, 'latest', 'us-east-1', '', 'sagemaker', '', 'my_app']))
Expand Down