Feature: extra mounts #132

danielhollas · 2022-06-16T22:20:45Z

Closes #130

This needs tests and probably more error handling, but I'd like to have some eyes on this early to confirm if I am on the right track here.

CC @csadorf

aiidalab_launch/profile.py

danielhollas · 2022-06-16T22:25:24Z

aiidalab_launch/util.py

@@ -127,7 +127,7 @@ def iter_to_queue() -> None:
    return yield_queue_items()


-def get_latest_version(timeout: float = 0.1) -> Optional[Version]:
+def get_latest_version(timeout: float = 0.1) -> Union[LegacyVersion, Version, None]:


This is an unrelated mypy fix

Could you elaborate a bit further on this? Does this occur with a specific mypy version? We are running mypy checks as part of the CI so that should have come up.

Here's the output from conda list

_libgcc_mutex 0.1 main _openmp_mutex 5.1 1_gnu aiidalab-launch 2022.1013 dev_0 <develop> appdirs 1.4.4 pypi_0 pypi attrs 21.4.0 pypi_0 pypi bumpver 2021.1114 pypi_0 pypi bzip2 1.0.8 h7f98852_4 conda-forge ca-certificates 2022.6.15 ha878542_0 conda-forge cattrs 1.10.0 pypi_0 pypi certifi 2022.6.15 py39hf3d152e_0 conda-forge cffi 1.15.0 py39h4bc2ebd_0 conda-forge cfgv 3.3.1 pyhd8ed1ab_0 conda-forge charset-normalizer 2.0.12 pypi_0 pypi click 8.0.3 pypi_0 pypi click-spinner 0.1.10 pypi_0 pypi colorama 0.4.4 pypi_0 pypi coverage 6.4.1 pypi_0 pypi distlib 0.3.4 pyhd8ed1ab_0 conda-forge docker 5.0.3 pypi_0 pypi dunamai 1.7.0 pypi_0 pypi filelock 3.7.1 pyhd8ed1ab_0 conda-forge identify 2.5.1 pyhd8ed1ab_0 conda-forge idna 3.3 pypi_0 pypi iniconfig 1.1.1 pypi_0 pypi ld_impl_linux-64 2.38 h1181459_1 lexid 2021.1006 pypi_0 pypi libffi 3.4.2 h7f98852_5 conda-forge libgcc-ng 11.2.0 h1234567_1 libgomp 11.2.0 h1234567_1 libnsl 2.0.0 h7f98852_0 conda-forge libstdcxx-ng 11.2.0 h1234567_1 libuuid 2.32.1 h7f98852_1000 conda-forge libzlib 1.2.11 h166bdaf_1014 conda-forge mypy 0.910 pyhd3eb1b0_0 mypy_extensions 0.4.3 py39h06a4308_1 ncurses 6.3 h7f8727e_2 nodeenv 1.6.0 pyhd8ed1ab_0 conda-forge openssl 3.0.3 h166bdaf_0 conda-forge packaging 21.3 pypi_0 pypi pathlib2 2.3.7.post1 pypi_0 pypi pip 21.2.4 py39h06a4308_0 platformdirs 2.5.1 pyhd8ed1ab_0 conda-forge pluggy 1.0.0 pypi_0 pypi pre-commit 2.19.0 py39hf3d152e_0 conda-forge psutil 5.8.0 py39h27cfd23_1 py 1.11.0 pypi_0 pypi pycparser 2.21 pyhd8ed1ab_0 conda-forge pyparsing 3.0.9 pypi_0 pypi pytest 6.2.5 pypi_0 pypi pytest-asyncio 0.17.2 pypi_0 pypi pytest-cov 3.0.0 pypi_0 pypi python 3.9.12 h2660328_1_cpython conda-forge python-dotenv 0.19.1 pypi_0 pypi python_abi 3.9 2_cp39 conda-forge pyyaml 6.0 py39hb9d737c_4 conda-forge readline 8.1.2 h7f8727e_1 requests 2.26.0 pypi_0 pypi requests-cache 0.9.1 pypi_0 pypi responses 0.18.0 pypi_0 pypi setuptools 61.2.0 py39h06a4308_0 six 1.16.0 pyh6c4a22f_0 conda-forge sqlite 3.38.5 h4ff8645_0 conda-forge tabulate 0.8.9 pypi_0 pypi tk 8.6.12 h27826a3_0 conda-forge toml 0.10.2 pyhd3eb1b0_0 tomli 2.0.1 pypi_0 pypi types-click-spinner 0.1.11 pypi_0 pypi types-pyyaml 6.0.8 pypi_0 pypi types-redis 4.2.7 pypi_0 pypi types-requests 2.27.30 pypi_0 pypi types-tabulate 0.8.9 pypi_0 pypi types-toml 0.10.7 pypi_0 pypi types-ujson 5.3.0 pypi_0 pypi types-urllib3 1.26.15 pypi_0 pypi typing_extensions 4.1.1 pyh06a4308_0 tzdata 2022a hda174b7_0 ukkonen 1.0.1 py39hf939315_2 conda-forge url-normalize 1.4.3 pypi_0 pypi urllib3 1.26.9 pypi_0 pypi virtualenv 20.14.1 py39hf3d152e_0 conda-forge websocket-client 1.3.2 pypi_0 pypi wheel 0.37.1 pyhd3eb1b0_0 xz 5.2.5 h7f8727e_1 yaml 0.2.5 h7f98852_2 conda-forge zlib 1.2.11 h166bdaf_1014 conda-forge

We are using mypy version 0.961, not 0.910 . I am doubtful that this is the issue, but would be at least worth to check. Are the pre-commit hooks passing for you?

Pre-commit hooks are passing. I have updated to 0.961 but the issue persisted. But I am happy to revert this at the end (keeping for now so mypy output is clean during development).

Sounds good! Maybe it's a Python version issue? I see that you use Python 3.9 whereas I usually use Python 3.10 for development of this package.

codecov · 2022-06-16T22:35:23Z

Codecov Report

Merging #132 (b840d05) into main (891d74e) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   86.19%   86.47%   +0.27%     
==========================================
  Files           9        9              
  Lines         826      850      +24     
==========================================
+ Hits          712      735      +23     
- Misses        114      115       +1

Flag	Coverage Δ
py-3.10	`?`
py-3.7	`86.47% <100.00%> (+0.89%)`	⬆️
py-3.8	`?`
py-3.9	`?`

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

Impacted Files	Coverage Δ
aiidalab_launch/__main__.py	`78.00% <100.00%> (-1.14%)`	⬇️
aiidalab_launch/application_state.py	`100.00% <100.00%> (ø)`
aiidalab_launch/config.py	`100.00% <100.00%> (ø)`
aiidalab_launch/instance.py	`89.72% <100.00%> (-0.09%)`	⬇️
aiidalab_launch/profile.py	`96.62% <100.00%> (+1.71%)`	⬆️
aiidalab_launch/util.py	`83.60% <100.00%> (+0.84%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 891d74e...b840d05. Read the comment docs.

csadorf

Thanks for the draft! I think this is a very good start. I've tried to address all of your comments and questions.

From a conceptual level I think this looks sound.

aiidalab_launch/__main__.py

aiidalab_launch/instance.py

aiidalab_launch/profile.py

csadorf · 2022-06-17T08:05:02Z

aiidalab_launch/util.py

@@ -127,7 +127,7 @@ def iter_to_queue() -> None:
    return yield_queue_items()


-def get_latest_version(timeout: float = 0.1) -> Optional[Version]:
+def get_latest_version(timeout: float = 0.1) -> Union[LegacyVersion, Version, None]:


Could you elaborate a bit further on this? Does this occur with a specific mypy version? We are running mypy checks as part of the CI so that should have come up.

aiidalab_launch/instance.py

danielhollas · 2022-06-21T02:34:04Z

@csadorf Thank you for the initial review and detailed comments. I implemented your suggestions, added a readonly option ("source:target:readonly") and added determination of extra mounts for the container (needed for determining whether configuration changed underneath a running container).

I still need to add tests. I am not very familiar with pytest yet, so pointers welcome.

EDIT: Just realized I also need to write a migration to convert old config files to include the new option, perhaps in

aiidalab-launch/aiidalab_launch/application_state.py

Line 66 in 73fe854

def apply_migrations(self):

?

csadorf · 2022-06-21T10:24:46Z

@csadorf Thank you for the initial review and detailed comments. I implemented your suggestions, added a readonly option ("source:target:readonly") and added determination of extra mounts for the container (needed for determining whether configuration changed underneath a running container).

That's great, very much appreciated. Since we are essentially supporting the "short-syntax" from docker-compose, I would recommend that we use "rw" (implied) and "ro" to indicate read-write and read-only mounts.

I still need to add tests. I am not very familiar with pytest yet, so pointers welcome.

All tests are implemented in the tests/ directory, we use a hierarchy of test fixtures to isolate the tests from the actual user environment (we do not want to mess with the user's actual aiidalab containers and volumes). Since all of that is already taking care of, you don't typically have to worry about that too much and can just pattern-match the existing tests. Maybe I can write a short summary of how to develop tests and add it to the README?

EDIT: Just realized I also need to write a migration to convert old config files to include the new option, perhaps in

aiidalab-launch/aiidalab_launch/application_state.py

Line 66 in 73fe854

def apply_migrations(self):

?

Yes, I think it might be as simple as always updating the config version instead of only updating it when there is no previous version stored; something like this:

        # Update config version if necessary
        if self.config.version != str(parse(__version__))
            self.config.version = str(parse(__version__))
            config_changed = True

The save_config() function should write the whole config (including defaults – to be verified).

csadorf · 2022-06-22T10:45:21Z

@danielhollas Please re-request my review when you want me to have another detailed look at this. Thank you!

danielhollas · 2022-06-24T02:20:27Z

Since we are essentially supporting the "short-syntax" from docker-compose, I would recommend that we use "rw" (implied) and "ro" to indicate read-write and read-only mounts.
Huh. So I was originally looking at these this Docker docs which make it seems like the third argument can function as a list of options. But I tested that it doesn't working with my Docker version so I guess those Docs are outdated. So I went with your suggestion for ro|rw, with the default being rw and being explicit (e.g. if user does not provide it, we will autocomplete it).

Yes, I think it might be as simple as always updating the config version instead of only updating it when there is no previous version stored; something like this:

        # Update config version if necessary
        if self.config.version != str(parse(__version__))
            self.config.version = str(parse(__version__))
            config_changed = True

Yes, this indeed works and solved the migration issue.

I added a bunch of tests and apart from the printing in __main__.py everything is covered according to Codecov.
If this looks good I can add some Docs, but unfortunately I will not have too much extra time to work on this in the next two weeks (conference time). I will be using this actively so am happy to follow up if I discover any issues once this is released.

aiidalab_launch/profile.py

danielhollas · 2022-06-24T02:26:45Z

tests/test_cli.py

+            extra_volume, _, _ = instance.profile.parse_extra_mount(extra_mount)
+            # TODO: For some reason this assert fails,
+            # the extra mount volumes are not created.
+            # assert get_volume(str(extra_volume))


I tried a bunch of stuff but couldn't make this work...

I think manipulating the instance variable like this may not have any effect on the profile that is picked up by the CLI runner. You could check that by printing the output of runner.involke(cli.cli, ["profiles", "show", "default"]).

I can envision two ways to address this:

Create an additional fixture named use_extra_volumes or so that actually changes the configuration when requested.

Just configure all tests to use extra_volumes.

I don't like the 2nd approach, because it means that we are moving further away from the user default config in our tests, which reduces our test effectiveness, however it might be a good way to debug this problem.

I have already tried the second approach by modifying the Profile fixture, but that didn't work. I think we would need to go deeper and modify the config fixture.

I would need to think more about how to create use_extra_volumes fixture.

csadorf

Looks good overall! I have left a few comments and suggestions and my recommendation on how we might be able to address that test issue.

aiidalab_launch/__main__.py

tests/conftest.py

aiidalab_launch/profile.py

tests/test_profile.py

csadorf · 2022-06-24T08:32:20Z

tests/test_cli.py

+            extra_volume, _, _ = instance.profile.parse_extra_mount(extra_mount)
+            # TODO: For some reason this assert fails,
+            # the extra mount volumes are not created.
+            # assert get_volume(str(extra_volume))


I think manipulating the instance variable like this may not have any effect on the profile that is picked up by the CLI runner. You could check that by printing the output of runner.involke(cli.cli, ["profiles", "show", "default"]).

I can envision two ways to address this:

Create an additional fixture named use_extra_volumes or so that actually changes the configuration when requested.

Just configure all tests to use extra_volumes.

I don't like the 2nd approach, because it means that we are moving further away from the user default config in our tests, which reduces our test effectiveness, however it might be a good way to debug this problem.

aiidalab_launch/profile.py

danielhollas · 2022-06-24T17:59:57Z

@csadorf thanks, I have implemented the suggestions. Still not quite sure how to deal with the test_cli test, but I have unfortunately found a more pressing issue with the Profile.from_container which results in the test flakiness, see this comment for more details.

https://github.com/aiidalab/aiidalab-launch/pull/132/files#r906282515

EDIT: I think I solved the issue by converting extra_mounts into set(). I ran the test_instance.py tests many times and did not spot any flakiness.

for i in `seq 1 100`; do pytest --no-cov tests/test_instance.py; if [ $? -ne 0 ]; then break;fi;done

EDIT2: Also added a short documentation in README.md.

csadorf

Looks like this is almost ready! 😃 I've tested it locally and everything seems to be working as expected.

I've identified one bug and proposed a fix as well as merged a unit test that catches it in #136. Maybe you can rebase this branch so that we can test against it?

I have a few more minor change requests, but should all be easily addressable.

One last thing, I noticed that the volume is root-owned by default. That's expected, however should we try to address that somehow? I think we should not mess with ownership and permissions by default, but maybe it could be an option? Please let me know what you suggest.

tests/conftest.py

aiidalab_launch/config.py

README.md

This reverts commit 318733f.

This reverts commit 192a1d0.

This reverts commit 0ab5d66.

danielhollas · 2022-06-27T16:00:34Z

@csadorf thanks for shepherding this PR. :-) I've rebased on main and implemented your bugfix, thanks for catching that.

One last thing, I noticed that the volume is root-owned by default. That's expected, however should we try to address that somehow? I think we should not mess with ownership and permissions by default, but maybe it could be an option? Please let me know what you suggest.

I am confused, what exactly do you mean by root-owned? In my testing the extra mounts are owned by the aiida user in the container, same as the home_mount and conda volume.

csadorf · 2022-06-28T07:32:55Z

@csadorf thanks for shepherding this PR. :-) I've rebased on main and implemented your bugfix, thanks for catching that.

One last thing, I noticed that the volume is root-owned by default. That's expected, however should we try to address that somehow? I think we should not mess with ownership and permissions by default, but maybe it could be an option? Please let me know what you suggest.

I am confused, what exactly do you mean by root-owned? In my testing the extra mounts are owned by the aiida user in the container, same as the home_mount and conda volume.

When mounting a volume into a container the UID and GID are not changed. That means for a bind mount that the ownership of the source directory are maintained and you may not have access to the directory if the user in the container has a different UID. This blog article might provide additional insight.

For aiidalab-launch we currently use the "aiida" user, which has a UID and GID of 1000. That's the standard user id for the first user on most distributions so in general this will work just fine. On my system it is the UID of the root user which means I don't automatically have access to it. Long story short, since you are not encountering this problem and we can assume that most people won't, let's maybe hold off on implementing a "solution" to it since it could be easily done in a separate change set.

Since it is indeed not actually a random volume name, but the extra volume name for a specific class-scoped test instance.

csadorf

@danielhollas I think this is ready, thank you very much! 🥳

Let me know if there is any final thing you want to change or add, otherwise I'll go ahead and merge this.

danielhollas · 2022-06-28T15:48:49Z

@csadorf awesome, thank you for the final touches, lgtm.

danielhollas · 2022-06-29T12:26:57Z

@csadorf thanks for merging! 🚀 Would you be able to create a new release today or tomorrow?

csadorf · 2022-06-29T15:01:19Z

@csadorf thanks for merging! 🚀 Would you be able to create a new release today or tomorrow?

Done.

https://pypi.org/project/aiidalab-launch/2022.1014/

Let me know if there is any issue, I can make a new release very quickly.

danielhollas · 2022-06-29T16:36:43Z

@csadorf excellent, thank you very much. I have just tested it and it seems to work. btw; on another computer I am indeed running into the permission issue with the mounted volume that you mentioned above. I'll take a closer look at that medium post and think about what could be done (although it might take me some time before I get to it). For now the simple fix is to chmod add access to all users, which in my case is okay but not a great solution in general.

danielhollas commented Jun 16, 2022

View reviewed changes

aiidalab_launch/profile.py Outdated Show resolved Hide resolved

danielhollas commented Jun 16, 2022

View reviewed changes

aiidalab_launch/profile.py Outdated Show resolved Hide resolved

danielhollas commented Jun 16, 2022

View reviewed changes

aiidalab_launch/profile.py Outdated Show resolved Hide resolved

danielhollas commented Jun 16, 2022

View reviewed changes

danielhollas force-pushed the custom-mounts branch from 7af8c32 to 4ac086c Compare June 16, 2022 22:29

csadorf reviewed Jun 17, 2022

View reviewed changes

danielhollas changed the title ~~WIP: Feature Custom mounts~~ Feature Custom mounts Jun 21, 2022

danielhollas commented Jun 21, 2022

View reviewed changes

aiidalab_launch/instance.py Outdated Show resolved Hide resolved

danielhollas mentioned this pull request Jun 21, 2022

Fix mount configuration change check for bind mounts #134

Merged

danielhollas changed the title ~~Feature Custom mounts~~ Feature: extra mounts Jun 22, 2022

danielhollas requested a review from csadorf June 24, 2022 02:20

danielhollas commented Jun 24, 2022

View reviewed changes

aiidalab_launch/profile.py Outdated Show resolved Hide resolved

danielhollas commented Jun 24, 2022

View reviewed changes

danielhollas marked this pull request as ready for review June 24, 2022 02:27

csadorf reviewed Jun 24, 2022

View reviewed changes

danielhollas commented Jun 24, 2022

View reviewed changes

aiidalab_launch/profile.py Show resolved Hide resolved

danielhollas requested a review from csadorf June 27, 2022 13:48

danielhollas force-pushed the custom-mounts branch from 96a8202 to 5869135 Compare June 27, 2022 14:07

csadorf suggested changes Jun 27, 2022

View reviewed changes

danielhollas added 5 commits June 27, 2022 16:23

Fix return type in utils.py

c268fed

Feature: Custom volume mounts

5b2e7cb

Implement readonly option, implement feedback

ba2ab80

Determine extra mounts from the container

aa292b3

Revert "Fix mount configuration change check for bind mounts"

0ab5d66

This reverts commit 318733f.

danielhollas added 15 commits June 27, 2022 16:23

mode=ro|rw + first tests

c74ca5a

Improve profile tests

db2d0e7

Add basic instance check

5c83bbb

Fix profile tests

a30f134

Cleanup extra volumes in instance fixture

f6afe16

Add unsuccefull attempt in test_cli

beb904c

Minor updates based on review

aa14d7e

valid_volume_name in utils

823aa67

move valid_volume_name to profile.py

643fed8

Convert extra_mounts from list to set

967eb04

Revert "Fix return type in utils.py"

74774c5

This reverts commit 192a1d0.

Add docs to README.md

f517dac

Revert test_cli changes

33cb947

Fix config.py

256236f

Address review suggestions

53cb870

danielhollas force-pushed the custom-mounts branch from 0c9a559 to 53cb870 Compare June 27, 2022 15:42

Revert "Revert "Fix mount configuration change check for bind mounts""

dd26607

This reverts commit 0ab5d66.

danielhollas requested a review from csadorf June 27, 2022 16:13

csadorf added 3 commits June 28, 2022 12:52

Add note about the conda volume.

857e109

Add CLI test for extra volumes.

d55a0d5

Rename random_volume_name fixture to extra_volume_name.

b840d05

Since it is indeed not actually a random volume name, but the extra volume name for a specific class-scoped test instance.

csadorf approved these changes Jun 28, 2022

View reviewed changes

csadorf merged commit d017623 into aiidalab:main Jun 29, 2022

danielhollas deleted the custom-mounts branch June 29, 2022 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: extra mounts #132

Feature: extra mounts #132

danielhollas commented Jun 16, 2022

danielhollas Jun 16, 2022

csadorf Jun 17, 2022

danielhollas Jun 20, 2022 •

edited

Loading

csadorf Jun 21, 2022

danielhollas Jun 22, 2022

csadorf Jun 22, 2022

codecov bot commented Jun 16, 2022 •

edited

Loading

csadorf left a comment

csadorf Jun 17, 2022

danielhollas commented Jun 21, 2022 •

edited

Loading

csadorf commented Jun 21, 2022

csadorf commented Jun 22, 2022

danielhollas commented Jun 24, 2022

danielhollas Jun 24, 2022

csadorf Jun 24, 2022

danielhollas Jun 24, 2022

csadorf left a comment

csadorf Jun 24, 2022

danielhollas commented Jun 24, 2022 •

edited

Loading

csadorf left a comment

danielhollas commented Jun 27, 2022 •

edited

Loading

csadorf commented Jun 28, 2022

csadorf left a comment

danielhollas commented Jun 28, 2022

danielhollas commented Jun 29, 2022

csadorf commented Jun 29, 2022

danielhollas commented Jun 29, 2022

Feature: extra mounts #132

Feature: extra mounts #132

Conversation

danielhollas commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2022 • edited Loading

Codecov Report

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Jun 21, 2022 • edited Loading

csadorf commented Jun 21, 2022

csadorf commented Jun 22, 2022

danielhollas commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Jun 24, 2022 • edited Loading

csadorf left a comment

Choose a reason for hiding this comment

danielhollas commented Jun 27, 2022 • edited Loading

csadorf commented Jun 28, 2022

csadorf left a comment

Choose a reason for hiding this comment

danielhollas commented Jun 28, 2022

danielhollas commented Jun 29, 2022

csadorf commented Jun 29, 2022

danielhollas commented Jun 29, 2022

danielhollas Jun 20, 2022 •

edited

Loading

codecov bot commented Jun 16, 2022 •

edited

Loading

danielhollas commented Jun 21, 2022 •

edited

Loading

danielhollas commented Jun 24, 2022 •

edited

Loading

danielhollas commented Jun 27, 2022 •

edited

Loading