[feat] Allow dependencies between partitions

[ekouts]

Closes #1378 .

[pep8speaks]

Hello @ekouts, Thank you for updating!

• In the file tutorials/deps/osu_benchmarks.py:

Line 119:80: E501 line too long (119 > 79 characters)

Do see the ReFrame Coding Style Guide

Comment last updated at 2020-11-04 09:45:17 UTC

[codecov-commenter]

Codecov Report

Merging #1481 into master will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1481      +/-   ##
==========================================
- Coverage   91.65%   91.56%   -0.09%     
==========================================
  Files          83       83              
  Lines       13012    13034      +22     
==========================================
+ Hits        11926    11935       +9     
- Misses       1086     1099      +13     

Impacted Files	Coverage Δ
reframe/core/pipeline.py	92.81% <100.00%> (+0.01%)	⬆️
reframe/frontend/dependency.py	97.18% <100.00%> (+0.40%)	⬆️
unittests/resources/checks_unlisted/deps_simple.py	100.00% <100.00%> (ø)
unittests/test_dependencies.py	98.51% <100.00%> (+0.02%)	⬆️
reframe/utility/os_ext.py	86.38% <0.00%> (-5.54%)	⬇️
reframe/frontend/executors/policies.py	98.22% <0.00%> (-0.02%)	⬇️

---

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 3c8f324...0e981fd. Read the comment docs.

[victorusu]

First of all, I want to say that I love this feature! I have been dreaming about this one for quite some time now... 😄

I am a bit lost with the naming logics of DEPEND_BY_PARTITION.
I think we should change the name of the DEPEND_BY_PARTITION.
But I have to say in advance that I have no good naming scheme to propose. 😅 🤦
Let me try to reason why I think the name DEPEND_BY_PARTITION should be changed.

Basic idea

Given the tests A and B, the systems s0 and s1, the partitions p0 and p1 and the environments e0 and e1. Using the notation A->B to denote that test B depends on test A, then in order to achieve the following dependency relations one should:

1. `A(s0, p0, e0) -> B(s1, p0, e0)` => Error! Inter-system dependencies not supported by ReFrame. Because it either run on `s0` or on `s1`.
2. `A(s0, p0, e0) -> B(s0, p0, e0)` => use `how=DEPEND_BY_ENV`. This is the default behaviour.
3. `A(s0, p0, e0) -> B(s0, p0, e1)` => use `how=DEPEND_FULLY` or `how=DEPEND_EXACT`
4. `A(s0, p0, e0) -> B(s0, p1, e0)` => use `how=DEPEND_BY_PARTITION` (this PR)

DEPEND_BY_ENV meaning

In the context of the ReFrame check, where one writes

B.depends_on('A', how=DEPEND_...)

The DEPEND_BY_ENV means "when the tests have the SAME environment".
One can read

B.depends_on('A', how= DEPEND_BY_ENV)

as

The test B depends on test A when their environments are the same, given that the systems and the partitions of A and B are the same.

I think the original mindset we had was the sentence:

Test B depends on A [constrained] by the environment.

Using the notation above
A(s0, p0, e0) -> B(s0, p0, e0)

DEPEND_BY_PARTITION meaning

In the context of the ReFrame check, where one writes

B.depends_on('A', how=DEPEND_...)

The DEPEND_BY_PARTITION means "when the tests have the DIFFERENT partitions"
One can read

B.depends_on('A', how= DEPEND_BY_PARTITION)

as

The test B depends on test A when their partitions are different, given that the systems and the environment of A and B are the same.

The DEPEND_BY_PARTITION uses the original mindset:

Test B depends on A constrained by the partition.
But in this case, the partitions are different.

Using the notation above
A(s0, p0, e0) -> B(s0, p1, e0)

DEPEND_EXACT meaning

In the context of the ReFrame check, where one writes

B.depends_on('A', how=DEPEND_...)

The DEPEND_EXACT means "when the tests have exact dependencies"
One can read

B.depends_on('A', how=DEPEND_EXACT, subdeps={'e0': ['e1']})

as

The test B with environment e0 depends on test A with the environment e1, given that the systems and the partitions of A and B are the same.

In the original mindset:

Test B depends on A constrained by the exact environments.

Using the notation above
A(s0, p0, e1) -> B(s0, p0, e0)

Conclusions

The original notation for DEPEND_ does not imply similar environments, rather similar systems and partitions.
In DEPEND_EXACT one can have different environments, but similar partitions.
DEPEND_BY_ENV implies similar environments and partitions.
And the proposed DEPEND_BY_PARTITION implies similar environments but different partitions.

So, I think we should change the name from DEPEND_BY_PARTITION.

A non-optimal proposal is to rename it to DEPEND_DIFF_PARTITION.
I know, it is not the best name change 😭 , but it does not maintain the same name structure that implies different logic.

[victorusu]

I am requesting a name change for DEPEND_BY_PARTITION, but I am not sure what should be the replacement. We should come up with some logic.

[victorusu]

@ekouts and @vkarak, I think we miss an example in the tutorial about this feature.
I am not sure if the tutorial should also come in this PR, or if it should be a different issue.
What do you think?

[victorusu]

Should we also add a test for DEPEND_BY_PARTITION inside def test_build_deps_unknown_test(loader, exec_ctx):?

[ekouts]

The original notation for DEPEND_ does not imply similar environments, rather similar systems and partitions.

@victorusu For me it makes sense as it is now, but of course I am open to change to whatever is more useful. As far as I understand the meaning of how implies similarity. Now DEPEND_BY_STH means that everything is the same up to STH. So DEPEND_BY_ENV means partition and env is the same. So the dependency for B.depends_on('A', how= DEPEND_BY_ENV) is:

A(s0, p0, e0) -> B(s0, p0, e0)

A(s0, p0, e1) -> B(s0, p0, e1)

A(s0, p1, e0) -> B(s0, p1, e0)

A(s0, p1, e1) -> B(s0, p1, e1)

Similarly DEPEND_BY_PARTITION means a full dependency inside the same partition. This is the old DEPEND_FULLY. Now B.depends_on('A', how= DEPEND_BY_PARTITION) would be:

# partition p0
A(s0, p0, e0) -> B(s0, p0, e0)
A(s0, p0, e1) -> B(s0, p0, e0)
A(s0, p0, e0) -> B(s0, p0, e1)
A(s0, p0, e1) -> B(s0, p0, e1)

# partition p1
A(s0, p1, e0) -> B(s0, p1, e0)
A(s0, p1, e1) -> B(s0, p1, e0)
A(s0, p1, e0) -> B(s0, p1, e1)
A(s0, p1, e1) -> B(s0, p1, e1)

The new B.depends_on('A', how= DEPEND_FULLY) means:

# all to all dependencies
A(s0, p0, e0) -> B(s0, p0, e0)
A(s0, p0, e1) -> B(s0, p0, e0)
A(s0, p1, e0) -> B(s0, p0, e0)
A(s0, p1, e1) -> B(s0, p0, e0)
A(s0, p0, e0) -> B(s0, p0, e1)
A(s0, p0, e1) -> B(s0, p0, e1)
A(s0, p1, e0) -> B(s0, p0, e1)
A(s0, p1, e1) -> B(s0, p0, e1)
A(s0, p0, e0) -> B(s0, p1, e0)
A(s0, p0, e1) -> B(s0, p1, e0)
A(s0, p1, e0) -> B(s0, p1, e0)
A(s0, p1, e1) -> B(s0, p1, e0)
A(s0, p0, e0) -> B(s0, p1, e1)
A(s0, p0, e1) -> B(s0, p1, e1)
A(s0, p1, e0) -> B(s0, p1, e1)
A(s0, p1, e1) -> B(s0, p1, e1)

[victorusu]

@ekouts, Ah!
I had a different mindset when reviewing this PR!
You are right. In this case, DEPEND_BY_PARTITION is the appropriate name!

[victorusu]

Since the naming scheme got clarified... 😂
lgtm

[vkarak]

The meaning of "by" in the DEPEND_BY_XXX is to specify how the test cases of the two tests are connected. It defines a binding or grouping relation based on the XXX attribute, until now only the environment. That means that DEPEND_BY_ENV means that the test cases of two tests depend on each other only if they refer to the same environment: you can't have cross-environment dependencies. If we extend that logic to partitions we have 2^2 combinations to cover instead of just two (BY_ENV and FULLY). If we stick to the definition of BY as a binding factor, then we can derive the following meaning.

Assume that tests t0 and t1 are valid for {p0, p1} partitions and for {e0, e1} programming environments. Then we have the following cases:

DEPEND_FULLY

That's the fully connected graph with 16 edges, which I skip from posting it here.

DEPEND_BY_ENV

That's essentially two graphs, one per each environment, fully connected per partition:

(t0, p0, e0) -> (t1, p0, e0)
(t0, p0, e0) -> (t1, p1, e0)
(t0, p1, e0) -> (t1, p0, e0)
(t0, p1, e0) -> (t1, p1, e0)

(t0, p0, e1) -> (t1, p0, e1)
(t0, p0, e1) -> (t1, p1, e1)
(t0, p1, e1) -> (t1, p0, e1)
(t0, p1, e1) -> (t1, p1, e1)

DEPEND_BY_PARTITION

The exact analogous of DEPEND_BY_ENV, but the binding is on partition:

(t0, p0, e0) -> (t1, p0, e0)
(t0, p0, e0) -> (t1, p0, e1)
(t0, p0, e1) -> (t1, p0, e0)
(t0, p0, e1) -> (t1, p0, e1)

(t0, p1, e0) -> (t1, p1, e0)
(t0, p1, e0) -> (t1, p1, e1)
(t0, p1, e1) -> (t1, p1, e0)
(t0, p1, e1) -> (t1, p1, e1)

DEPEND_BY_ENV and DEPEND_BY_PARTITION

This can be represented as a bitwise OR in the how parameter: DEPEND_BY_ENV | DEPEND_BY_PARTITION. This will bind on both environment and partition (I don't see a better way to represent this programmatically other than a bitwise OR if we don't want to introduce yet another constant):

(t0, p0, e0) -> (t1, p0, e0)
(t0, p0, e1) -> (t1, p0, e1)
(t0, p1, e0) -> (t1, p1, e0)
(t0, p1, e1) -> (t1, p1, e1)

DEPEND_EXACT

Exactly as before. The user can create the exact relationships by herself.

I think the definitions I just presented are consistent and cover all the possibilities. By reading the comments here, I think that the current PR does not follow exactly this principle am I right?

[ekouts]

I think the definitions I just presented are consistent and cover all the possibilities. By reading the comments here, I think that the current PR does not follow exactly this principle am I right?

@vkarak You are right, the DEPEND_BY_PARTITION was supposed to replace the old DEPEND_FULLY. I will change the code and the unittests with the new flags.

[vkarak]

We discussed this a bit further with @victorusu and came up with a more general and flexible solution. To avoid the confusion that the DEPEND_BY variables mean, we could have the following syntax:

self.depends_on('T1', when=always)
self.depends_on('T1', when=part_equal)
self.depends_on('T1', when=env_equal)

Then when argument is a callable that takes the source and destination nodes of the test case graph and returns true if there is an edge or false otherwise. This is much more flexible compared to the DEPEND_EXACT for supporting specific patterns, e.g., partition is not named x or partitions are not equal etc. You could even compose a function from others.

These predefined functions are as simple as follows:

def always(src, dst):
    return True

def part_equal(src, dst):
    p0, _ = src
    p1, _  = dst
    return p0 == p1

def env_equal(src, dst):
    _, e0 = src
    _, e1  = dst
    return e0 == e1

You could even define a function that would create dependencies only if the target partition has a specific name:

def target_part_is(name):
    def _has_edge(src, dst):
        return dst[0] == name

    return _has_edge

And then

self.depends_on('T1', when=target_part_is('foo'))

I also believe that this approach will simplify a lot the building of the dependency DAG.

I suggest moving this PR to ReFrame 3.3, since there is too little time to make it for this release.

[vkarak]

Check my last comment in the main conversation.

[vkarak]

I converted this to a draft.

[vkarak]

@ekouts Another thing we will need is to update the tutorial example of dependencies. Since we now support dependencies across partitions, we could have a test that only downloads the OSU benchmarks running on the login partitions.

[vkarak]

Just minor comments. Regarding the documentation, I will go over it and I will fix myself anything wrong there.

[vkarak]

I've updated the documentation. The only thing remaining is to update the tutorial example.

[vkarak]

The updated tutorial example looks ok. We need to update the text and pay attention to the builtin environment that you have added. You may need to update the text of the main tutorial, too.

[vkarak]

All good! Ready to go!

[vkarak]

Oh, still some PEP8 issues, I can fix them.

[victorusu]

lgtm too! Excellent PR. With lots of discussions! Love it!

[vkarak]

For the failing tutorial check, see issue #1573.