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: adjust optionEatAll for click >= 8 #492

Merged
merged 3 commits into from
Jan 14, 2023
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
7 changes: 5 additions & 2 deletions neurodocker/cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
def cli():
"""Generate custom containers, and minify existing containers.

The minify command is available only if Docker is installed and running.
Note: The minify command has been removed due to lack of ptrace in docker.
"""


Expand All @@ -20,11 +20,14 @@ def cli():
# `docker-py` is required for minification but is not installed by default.
# We also pass if there is an error retrieving the Docker client.
# Say, for example, the Docker engine is not running (or it is not installed).
"""
# Removing minify due to lack of ptrace availability in docker

try:
from neurodocker.cli.minify.trace import minify

cli.add_command(minify)
except (ImportError, RuntimeError):
# TODO: should we log a debug message? We don't even have a logger at the
# time of writing, so probably not.
pass
"""
2 changes: 1 addition & 1 deletion neurodocker/cli/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ def parser_process(value, state):
if not done:
value.append(state.rargs.pop(0))
value = tuple(value)

# call the actual process
self._previous_parser_process(value, state)

Expand Down Expand Up @@ -221,6 +220,7 @@ def _get_common_renderer_params() -> ty.List[click.Parameter]:
OptionEatAll(
["--install"],
multiple=True,
type=tuple,
help="Install packages with system package manager",
),
OptionEatAll(
Expand Down
6 changes: 2 additions & 4 deletions neurodocker/cli/minify/_trace.sh
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,12 @@ function install_missing_dependencies() {
function install_conda_reprozip() {
TMP_CONDA_INSTALLER=/tmp/miniconda.sh
ls /tmp
curl -sSL -o "$TMP_CONDA_INSTALLER" "$CONDA_URL"
ls /tmp
curl -sSL -o "$TMP_CONDA_INSTALLER" "https://github.com/conda-forge/miniforge/releases/latest/download/Mambaforge-$(uname)-$(uname -m).sh"
bash $TMP_CONDA_INSTALLER -b -f -p $REPROZIP_CONDA
rm -f $TMP_CONDA_INSTALLER
${REPROZIP_CONDA}/bin/python -m pip install --no-cache-dir reprozip
${REPROZIP_CONDA}/bin/mamba install -c conda-forge -y reprozip
}


function run_reprozip_trace() {
# https://askubuntu.com/a/674347
cmds=("$@")
Expand Down
6 changes: 5 additions & 1 deletion neurodocker/cli/minify/tests/test_minify.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
docker = pytest.importorskip("docker", reason="docker-py not found")


@pytest.mark.skip(reason="ptrace no longer supported under docker")
def test_minify():
client = docker.from_env()
container = client.containers.run("python:3.9-slim", detach=True, tty=True)
container = client.containers.run("python:3.9-slim", detach=True, tty=True,
platform="Linux/amd64", privileged=True)
Comment on lines +14 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if adding cap_add=["SYS_PTRACE"] would fix this. not a priority for now but hopefully we can get this working again. though I wonder if privileged=True implies that.

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 have tried all variants, none of them worked on my m1 mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and from docker's docs it seems they may have fully disabled this.

commands = ["python --version", """python -c 'print(123)'"""]
try:
runner = CliRunner()
Expand All @@ -37,6 +39,7 @@ def test_minify():
container.remove()


@pytest.mark.skip(reason="ptrace no longer supported under docker")
def test_minify_abort():
client = docker.from_env()
container = client.containers.run("python:3.9-slim", detach=True, tty=True)
Expand Down Expand Up @@ -65,6 +68,7 @@ def test_minify_abort():
container.remove()


@pytest.mark.skip(reason="ptrace no longer supported under docker")
def test_minify_with_mounted_volume(tmp_path: Path):
client = docker.from_env()

Expand Down
2 changes: 1 addition & 1 deletion neurodocker/reproenv/tests/test_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def test_validate_template_invalid_templates():
_validate_template({})

with pytest.raises(
exceptions.TemplateError, match="'binaries' is a required property"
exceptions.TemplateError, match="{'name': 'bar'} is not valid"
):
_validate_template({"name": "bar"})

Expand Down