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.

Sign up for GitHub

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

Call reset when remove profile #189

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Call reset when remove profile #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f03ef7
call reset when remove profile
unkcpz Aug 14, 2023
0e6f26b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 14, 2023
b1bc7dc
Add test
unkcpz Sep 5, 2023
a0ebfd3
f-d
unkcpz Sep 5, 2023
a4549ff
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 5, 2023
79eb2db
f-d
unkcpz Oct 2, 2023
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
47 changes: 26 additions & 21 deletions aiidalab_launch/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,34 +229,39 @@ def add_profile(ctx, app_state, port, home_mount, image, profile):
@click.argument("profile")
@click.option("--yes", is_flag=True, help="Do not ask for confirmation.")
@click.option("-f", "--force", is_flag=True, help="Proceed, ignoring any warnings.")
@click.option("--purge", is_flag=True, help="Remove all data associated with profile.")
@pass_app_state
def remove_profile(app_state, profile, yes, force):
@click.pass_context
def remove_profile(ctx, app_state, profile, yes, force, purge):
"""Remove an AiiDAlab profile from the configuration."""
try:
profile = app_state.config.get_profile(profile)
except ValueError:
raise click.ClickException(f"Profile with name '{profile}' does not exist.")
else:
if not force:
instance = AiidaLabInstance(client=app_state.docker_client, profile=profile)
status = asyncio.run(instance.status())
if status not in (
instance.AiidaLabInstanceStatus.DOWN,
instance.AiidaLabInstanceStatus.CREATED,
instance.AiidaLabInstanceStatus.EXITED,
):
raise click.ClickException(
f"The instance associated with profile '{profile.name}' "
"is still running. Use the -f/--force option to remove the "
"profile anyways."
)

if yes or click.confirm(
f"Are you sure you want to remove profile '{profile.name}'?"
if not force:
instance = AiidaLabInstance(client=app_state.docker_client, profile=profile)
status = asyncio.run(instance.status())
if status not in (
instance.AiidaLabInstanceStatus.DOWN,
instance.AiidaLabInstanceStatus.CREATED,
instance.AiidaLabInstanceStatus.EXITED,
):
app_state.config.profiles.remove(profile)
app_state.save_config()
click.echo(f"Removed profile with name '{profile.name}'.")
raise click.ClickException(
f"The instance associated with profile '{profile.name}' "
"is still running. Use the -f/--force option to remove the "
"profile anyways."
)

if purge:
unkcpz marked this conversation as resolved.
Show resolved Hide resolved
ctx.invoke(reset, profile=profile, yes=yes)

if yes or click.confirm(
f"Are you sure you want to remove profile '{profile.name}'?"
):
app_state.config.profiles.remove(profile)
app_state.save_config()
click.echo(f"Removed profile with name '{profile.name}'.")


@profile.command("edit")
Expand Down Expand Up @@ -696,7 +701,7 @@ def reset(app_state, profile, yes):
)

click.secho(
f"Resetting instance for profile '{profile.name}'. This action cannot be undone!",
f"Resetting instance for profile '{profile.name}'. This will remove ALL DATA, including the docker container and the associated volumes. This action cannot be undone!",
err=True,
fg="red",
)
Expand Down
13 changes: 13 additions & 0 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,13 @@ def test_add_remove_profile():
assert result.exit_code == 0
assert "Added profile 'new-profile'." in result.output

# Add second profile to be clean with delete volume
runner.invoke(cli.cli, ["profile", "add", "second-profile"], input="n\n")

# Check that new-profile exists
result: Result = runner.invoke(cli.cli, ["profile", "list"])
assert "new-profile" in result.output
unkcpz marked this conversation as resolved.
Show resolved Hide resolved
assert "second-profile" in result.output
result: Result = runner.invoke(cli.cli, ["profile", "show", "new-profile"])
assert result.exit_code == 0

Expand Down Expand Up @@ -138,6 +142,15 @@ def test_add_remove_profile():
assert result.exit_code == 1
assert "Profile with name 'new-profile' does not exist." in result.output

# Remove second-profile with reset (`--purge` option)
result: Result = runner.invoke(
cli.cli,
["profile", "remove", "--purge", "second-profile"],
input="y\nsecond-profile\n",
)
assert result.exit_code == 0
assert "Please enter the name of the profile to continue" in result.output
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also assert that the volumes were indeed deleted, while for the first profile we should assert that they were NOT deleted.

Because the volumes are not automatically created when the profile is created, we probably need a fixture for that. I think there should be be an existing fixture you might be able to reuse or use directly).

Copy link
Member Author

@unkcpz unkcpz Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't test this since it was covered by the TestLifeCycle below. This pr does not include adding new feature but it calls reset to remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to move get_volume out and reuse it for this purge option, but give up since I realize it break the good design of using instance fixture as the class scope and cleaned up it after the test was finalized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking a look. I think my idea was more simple. Instead of launching the whole lifecycle, you would create the docker volumes manually in the fixture.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a second try and found the problem is the profile and container serve two different cases. When user run aiidalab-launch profile it only create the profile but did nothing with container and the volume. It is when the profile first time used the container and its volumes are created.

If you try with -vvv with this pr branch, you'll find that before the newly creating profile first time running, running with --purge will go through reset which manipulate docker container directly and hit the code path

aiidalab-launch/aiidalab_launch/instance.py

Lines 239 to 242 in 33c7a26

except docker.errors.NotFound: # already removed
LOGGER.debug(
f"Failed to remove conda volume '{self.profile.conda_volume_name()}', likely already removed."
)

It won't cause any issues because it logs to the debug level. I am now not very confident in myself it is a good way to implement the purge here which mixes the profile and container controlling part of the code.
From what you are asking here, it is useless to create docker volume fixture because in this test, only the profile is created but not the container. So it has to go to the real life cycle which has the container running with volumes created.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I think the profile concept is not very useful to end user. If we regard aiidalab-launch as a wrapper of a combination of multiple docker commands, it maybe makes sense to have the CLI command match with the docker command which are:

  • docker run -> aiidalab-launch run: to start from an image. (this will be the combination of current aiidalab-launch profile add + aiidalab-launch profile start)
  • docker stop -> aiidalab-launch stop: stop the container, it is the same from what it is now.
  • docker rm -> aiidalab-launch rm: remove the container (for aiidalab-launch case also remove the volume), this is then what I was going to achieve in this PR. It is the combination of the current aiidalab-launch reset and aiidalab-launch profile remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @danielhollas, can you give this a look? We can then decide we halt this PR or continue it with brining some mixture of profile and container commands.



def test_add_profile_invalid_name():
runner: CliRunner = CliRunner()
Expand Down