Improved depends_on

[pierlauro]

Hi,
I eliminated the arbitrary limit of 100 re-try at containers' start time. It was used to detect a probable circular dependency in the use of depends_on.

In this PR:

• Each time the configuration file is parsed, instances are now sorted in a DAG fashion so that - at instance start time - it is possible to determine if there is a circular dependency.
• singularity-compose down is shutting down the services in reverse order based on depends_on: if A is needed by B (B depends on A), it means that B will be shutted down before A.

[pierlauro]

I have a doubt on get_instance_names: to me it seems that self.instances is always instantiated, maybe it is worth removing the configuration elif branch?

If the PR looks good I will also update the documentation accordingly.

[vsoch]

I have a doubt on get_instance_names: to me it seems that self.instances is always instantiated, maybe it is worth removing the configuration elif branch?

Oh yes, this is spot on! I realized this during my review and it looks like you picked up on it too. +1.

[pierlauro]

I just tested manually the over-mentioned features (sorting based on depends_on, circular dependencies and eliminate outer loop with do_create).

I could easily test the DAG sorting function and I am gonna do that.

Regarding the other parts - instead - right now there is no easy way to mock components (e.g. mock containers creation/execution without using singularity for real) and setting that up seems a lengthy task detached from this pull request. ATM I would rely on the manual checks I did, what are your feelings about that?

[vsoch]

Well, if you look at the circle setup and tests that are currently run, the hard part to have singularity is already handled. From this starting point, wouldn't it be easy to add another config file to the tests folder, and bring up a small busybox group that would each (as their startscript) write their own name to a file, then we could read the file and confirm that the order is correct?

[vsoch]

If you are exhausted I could take a shot as well - and I'd open with your commits in another PR so the testing could run (as opposed to PR to your branch which wouldn't run).

I'll be back at computer in maybe 4-5 hours (still 6am here!) and would be happy to help :)

[pierlauro]

Well, if you look at the circle setup and tests that are currently run, the hard part to have singularity is already handled. From this starting point, wouldn't it be easy to add another config file to the tests folder, and bring up a small busybox group that would each (as their startscript) write their own name to a file, then we could read the file and confirm that the order is correct?

Oh, perfect! Let me look into it during the weekend as I am working on something else ATM :)

[vsoch]

Please see #32 hopefully I got us most of the way there!

[pierlauro]

Please see #32 hopefully I got us most of the way there!

I've merged your PR into this one since I had to test the changes. I've created a failing test (with a circular dependency) and a successful one (also checking correctness of the "DAG list")

[vsoch]

@pierlauro it's good practice to respect work by others - you've taken my work and added via commits under your name. Please either pull those commits, or work from my branch, I won't be merging something that does not correctly give attribution to the correct author.

[vsoch]

I've merged your PR in this one since I had to test the changes.

I do not see my commits represented here.

[pierlauro]

My bad, as long as the software is enhanced I usually don't care about attribution but I understand it can be different for other people.
I had copied your files as github's web interface doesn't allow to open a PR from the original repo (your depends_on branch) to a forked one (my depends_on branch) but just the opposite. As you raised the issue, how would you recommend to proceed? (Just in case, I've anyway saved the last changed in the branch depends_on_bak of my repo).

[pierlauro]

My bad, as long as the software is enhanced I usually don't care about attribution but I understand it can be different for other people.
I had copied your files as github's web interface doesn't allow to open a PR from the original repo (your depends_on branch) to a forked one (my depends_on branch) but just the opposite. As you raised the issue, how would you recommend to proceed? (Just in case, I've anyway saved the last changed in the branch depends_on_bak of my repo).

Ok, I merged through the CLI. Give me a couple of minutes to replicate my changes

[vsoch]

we should be good to go! The CI (CircleCI) wasn't triggered to run on a pull request, so I updated the API docs and enabled building forks, and after that passes we should be good to go.

[vsoch]

okay, so the Singularity install is working ok (and I've updated to use more recent versions) what it looks like is that the runner on CircleCI is expecting a Singularity recipe file (and not honoring Singularity.first, etc.). I'm not sure why, especially because the test_client.py works (which does a similar build of https://github.com/singularityhub/singularity-compose-examples/tree/master/apache-simple, and there isn't a Singularity file there.

[pierlauro]

okay, so the Singularity install is working ok (and I've updated to use more recent versions) what it looks like is that the runner on CircleCI is expecting a Singularity recipe file (and not honoring Singularity.first, etc.). I'm not sure why, especially because the test_client.py works (which does a similar build of https://github.com/singularityhub/singularity-compose-examples/tree/master/apache-simple, and there isn't a Singularity file there.

The build step requires sudo privileges, could it be a problem of permissions?

[vsoch]

I think it could be - that's the only difference that I can produce locally. Before we dig into that, I just want to do some print outs in the CI for a sanity check (it does work for me locally). Here is a local run:

Copying Singularity.second to /tmp/pytest-of-vanessa/pytest-16/test_circular_dependency0/Singularity.second
Copying singularity-compose.yml to /tmp/pytest-of-vanessa/pytest-16/test_circular_dependency0/singularity-compose.yml
Copying Singularity.first to /tmp/pytest-of-vanessa/pytest-16/test_circular_dependency0/Singularity.first
Creating project...
DEBUGGING PARAMS
Singularity.first
{'build': {'context': '.', 'recipe': 'Singularity.first'}, 'depends_on': ['second']}
PRESENT WORKING DIRECTORY
/tmp/pytest-of-vanessa/pytest-16/test_circular_dependency0
['Singularity.third', 'Singularity.second', 'singularity-compose.yml', 'Singularity.first']
DEBUGGING PARAMS
Singularity.second
{'build': {'context': '.', 'recipe': 'Singularity.second'}, 'depends_on': ['second']}
PRESENT WORKING DIRECTORY
/tmp/pytest-of-vanessa/pytest-16/test_circular_dependency0
['Singularity.third', 'Singularity.second', 'singularity-compose.yml', 'Singularity.first']
DEBUGGING PARAMS
Singularity.third
{'build': {'context': '.', 'recipe': 'Singularity.third'}}
PRESENT WORKING DIRECTORY
/tmp/pytest-of-vanessa/pytest-16/test_circular_dependency0
['Singularity.third', 'Singularity.second', 'singularity-compose.yml', 'Singularity.first']

and I'll post the CI run here when I get it. What we could try (not sure if it would be supported) is --fakeroot on the CI. What isn't clear to me is why the self.recipe (which should be Singularity.first, second, third, etc. is just being parsed as "Singularity"). Another possibility is if instances / tests are being run in parallel, it could be that messes something up.

Oh wait, I think I just reproduced locally - it's because of the command to pytest (and perhaps where the $PWD is?) Let me try again - I think it might be because pytest is running things at the same time, and it's messing it up.

[pierlauro]

I think it might be because pytest is running things at the same time, and it's messing it up.

Good guess! If tests are run in parallel, both the depends_on ones are sharing the same container names and messing things up

[vsoch]

Woot, that did the trick! The giveaway was seeing unexpected output for another test mixed with the tests we wrote. And then on top of this issue, the circle config was (for some reason) no longer valid, so I was working on it locally:

circleci config validate .circleci/config.yml 

And updated the usage for the singularity orb, and also enforced running the tests one at a time by way of calling pytest more than once. And whew, all is working as expected! I also simplified the config quite a bit - I always feel clever at points when I do some wacky, advanced yaml nonsense, and then years later, usually regret it (and like I did now, realize that the simplest structure is going to work the best!)

@pierlauro such great work you did here, and now we are tested and green! It's time for merge! I'll post the package release here shortly after. Thank you so kindly for working on this!

[vsoch]

https://pypi.org/project/singularity-compose/0.0.18/

[pierlauro]

Woot, that did the trick! The giveaway was seeing unexpected output for another test mixed with the tests we wrote. And then on top of this issue, the circle config was (for some reason) no longer valid, so I was working on it locally:

circleci config validate .circleci/config.yml 

And updated the usage for the singularity orb, and also enforced running the tests one at a time by way of calling pytest more than once. And whew, all is working as expected! I also simplified the config quite a bit - I always feel clever at points when I do some wacky, advanced yaml nonsense, and then years later, usually regret it (and like I did now, realize that the simplest structure is going to work the best!)

@pierlauro such great work you did here, and now we are tested and green! It's time for merge! I'll post the package release here shortly after. Thank you so kindly for working on this!

Great! Thank you very much for your time on that!