Call reset when remove profile

[unkcpz]

fixes #172

The aiidalab-launch reset can be used to purge the data mounted such as the home mount as a docker volume and conda volume mounted.
Usually, when deleting the profile, users are supposed to reset the data as well, in this PR, I call reset before the profile remove.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (33c7a26) 86.32% compared to head (afb325b) 86.38%.

❗ Current head afb325b differs from pull request most recent head 79eb2db. Consider uploading reports for the commit 79eb2db to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   86.32%   86.38%   +0.05%     
==========================================
  Files           9        9              
  Lines         914      918       +4     
==========================================
+ Hits          789      793       +4     
  Misses        125      125              

Flag	Coverage Δ
py-3.10	86.27% <100.00%> (+0.06%)	⬆️
py-3.11	86.27% <100.00%> (+0.06%)	⬆️
py-3.8	86.22% <100.00%> (+0.06%)	⬆️
py-3.9	86.33% <100.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
aiidalab_launch/__main__.py	79.87% <100.00%> (+0.25%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[danielhollas]

@unkcpz sorry for a very delayed review. Code looks good, but please add tests for this. This is a very desctructive feature so we need to be super careful. I will then do careful testing locally. Thanks!

[unkcpz]

Thanks @danielhollas
The test is added.

I believe this code should be called at the very end right?

I was thinking about this as well, but still have not convinced myself although it makes more sense in logic. Because once the profile is removed there is no way to remove volume through the command line again. However, if the profile deletes is failed after cleaning the volume, the profile still can be deleted.

Also please add a specific WARNING and confirmation for this case.

This is taken care of by reset already, it asks the user to input the profile name to confirm.

Anyway, I moved the purge to the end after the profile was removed, if you think it is better to move it before, I'll change it.

[unkcpz]

Hi @danielhollas, I think this one is ready to have a second review.

[danielhollas]

@unkcpz Appologies for long delay on review, I wanted to test things carefully. I have some final comments and then we should be good to go.

[danielhollas]

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).

[unkcpz]

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.

[unkcpz]

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.

[danielhollas]

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.

[unkcpz]

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.

[unkcpz]

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.

[unkcpz]

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.

[danielhollas]

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.