Implement singularity-compose.yml override configuration feature #48

PauloMigAlmeida · 2021-10-05T21:14:27Z

There is one feature in docker-compose that I would love to have in singularity-compose that is the ability to specify multiple composer.yml files in which the properties would get either merged or overidden.

Example:

singularity-compose.yml

version: "2.0"
instances:

  cvat:
    image: cvat.sif
    start:
      options:
        - containall
    volumes:
      - ./recipes/cvat/env.sh:/.singularity.d/env/env.sh
      - ./volumes/cvat/data:/home/django/data
      - ./volumes/cvat/keys:/home/django/keys
      - ./volumes/cvat/logs:/home/django/logs
      - ./volumes/cvat/logs:/var/log/supervisor
      - ./volumes/cvat/tmp:/tmp
    network:
      enable: false

singularity-compose-dev.yml

version: "2.0"
instances:

  cvat:
    start:
      options:
        - containall
        - fakeroot

  cvatproxy:
    image: cvatproxy.sif
    start:
      options:
        - containall
    volumes:
      - ./volumes/cvatproxy/cache/nginx:/var/cache/nginx
      - ./volumes/cvatproxy/run:/var/run/
      - ./volumes/cvatproxy/log:/var/log/nginx
      - ./volumes/cvatproxy/lib:/var/lib/nginx
    network:
      enable: false
    depends_on:
      - cvat

By executing something like:

singularity-compose -f singularity-compose.yml -f singularity-compose.dev.yml <any_command>

we would get the runtime config combined into:

version: "2.0"
instances:

  cvat:
    image: cvat.sif
    start:
      options:
        - containall
        - fakeroot
    volumes:
      - ./recipes/cvat/env.sh:/.singularity.d/env/env.sh
      - ./volumes/cvat/data:/home/django/data
      - ./volumes/cvat/keys:/home/django/keys
      - ./volumes/cvat/logs:/home/django/logs
      - ./volumes/cvat/logs:/var/log/supervisor
      - ./volumes/cvat/tmp:/tmp
    network:
      enable: false

  cvatproxy:
    image: cvatproxy.sif
    start:
      options:
        - containall
    volumes:
      - ./volumes/cvatproxy/cache/nginx:/var/cache/nginx
      - ./volumes/cvatproxy/run:/var/run/
      - ./volumes/cvatproxy/log:/var/log/nginx
      - ./volumes/cvatproxy/lib:/var/lib/nginx
    network:
      enable: false
    depends_on:
      - cvat

which essentially overrode cvat->start->options and appended/merged cvatproxy to the runtime config.

I'm looking for feedback on the following points

whether or not you agree this is a feature that you want to see in singularity-compose?
I did a draft implementation, any thing that you want me to change or implement differently?

vsoch

Very cool idea! We could definitely support this. I've added some comments because we need to:

support previous functionality (e.g., the user providing no -f defaults to a single singularity-compose.yml as before)
the Python client in project.py also needs to work the same, still accepting a string (and you are extending to allow a list to merge).
The merge / load multiple yaml should not live in project.py - you can create a new module for it until utils if that's appropriate!

vsoch · 2021-10-05T21:20:05Z

scompose/project/project.py

@@ -64,8 +64,8 @@ def set_filename(self, filename):
        ==========
        filename: the singularity-compose.yml file to use
        """
-        self.filename = filename or "singularity-compose.yml"
-        self.working_dir = os.path.dirname(os.path.abspath(self.filename))
+        self.filename = filename


I would leave the default here to "singularity-compose.yml" - and if you are changing this to require a list, then please support the old behavior to still provide a single filename, and have it checked if it's a list, single string, or None, and process accordingly. And then if your preference is to store in a list, the variable should be self.filenames to indicate plural / multiple files.

And if I understand correctly, the working directory is changed to os.getcwd() to account for possibly different compose files in different directories. Of course the issue here is that all compose files need to then have their context relevant to that directory (is that your intention?)

I would leave the default here to "singularity-compose.yml"

The reason why I removed it is because this default value comes from the argparse as well, so I thought it was redundant. Quick question, the reason why you want the default value here as well is because you are envisioning devs instantiating this class programatically instead of using the CLI, is that correct?

yes correct! We wouldn't want to break previous scripts that are providing a string. The simple solution I think is to allow either.

vsoch · 2021-10-05T21:22:25Z

scompose/project/project.py

+            for f in self.filename:
+                # ensure file exists
+                if not os.path.exists(f):
+                    bot.error("%s does not exist." % f)


Please move the multiple load of yaml files outside of the project.py - there should ideally be a separate utils function or set of config functions (some config.py or similar) that takes the list. and returns the single config here. Also - self.filename implies one file, and there should either be a self.filenames or self.filename to match the arity.

vsoch · 2021-10-05T21:25:08Z

scompose/project/project.py

+    def deep_merge(self, yaml_files):
+        """merge singularity-compose.yml files into a single dict"""
+        if len(yaml_files) == 1:
+            # nothing to merge as the user specified a single file
+            return yaml_files[0]
+
+        base_yaml = None
+        for idx, item in enumerate(yaml_files):
+            if idx == 0:
+                base_yaml = item
+            else:
+                base_yaml = self.merge(base_yaml, item)
+
+        return base_yaml
+


All of this logic should not be in project.py - it's for merging yaml.

vsoch · 2021-10-05T21:25:39Z

scompose/project/project.py

+
+        return base_yaml
+
+    def merge(self, a, b):


Move this too.

vsoch · 2021-10-05T21:27:58Z

Also - if we provide this functionality I'd add an ability to preview a merge, basically taking the files, doing the merge, and printing yaml back to the screen.

PauloMigAlmeida · 2021-10-05T22:49:00Z

Also - if we provide this functionality I'd add an ability to preview a merge, basically taking the files, doing the merge, and printing yaml back to the screen.

Do you mean like a CLI option similar to what --read_only is ? Or printing it to console with (let's say logging verbosity set to debug)?

vsoch · 2021-10-05T23:31:49Z

Do you mean like a CLI option similar to what --read_only is ? Or printing it to console with (let's say logging verbosity set to debug)?

It could be that we have a separate command group for the config, and then something like a --preview? It doesn't have to match docker-compose exactly, although if docker compose already has that functionality we could. E.g.,:

# If we still use the up command group
$ singularity-compose up -d --preview

# A more general validation function (that could validate...
$ singularity-compose config --check

# And then support a preview
$ singularity-compose config --check

We can definitely split this into two different PRs - the reason I mentioned it is because not having the final file in a repository, for example, hinders reproducibility. So minimally I think we'd want to be able to output it to slightly better encourage saving it :)

PauloMigAlmeida · 2021-10-06T01:12:05Z

Hi @vsoch, apologies for asking one more question on that subject, I just want to make sure I understood what needs to be done.

Currently, if we run singularity-compose -f file1 -f file2 config this will already print the interpolated version (if multiple files are specified) or the original file (if just a single one is specified). The only caveat with that command is that it prints the content in JSON instead of YAML but given that its contents is interchangeable then that all good.

(that's the part which I'm not 100% sure if I got right, please correct me where I'm wrong):
If I understood you correctly, you want singularity-compose config --check to do pretty much the same thing but printing it in YAML format so that would slightly encourage devs to save the content.

If the above is not correct, I am not sure if I got the --check option's purpose 😬

vsoch · 2021-10-06T01:21:59Z

Yes sorry I'm not being clear!

(that's the part which I'm not 100% sure if I got right, please correct me where I'm wrong):
If I understood you correctly, you want singularity-compose config --check to do pretty much the same thing but printing it in YAML format so that would slightly encourage devs to save the content.

So my higher level idea in adding this check (akin to black --check for formatting) is to check the formatting / correctness of the files. To do this would be fairly simple - we'd add json schema (which I noticed we don't have here!) to do the check, and then the return code would be 0 all is good, 1 there was an error. So:

# Tell me if my config file(s) look okay (run against json schema)
$ singularity-compose config --check
$ echo $?
# 0 indicates everything looks okay

# Without --check we _could_ do a preview, but for one config this wouldn't make sense, so I'd suggest `--preview`
$ singularity-compose config --preview

which would then print the consolidated config to the screen. Perhaps for one config it would show default options that aren't explicitly stated.

To be clear, adding a command group is a big thing, and I'd be happy to help with this! If you want to focus on working on the merge functionality (which you've already done most of, just need to move it around to a different spot) I'd be happy to extend that PR this weekend to get the config command group to preview results, and it would be logical to add tests.

The only caveat with that command is that it prints the content in JSON instead of YAML but given that its contents is interchangeable then that all good.

We can easily yaml.dump the json to see the yaml instead! The user might want to dump (and save) the new compiled yaml file.

PauloMigAlmeida · 2021-10-06T03:10:04Z

thanks for the clarification, I got it now :)

Sure, feel free to commit stuff to this PR. For now, I will focus on adding the tests and docs related to this feature before taking on any of the other stuff.

vsoch · 2021-10-06T03:11:37Z

Sounds good! And I can definitely help too - I've done a bit of work with jsonschema so it's pretty quick for me to do.

PauloMigAlmeida · 2021-10-06T03:15:10Z

Your help is always welcome 😃

PauloMigAlmeida · 2021-10-06T04:32:36Z

I moved the logic out of project, added tests and docs. I will mark this PR as 'ready to review' just in case you want to expand those functionalities in a different PR.

this will add jsonschema as an optional dependency to check the validity of a singularity-compose.yml file. We will eventually extend this to check (and preview) combined files. Signed-off-by: vsoch <vsoch@users.noreply.github.com>

vsoch · 2021-10-06T04:44:50Z

Here is what I had in mind for check! #49

So if we add a --preview to that given two combined files (as you've done here) we can print the final thing to the console.

vsoch · 2021-10-06T04:45:36Z

Just saw your last message - I can pull down your changes into my branch and make a combined PR - not tonight but probably another night / this weekend (unless you beat me to it)

PauloMigAlmeida · 2021-10-06T04:53:00Z

Nice! I liked your implementation for check, I can implement the the --preview tomorrow =)

vsoch · 2021-10-06T04:54:13Z

And I am aware of the comedic value of failing my own lint check! I think it's probably time to remove the version constraint for black, and assume that most people will create a new venv to have it installed freshly. 😆

PauloMigAlmeida · 2021-10-06T05:00:35Z

A bit off-topic (in relation to this PR) but something that it would be nice for us to start thinking before writing any code... another thing I would love to have on singularity-compose is to be able to create volumes and reference them by name rather than path.

Usually docker create these persistent volumes under $HOME/.docker and I was thinking about using a path along the same train of thought.

I cannot count how many times I've (accidentally) deleted my 'persistent' paths before as I'm always tempted to have those mapped folders within my project directory structure 😬 There are other cases in which this is relevant (such as file system capabilities) but the key point is, if we could start thinking about this now, we can get a nice implementation shortly 😄

PauloMigAlmeida · 2021-10-06T05:02:57Z

Moved that to an issue to make sure the 2 subjects won't get mixed (I should've thought about that before 😓 )

…ide_files # Conflicts: # docs/commands.md

PauloMigAlmeida · 2021-10-06T22:26:00Z

@vsoch

I pulled your check feature PR (and tweaked it so it could take into consideration multiple files. I also implemented implemented the --preview argument. Let me know if there is anything missing :)

vsoch

Beautiful documentation, and tests too! Am I in OSS heaven?

I added a few changes, and once those are in I'll test locally!

docs/commands.md

scompose/project/project.py

scompose/utils/__init__.py

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>

PauloMigAlmeida · 2021-10-07T00:32:16Z

Done, feel free to pull it locally and run your tests :)

vsoch

A few more comments after local testing!

See updates to printing logic in the main function of check,py - basically removing extra output so it's a little cleaner and we only get what we ask for w.r.t the check command.
for the tests folder, please rename config_override to config_merge to match what we are doing.

And that should do it!

docs/commands.md

scompose/client/check.py

docs/commands.md

PauloMigAlmeida · 2021-10-07T02:04:59Z

Done, I also moved the tests belonging to this feature to a new file since we moved it out of the utils module

scompose/client/check.py

scompose/config/__init__.py

Signed-off-by: vsoch <vsoch@users.noreply.github.com>

and moving validate.py into its own file so we do not always import Signed-off-by: vsoch <vsoch@users.noreply.github.com>

Signed-off-by: vsoch <vsoch@users.noreply.github.com>

vsoch · 2021-10-07T02:59:37Z

This is such a great feature - thank you @PauloMigAlmeida !

PauloMigAlmeida added 2 commits October 5, 2021 20:45

implement deep_merge functionally

5f7bb40

fix typo

8c7a592

vsoch reviewed Oct 5, 2021

View reviewed changes

implement some changes suggested during code review

5491961

PauloMigAlmeida added 3 commits October 6, 2021 03:54

add tests for the config override feature

2285aba

good old black formatting that I keep forgetting :)

e2fbef0

add docs

7deee7d

PauloMigAlmeida marked this pull request as ready for review October 6, 2021 04:32

adding support for singularity-compose check

746fb07

this will add jsonschema as an optional dependency to check the validity of a singularity-compose.yml file. We will eventually extend this to check (and preview) combined files. Signed-off-by: vsoch <vsoch@users.noreply.github.com>

PauloMigAlmeida mentioned this pull request Oct 6, 2021

Singularity-compose named volumes #50

Open

PauloMigAlmeida added 5 commits October 6, 2021 20:13

implement deep_merge functionally

41731fa

fix typo

e7e6694

implement some changes suggested during code review

e1f5bf6

add tests for the config override feature

14996ab

good old black formatting that I keep forgetting :)

0fa8477

PauloMigAlmeida added 5 commits October 6, 2021 20:17

add docs

35388e9

Merge remote-tracking branch 'origin/ft/override_files' into ft/overr…

7a35b99

…ide_files # Conflicts: # docs/commands.md

tweak check group so it takes into account multiple files instead

6f9544f

implement preview option

01a522a

update changelog and add docs

2d10fe3

vsoch reviewed Oct 6, 2021

View reviewed changes

PauloMigAlmeida and others added 2 commits October 7, 2021 13:18

Apply suggestions from code review

a40e8f4

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>

move deep_merge stuff to a brand new module

900d8ee

PauloMigAlmeida added 2 commits October 7, 2021 00:32

black formatting

949b088

remove extra bullet point

86c0905

vsoch reviewed Oct 7, 2021

View reviewed changes

docs/commands.md Outdated Show resolved Hide resolved

scompose/client/check.py Outdated Show resolved Hide resolved

docs/commands.md Outdated Show resolved Hide resolved

implement code review changes

acea351

vsoch reviewed Oct 7, 2021

View reviewed changes

scompose/client/check.py Outdated Show resolved Hide resolved

PauloMigAlmeida added 2 commits October 7, 2021 02:11

implement code review changes

22dd789

bitten by the black formatting (again)

37f9e7e

vsoch reviewed Oct 7, 2021

View reviewed changes

scompose/config/__init__.py Show resolved Hide resolved

vsoch added 3 commits October 6, 2021 20:40

move config functions into proper module

2d75868

Signed-off-by: vsoch <vsoch@users.noreply.github.com>

trying to pin black to uptodate version

779456a

and moving validate.py into its own file so we do not always import Signed-off-by: vsoch <vsoch@users.noreply.github.com>

use bot.exit over print and sys.exit (which combines the two)

352ee85

Signed-off-by: vsoch <vsoch@users.noreply.github.com>

vsoch self-requested a review October 7, 2021 02:57

vsoch approved these changes Oct 7, 2021

View reviewed changes

vsoch merged commit 55db827 into singularityhub:master Oct 7, 2021

PauloMigAlmeida mentioned this pull request Feb 28, 2022

Fix check command #55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement singularity-compose.yml override configuration feature #48

Implement singularity-compose.yml override configuration feature #48

PauloMigAlmeida commented Oct 5, 2021

vsoch left a comment

vsoch Oct 5, 2021

PauloMigAlmeida Oct 5, 2021

vsoch Oct 5, 2021

vsoch Oct 5, 2021

vsoch Oct 5, 2021

vsoch Oct 5, 2021

vsoch commented Oct 5, 2021

PauloMigAlmeida commented Oct 5, 2021 •

edited

Loading

vsoch commented Oct 5, 2021

PauloMigAlmeida commented Oct 6, 2021 •

edited

Loading

vsoch commented Oct 6, 2021 •

edited

Loading

PauloMigAlmeida commented Oct 6, 2021 •

edited

Loading

vsoch commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

vsoch commented Oct 6, 2021

vsoch commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

vsoch commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

vsoch left a comment

PauloMigAlmeida commented Oct 7, 2021

vsoch left a comment

PauloMigAlmeida commented Oct 7, 2021

vsoch commented Oct 7, 2021

Implement singularity-compose.yml override configuration feature #48

Implement singularity-compose.yml override configuration feature #48

Conversation

PauloMigAlmeida commented Oct 5, 2021

vsoch left a comment

Choose a reason for hiding this comment

vsoch Oct 5, 2021

Choose a reason for hiding this comment

PauloMigAlmeida Oct 5, 2021

Choose a reason for hiding this comment

vsoch Oct 5, 2021

Choose a reason for hiding this comment

vsoch Oct 5, 2021

Choose a reason for hiding this comment

vsoch Oct 5, 2021

Choose a reason for hiding this comment

vsoch Oct 5, 2021

Choose a reason for hiding this comment

vsoch commented Oct 5, 2021

PauloMigAlmeida commented Oct 5, 2021 • edited Loading

vsoch commented Oct 5, 2021

PauloMigAlmeida commented Oct 6, 2021 • edited Loading

vsoch commented Oct 6, 2021 • edited Loading

PauloMigAlmeida commented Oct 6, 2021 • edited Loading

vsoch commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

vsoch commented Oct 6, 2021

vsoch commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

vsoch commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

PauloMigAlmeida commented Oct 6, 2021

vsoch left a comment

Choose a reason for hiding this comment

PauloMigAlmeida commented Oct 7, 2021

vsoch left a comment

Choose a reason for hiding this comment

PauloMigAlmeida commented Oct 7, 2021

vsoch commented Oct 7, 2021

PauloMigAlmeida commented Oct 5, 2021 •

edited

Loading

PauloMigAlmeida commented Oct 6, 2021 •

edited

Loading

vsoch commented Oct 6, 2021 •

edited

Loading

PauloMigAlmeida commented Oct 6, 2021 •

edited

Loading