-
Notifications
You must be signed in to change notification settings - Fork 202
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
initial work on docker containerization support #2479
Conversation
easybuild/tools/containers/docker.py
Outdated
if build_option('force'): | ||
print_msg("WARNING: overwriting existing Dockerfile at %s due to --force" % dockerfile) | ||
else: | ||
raise EasyBuildError("Dockerfile recipe at %s already exists, not overwriting it without --force", dockerfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (122 > 120 characters)
easybuild/tools/containers/docker.py
Outdated
dockerfile = os.path.join(cont_path, 'Dockerfile.%s' % file_label) | ||
if os.path.exists(dockerfile): | ||
if build_option('force'): | ||
print_msg("WARNING: overwriting existing Dockerfile at %s due to --force" % dockerfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'print_msg'
easybuild/tools/containers/docker.py
Outdated
|
||
module_naming_scheme = ActiveMNS() | ||
|
||
ec = easyconfigs[-1]['ec'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local variable 'ec' is assigned to but never used
easybuild/tools/containers/docker.py
Outdated
res = [] | ||
_os_deps = reduce(operator.add, [obj['ec']['osdependencies'] for obj in easyconfigs], []) | ||
for os_dep in _os_deps: | ||
if isinstance(os_dep, basestring): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'basestring'
easybuild/tools/containers/docker.py
Outdated
|
||
def _det_os_deps(easyconfigs): | ||
res = [] | ||
_os_deps = reduce(operator.add, [obj['ec']['osdependencies'] for obj in easyconfigs], []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'reduce'
easybuild/tools/containers/docker.py
Outdated
if build_option('force'): | ||
print_msg("WARNING: overwriting existing Dockerfile at %s due to --force" % dockerfile) | ||
else: | ||
raise EasyBuildError("Dockerfile recipe at %s already exists, not overwriting it without --force", dockerfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (122 > 120 characters)
easybuild/tools/containers/docker.py
Outdated
dockerfile = os.path.join(cont_path, 'Dockerfile.%s' % file_label) | ||
if os.path.exists(dockerfile): | ||
if build_option('force'): | ||
print_msg("WARNING: overwriting existing Dockerfile at %s due to --force" % dockerfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'print_msg'
easybuild/tools/containers/docker.py
Outdated
|
||
module_naming_scheme = ActiveMNS() | ||
|
||
ec = easyconfigs[-1]['ec'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local variable 'ec' is assigned to but never used
easybuild/tools/containers/docker.py
Outdated
res = [] | ||
_os_deps = reduce(operator.add, [obj['ec']['osdependencies'] for obj in easyconfigs], []) | ||
for os_dep in _os_deps: | ||
if isinstance(os_dep, basestring): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'basestring'
easybuild/tools/containers/docker.py
Outdated
|
||
def _det_os_deps(easyconfigs): | ||
res = [] | ||
_os_deps = reduce(operator.add, [obj['ec']['osdependencies'] for obj in easyconfigs], []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'reduce'
0584a88
to
760ead4
Compare
setup.cfg
Outdated
# cfr. https://stackoverflow.com/questions/47427916/how-to-config-hound-ci-to-support-python2-7 | ||
builtins = | ||
basestring, | ||
reduce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx Why add this here too, only belongs in .flake8.ini
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response. setup.cfg
according to the documentation is one the default locations that flake8
checks for configuration. I use flake8
as part of the development workflow ( without specifying the exact location of the config file) so I've replicated that section from .flake8.ini
into the setup.cfg
. Since duplication is obviously not a good thing, I can (1) keep this change (since it's more standard) and remove the reference to .flake8.ini
from the houndci config files or (2) keep using the current way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we change .hound.yml
to use setup.cfg
as config_file
rather than using .flake8.ini
?
If that doesn't cause trouble, let's make that change (maybe in a separate PR) and drop .flake8.ini
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made that change in #2511, together with the pure stylistic changes in easybuild/tools/options.py
Once that's merged, please re-sync this with develop
@FooBarQuaxx This needs another sync with |
5549e35
to
6112814
Compare
I gave this a quick look, and I think it needs a bit of work before it can go into
After (finally) taking a closer look at this, I really feel this should go in first before we start tackling other issues to enhance the current containers support we have (see containers ). @FooBarQuaxx Any idea if and when you would have time to work with us on that? |
I for sure can help on this but I want to get the clear direction. If the general consensus here is to make use of |
It seems some jobs are failing because of an unavailable python package. |
@FooBarQuaxx I've retriggered the tests, they were failing because some kind of caching issue (we released a new version of |
@boegel I've done some refactoring according to your review on a branch other that this PR's branch. Please give feedback on whether the layout of the code is any better. Note that I intentional left out tests and code-style-validation/docstring/headers. I didn't wanted to bother creating those before getting your opinion about the shape of new code. The commit is here and the branch name on my fork is named docker-packaging-refactoring. Thanks. |
@FooBarQuaxx The restructured code looks indeed more what I had in mind, and the switch to object orientation for supporting both Docker & Singularity makes a lot of sense. There are a couple of details that I may want see changed, but overall this is a lot closer to something we can move forward with imho, thanks a lot! So, let's update this PR accordingly and tackle the details? |
@FooBarQuaxx Any updates? |
@boegel Yes, the plan is to update this PR anytime during next week. |
test/framework/containers.py
Outdated
|
||
self.run_main(base_args + ['--container-base=centos:7'], raise_error=True) | ||
|
||
error_pattern = "Container recipe at %s/containers/Dockerfile.toy-0.0 already exists, not overwriting it without --force" % self.test_prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (148 > 120 characters)
easybuild/tools/containers/utils.py
Outdated
tool_version = res.group(0) | ||
version_ok = LooseVersion(str(min_tool_version)) <= LooseVersion(tool_version) | ||
if version_ok: | ||
print_msg("{0} version '{1}' is {2} or higher ... OK".format(tool_name.capitalize(), tool_version, min_tool_version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (125 > 120 characters)
print_msg("WARNING: overwriting existing container image at %s due to --force" % img_path) | ||
remove_file(img_path) | ||
else: | ||
raise EasyBuildError("Container image already exists at %s, not overwriting it without --force", img_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (122 > 120 characters)
easybuild/tools/containers/base.py
Outdated
if self._force: | ||
print_msg("WARNING: overwriting existing container recipe at %s due to --force" % recipe_path) | ||
else: | ||
raise EasyBuildError("Container recipe at %s already exists, not overwriting it without --force", recipe_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (126 > 120 characters)
print_msg("WARNING: overwriting existing container image at %s due to --force" % img_path) | ||
remove_file(img_path) | ||
else: | ||
raise EasyBuildError("Container image already exists at %s, not overwriting it without --force", img_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (122 > 120 characters)
test/framework/containers.py
Outdated
|
||
self.run_main(base_args + ['--container-base=centos:7'], raise_error=True) | ||
|
||
error_pattern = "Container recipe at %s/containers/Dockerfile.toy-0.0 already exists, not overwriting it without --force" % self.test_prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (148 > 120 characters)
easybuild/tools/containers/utils.py
Outdated
tool_version = res.group(0) | ||
version_ok = LooseVersion(str(min_tool_version)) <= LooseVersion(tool_version) | ||
if version_ok: | ||
print_msg("{0} version '{1}' is {2} or higher ... OK".format(tool_name.capitalize(), tool_version, min_tool_version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (125 > 120 characters)
easybuild/tools/containers/base.py
Outdated
if self._force: | ||
print_msg("WARNING: overwriting existing container recipe at %s due to --force" % recipe_path) | ||
else: | ||
raise EasyBuildError("Container recipe at %s already exists, not overwriting it without --force", recipe_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (126 > 120 characters)
7b48a61
to
3dbae48
Compare
@boegel The tests are green now. It would be time to give any review for the structure. Afterward, I will try to add some docstring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall structure looks good to me, just a bunch of minor remarks, and missing docstrings
easybuild/tools/containers/common.py
Outdated
from .docker import DockerContainer # noqa | ||
from .singularity import SingularityContainer # noqa | ||
|
||
_log = fancylogger.getLogger('tools.containers.singularity') # pylint: disable=C0103 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx This should say tools.containers.common
easybuild/tools/containers/base.py
Outdated
from .utils import check_tool | ||
|
||
|
||
_log = fancylogger.getLogger('tools.containers.singularity') # pylint: disable=C0103 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use tools.containers.base
but via self.log
(created in __init__
) rather than a global variable _log
easybuild/tools/containers/base.py
Outdated
self._tmpdir = build_option('container_tmpdir') | ||
self._container_build_image = build_option('container_build_image') | ||
self._container_base = build_option('container_base') | ||
self._container_path = container_path() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx Any specific reason you're making these 'hidden' via _
?
I see no need for it, and dropping the _
helps with readability imho...
Style nitpicking: please sort these initializations alphabetically
easybuild/tools/containers/base.py
Outdated
|
||
def validate(self): | ||
if not self._container_build_image: | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wrong place, only call validate
when container_build_image
is True
in generate
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing here is that there are some validations that should be execute for just producing the recipe e.g. the container_base
for docker should be chosen from the list of supported base docker images namely ubuntu:16.04
and centos:7
.
easybuild/tools/containers/base.py
Outdated
from easybuild.tools.filetools import write_file | ||
from easybuild.tools.config import build_option, container_path | ||
from easybuild.tools.build_log import EasyBuildError, print_msg | ||
from .utils import check_tool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx We don't use relative imports anywhere, so make this from easybuild.tools.containers.utils
for consistency?
Also, please sort alphabetically (build_log
, config
, containers
, filetools
)
easybuild/tools/containers/utils.py
Outdated
|
||
def det_os_deps(easyconfigs): | ||
res = set() | ||
_os_deps = reduce(operator.add, [obj['ec']['osdependencies'] for obj in easyconfigs], []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to name it _os_deps
, since it's a local variable, os_deps
is fine?
easybuild/tools/containers/utils.py
Outdated
if not tool_path: | ||
return False | ||
|
||
print_msg("{0} tool found at {1}".format(tool_name.capitalize(), tool_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx I wouldn't use capitalize here, don't see why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that's not needed, but the testing code was using a capitalized string for the docker format. I can get rid of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not capitalizing makes more sense to me
easybuild/tools/containers/utils.py
Outdated
|
||
out, ec = run_cmd("{0} --version".format(tool_name), simple=False, trace=False, force_in_dry_run=True) | ||
if ec: | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some logging would be nice here (show the error message that occurred when running --version
), or even raise an error?
test/framework/containers.py
Outdated
@@ -89,11 +99,11 @@ def test_parse_container_base(self): | |||
expected.update({'arg2': 'bar'}) | |||
self.assertEqual(parse_container_base('%s:foo:bar' % agent), expected) | |||
|
|||
def run_main(self, args): | |||
def run_main(self, args, raise_error=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx Leave the default to True
for raise_error
?
test/framework/containers.py
Outdated
@@ -120,6 +130,7 @@ def test_end2end_singularity_recipe(self): | |||
args = [ | |||
toy_ec, | |||
'--containerize', | |||
'--container-type=singularity', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed (since default type is Singularity)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that will be explicit and clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but this way you're not testing what the default container type is, since it's always specified.
Leaving it unspecified is intentional here, it serves as a check that Singularity is the default.
easybuild/tools/containers/docker.py
Outdated
def __init__(self, *args, **kwargs): | ||
super(DockerContainer, self).__init__(*args, **kwargs) | ||
# NOTE (med): set default value for _container_base | ||
self._container_base = self._container_base or DEFAULT_DOCKER_BASE_IMAGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx Isn't this something that should be covered in tools/config.py
, where we define defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state of the --container-base
option is kind of non consistent. For singularity, the option does not allow for an empty value so it does not default to anything. But for docker, we're defaulting to ubuntu:16.04
. I'll change it to also be required for docker as well.
3dbae48
to
ed8c92e
Compare
easybuild/tools/containers/base.py
Outdated
self.easyconfigs = easyconfigs | ||
self.image_format = build_option('container_image_format') | ||
self.img_name = build_option('container_image_name') | ||
self.log = fancylogger.getLogger(self.__class__.__module__.replace('easybuild.', '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please initialize the logger with fancylogger.getLogger(self.__class__.__name__, fname=False)
like we do elsewhere
easybuild/tools/containers/docker.py
Outdated
RECIPE_FILE_NAME = 'Dockerfile' | ||
|
||
def resolve_template(self): | ||
return (2 * "\n").join([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nitpicking: I'd go with '\n\n'
, looks a bit weird otherwise...
easybuild/tools/containers/docker.py
Outdated
} | ||
|
||
def validate(self): | ||
if self.container_base not in [DOCKER_BASE_IMAGE_UBUNTU, DOCKER_BASE_IMAGE_CENTOS]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx does it make sense to use DOCKER_OS_INSTALL_DEPS_TMPLS.keys()
here?
|
||
from easybuild.tools.build_log import EasyBuildError, print_msg | ||
from easybuild.tools.config import CONT_IMAGE_FORMAT_EXT3, CONT_IMAGE_FORMAT_SANDBOX, CONT_IMAGE_FORMAT_SQUASHFS | ||
from easybuild.tools.config import container_path, build_option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nitpicking: fix sorting here, build_option
goes first
easybuild/tools/containers/utils.py
Outdated
|
||
out, ec = run_cmd("{0} --version".format(tool_name), simple=False, trace=False, force_in_dry_run=True) | ||
if ec: | ||
print_error("command '{0} --version' failed with output: {1}".format(tool_name, out)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be raise EasyBuildError
, since print_error
is not fatal?
cfr. original behaviour in check_singularity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_tool
is a predicate (returns a boolean) so raising an exception IMO is the responsibility of the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx We also raise an error when we fail to parse the version...
If running --version
doesn't work, there's something seriously wrong, so the right thing to do is to exit with an error straight away I think...
test/framework/containers.py
Outdated
"module load toy/0.0 GCC/4.9.2", | ||
] | ||
self.check_regexs(regexs, def_file) | ||
remove_file(os.path.join(self.test_prefix, 'containers', 'Dockerfile.toy-0.0')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed strictly speaking, the test-specific temporary directory at self.test_prefix
is automatically cleaned up for each test
|
||
class SingularityContainer(ContainerGenerator): | ||
|
||
TOOLS = {'singularity': '2.4'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FooBarQuaxx sudo
is also required here?
easybuild/tools/containers/base.py
Outdated
def validate_tools(self): | ||
for tool_name, tool_version in self.TOOLS.items(): | ||
if not check_tool(tool_name, tool_version): | ||
raise EasyBuildError("{0} not found on your system.".format(tool_name,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the tool is there, but it's not the correct version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest changing the message or being more precise about the non valid condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the message to also include the version requirement (if there is one)
easybuild/tools/containers/docker.py
Outdated
def resolve_template_data(self): | ||
os_deps = det_os_deps(self.easyconfigs) | ||
|
||
module_naming_scheme = ActiveMNS() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we need this in two places, should we define a self.mns
in __init__
?
from easybuild.tools.run import run_cmd | ||
|
||
|
||
def det_os_deps(easyconfigs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more missing docstrings here
easybuild/tools/containers/utils.py
Outdated
version_cmd = "{0} --version".format(tool_name) | ||
out, ec = run_cmd(version_cmd, simple=False, trace=False, force_in_dry_run=True) | ||
if ec: | ||
print_error("command '{0}' failed with output: {1}".format(version_cmd, out)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more need for print_error
if an error is being raised?
This looks good to go, thanks a lot for all your efforts here @FooBarQuaxx! There's certainly room for more improvement, but I don't see a need to hold this PR back any further, so I'll merge it so these changes are included in EasyBuild v3.6.2 . Further enhancements can be done step by step, following the relevant issues in containers . An update to the documentation (http://easybuild.readthedocs.io/en/latest/Containers.html, via https://github.com/easybuilders/easybuild/tree/master/docs) would make sense too, if you're up for it @FooBarQuaxx ... |
@boegel Thanks for assisting and accepting my contribution to this awesome project. |
This is the initial implementation of packaging easybuild buids into containers.