Add deploy control group #56
Conversation
…g thrown instead of printing error message Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
…g thrown instead of printing error message Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
…group # Conflicts: # CHANGELOG.md # scompose/version.py
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
scompose/project/project.py
Outdated
| # Validates params | ||
| self.instances[instance_name] = Instance( | ||
| name=instance_name, | ||
| config_name=name, |
There was a problem hiding this comment.
It seems redundant to carry around a name plug a "config name." Can we not just have the instance name include the replica number / why include both?
There was a problem hiding this comment.
The config_name property is used in a few special cases:
- Ensure we find the instance when sorting instance dependencies
- re-utilise same <instance.sif> for multiple replicas
If I hadn't config_name then I would have to keep striping out the réplica index from instance name... which could fail in case the instance name already ended with a number.
For instance, if compose yml had an instance "hello1" with replica=1 then that would become "hello11".. stripping numbers out of that would return "hello"
Apologies for the poor example/formatting, I'm my phone
If you have another approach in mind, let me know :)
There was a problem hiding this comment.
No worries I’m on my phone too now :) how about:
- we explicitly store the replica name
- The config_name becomes replica_name and is a property derived from combing name and replica, and falls back to name if replica is unset.
I think that would be more clear to describe what is going on let me know your thoughts.
There was a problem hiding this comment.
I agree with you. Will make the changes when I get home today. Thanks a lot Vanessa
There was a problem hiding this comment.
Hi @vsoch,
Just to let you know, I couldn't finish the implementation today...It turns out that creating (dynamically or statically) the replica_name property forces me to change a few other places for the logic to fully work.
I think I should be able to finish it tomorrow.
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
|
@vsoch this PR is ready for review. Let me know what you think about this implementation |
|
ping @vsoch Let me know if there is anything else missing on this PR :) |
| if not dep in sorted_instances: | ||
| sorted_instances.insert(index, dep) | ||
| for inst in instances.values(): | ||
| if dep == inst.name: |
There was a problem hiding this comment.
So here we are using just name and not get_replica_name because any of the replicas satisfies the condition (e.g., we insert them in the same block?
There was a problem hiding this comment.
And I remember correctly, the depends_on wouldn't include the replica number.
There was a problem hiding this comment.
You got the rationale right. We use the name (and not the get_replica_name) because when we write the dependencies in the compose.yml file, we don't include the replica number.
The existing sorting method will place all dependent replicas before the 'dependant' instance:
For instance:
hello ( replicas: 3) depends on hallo ( replicas: 1). After self._sort_instances(self.instances) it will be: hello1, hello2, hello3 (order doesn't matter for replicas) and then hallo1.
There was a problem hiding this comment.
Ooops, I wrote my example in the wrong way.. let me fix it before it becomes confusing
There was a problem hiding this comment.
This is the right exemple:
hallo ( replicas: 1) depends on hello ( replicas: 3) . After self._sort_instances(self.instances) it will be: hello1, hello2, hello3 (order doesn't matter for replicas) and then hallo1.
| ) | ||
|
|
||
| created.append(instance.name) | ||
| created.add(instance.name) |
There was a problem hiding this comment.
Same here - we aren't telling the user about replicas - just the parent name (just want to confirm this is intended).
There was a problem hiding this comment.
Same rationale of the previous comment.
2 participants
This PR adds support for instance replicas similar to what is available on docker-compose.
This feature is useful for workloads that require a fleet of singularity instances with the exact same config such as Queue Workers, Agents and so on.