Target dependency rules

[kaos]

Copied from the design doc:

Background

Monorepos can by definition house very large code bases. In order to maintain good architectural hygiene it is important to adhere to intermodule interfaces and protocols to avoid chaos and escalating maintenance.

Use Case

By implementing clear boundaries for modules to interact only via public interfaces, the inner workings of the modules can be kept private, avoiding intricate and complicated dependency patterns.

The dependency rules instructions offers the ability in a declarative way to specify which modules may depend on a certain target.

Motivation

We want to support large projects promoting good systems design. The dependency rules feature will be a tool that helps with that.

Feature request: #13393

Core API PR: #17550
Visibility implementation PR: #17401

[kaos]

I've adjusted the syntax slightly, to accommodate a per target type style of specifying visibility rules, using the same style as for __defaults__.

[kaos]

And another round of syntax adjustments have been made. The rule specs are now a list of list (or tuples) where each spec's first item is the target types the rules apply to followed by all rules (spread out or as a single list). Target types may be specified with a glob pattern (as well as the rules).

Example BUILD file (from the PR description):

# Limit who may depend on targets in this part of the source tree
__dependents_rules__(
  # No one outside this directory may depend on the python_sources in this subtree.
  (python_sources, ".", "!*",),

  # Test dependents against this set of rules for all targets that don't have any type-specific rules until the first matching one.
  # Any path beginning with the ! is a deny rule, otherwise allow incoming dependency.
  ("*",
     "src/green-field/path",
     "src/*/cleared",
     "!src/danger/*/zone",
     "!tests/*",
     "*",  # allow by default if no other rule applied.
     # Other alternatives here could have been:
     "!*",  # deny, or
     "?*",  # warn
  ),
)

# Limit what may be depended upon from this part of the source tree
__dependencies_rules__(
  # Python tests, test utils and files targets may only depend on sources in this directory.
  (("python_test*", files), ".", "!*"),

  # Test dependencies against this set of rules for all targets that don't have any type-specific rules until the first matching one.
  # Any path beginning with the ! is a deny rule, otherwise allow outgoing dependency.
  ("*",
     "src/green-field/path",
     "src/*/cleared",
     "!src/danger/*/zone",
     "!tests/*",
     "*",  # there must normally be a catch all rule at the end, as any target that falls through is an error, unless extend is true, see below.
  ),

  extend=True,  # may be used to inherit any parent rules, in which case fall-through as declared here is OK, when there exists a catch-all rule in the parent rules.
)

It's not picky on the nesting of rule patterns, so use what makes most sense for your style. (this may help to preserve your formatting in face of black ;) )

__dependencies_rules__(
  (
    "*",  # rules for all targets
    (
      ("src/green-field/path", "src/*/cleared"),  # src/ allow rules
      ["!src/danger/*/zone"],  # src/ deny rules
      ["!tests/*"], # tests/ rules
      "*",  # catch all rule
    ),
  ),
)

[kaos]

also worth noting is that this applies to the pants.backend.experimental.visibility implementation, other implementations are free to use the *args, **kwargs to the dunder calls however they please.

[kaos]

The motivation for the list syntax rather than the previous dict based one is to get a defined stable and predictable order the rules are applied to a target.

It is only ever one single rule that will be applied, and that is the first rule that matches for any particular target.

[benjyw]

Thanks for taking this up! would love to see this happen, it's one of our most requested features.

One thing that I'd love to see in this doc, before proceeding to an implementation, is a list of real-world example use-cases, from multiple teams, that support the design.

I.e., ask users for 5-8 real-world scenarios and show in each case how this mechanism supports each scenario with minimal boilerplate.

The reason I think this is important is the same reason I've been reluctant to just go ahead and implement something here months ago: We need to keep in mind that the problem to be solved is "dependency hygiene enforcement", not "lack of visibility rules". "visibility rules" is a paradigm for solving "dependency hygiene enforcement", and it happens to be Bazel's paradigm, so it's the one people focus on.

I'm not yet convinced that the "visibility" paradigm solves the problem of "dependency hygiene" in all cases. So I wonder if we need a lower-level, highly customizable, mechanism, with a very easy way to plug in your own repo's logic, and with "visibility" as the default "logic plugin" we ship.

[kaos]

Sounds fair. I'm not well positioned to conduct such a survey, however I did whip up an implementation for it already (consider it a POC if you will #17401) that also shows that what it actually does is very pluggable, you can override every piece of the machinery besides the actual syntax used in the BUILD file (if you want to do that, you might just as well write your own thing any way).
Notably the experimental backend shows the moving pieces required to get the visibility checks going: https://github.com/pantsbuild/pants/blob/d3927684672c942e4a0897708c76abf05909b65b/src/python/pants/backend/experimental/visibility/register.py
And from that, it should be fairly straight forward to see how to customize the class being registered to do custom things, as well as how the actual check is invoked relying on the ValidateDependenciesRequest.

I could add more examples of the customizability if desired, apart from that I suppose the missing thing now is more data to show if this supports the needed use cases or not.

[cognifloyd]

I have 2 use cases for visibility in the StackStorm (st2) project.

1. Prevent circular imports:

   • st2common module may be a dependent of most other modules, but should not have other modules in its dependencies.
   • st2client module should not have dependencies on any of our other modules, especially st2common.
   • today I'm using regex-lint to do this, but there are false positives in strings and comments that prevent full coverage:
     https://github.com/st2sandbox/st2/blob/pants/lint-configs/regex-lint.yaml
   • visibility would allow me to restrict the dependencies of the st2client module and the st2common module.

2. Help inference resolve ambiguous imports:

   • There are many different tests modules in the monorepo, several of which have a tests/unit/base.py file. Because there are so many tests modules, pants cannot infer dependencies for tests.unit.base.
   • I added explicit dependencies on st2common/tests/unit/base.py (where st2common/ is a source root; the st2common module is defined in st2common/st2common/ and tests for it are in st2common/tests/) in relevant BUILD files under st2common/tests. I did something similar in the other source roots where pants had issues with the imports.
   • With pants 2.14's switch to [python-infer].unowned_dependency_behavior = "warning" (instead of "ignore"), there are a lot of warnings that pants cannot infer tests.unit.base (and similar), so I should add # pants: no-infer-dep to each of those imports. That is a very invasive solution for a dependency I had to explicitly add to BUILD files, so I'm leaning towards switching that back to "ignore" instead of adding the import comment to so many test files.
   • visibility would let me say that there can be NO dependents on the tests modules, except within itself. That should resolve the ambiguity and the inference issues.

[cognifloyd]

I'm imagining something like this for the fist use case (replace the regex-lint config):

st2common/st2common/BUILD:

__dependencies_rules__(
    all=(
        # st2common can only depend on itself, contrib (packs, runners, etc), and st2client.
        ".",
        "contrib/**",
        "st2client/st2client/**",
        # prohibit st2common from depending on any st2* libs except st2common and st2client.
        "!st2*/st2*/**",
    ),
    default="error",
)

__dependents_rules__(
    all=(
        # prohibit st2client from depending on st2common
        "!st2client/st2client/**"
    ),
    default="allow",
)

st2common/st2common/services/BUILD:

__dependencies_rules__(
    all=(
        # TODO: refactor inquiry.py to not import from st2actions
        "st2actions/st2actions/container/base.py",
    ),
    extend=True,
)

st2client/st2client/BUILD:

__dependencies_rules__(
    all=(
        ".",
        # st2client cannot depend on any other modules in this repo.
        "!**",
    ),
    default="error",
)

I can also imagine inverting the st2common rules, so that the other st2* modules have __dependents_rules__ that disallow st2common from depending on them, and st2actions/st2actions/container/BUILD would add an exception for st2common/st2common/services/inquiry.py.

[cognifloyd]

And I'm imagining something like this for the second use case (disambiguate the tests modules):

st2*/tests/BUILD, contrib/*/tests/BUILD, and contrib/runners/*/tests/BUILD:

__dependents_rules__(
    all=(
        # only files within this tests directory can import from its contents.
        ".",
        "!**",
    ),
    default="error",
)

or do something like this, though this would be more complex to maintain:

__dependencies_rules__(
    all=(
        # this tests directory cannot import from any other tests directory
        ".",
        "!st2*/tests/**",
        "!contrib/*/tests/**",  # packs
        "!contrib/runners/*/tests/**",  # runners
    ),
    default="allow",
)

[kaos]

Thank you Jacob for these detailed examples, they are really helpful.
Regarding the disambiguation support though, I looked at the dependency inference logic, but there's very little shared between target types, and they are all implemented a little bit differently so there's currently not a good way forward to implement the disambiguation with the dependency rules at this time. It'll be a feature on its own where we'd want to look at a way to generalize it across the various target types first, I think it's still a good idea, just out-of-scope for the initial version.

[cognifloyd]

So, it doesn't work across types, but what if I used python_source (in a dict) instead of all. Would that make it work?

Or will the dep inference code have to be changed to respect the dependency rules?

Or do you think that the dep inference layer will need knobs separate from the dependency rules?

[kaos]

Or will the dep inference code have to be changed to respect the dependency rules?

Something along these lines, I think. Dep inference need to invoke the dep rules at some point, or it will trigger post-fact and blow up (in case of violations where other alternatives may have existed).

[kaos]

I have an open question for the visibility implementation regarding how to treat relative paths (paths that begin with a period) when inherited in subdirectories.

Example:

# src/example/BUILD
__dependencies_rules__(
  # Python sources must only depend on targets in this directory.
  (python_sources, ".", "!*"),
)

Now, for targets in the src/example this makes perfect sense, but what about sources in src/example/a?
Currently, the rule stays the same, with no regard for where it was first declared, so the limitation stays the same, i.e. any Python sources in src/example/a may only depend on other targets in src/example/a.

On one hand, I think this makes perfect sense, but what if what I wanted was to just root my rule where it was defined, and any rules that inherit it also inherit the root where it was declared rather than where it gets inherited. Lets look at another example:

# src/example/BUILD
__dependencies_rules__(
  # Python sources must only depend on targets in this directory subtree.
  (python_sources, "./*", "!*"),
)

Now this translates to a rule that says that any Python sources may only depend on targets in src/example/*. When inherited in sub directory a it becomes src/example/a/* but in this case I may have had in mind that the entire subtree should be a "single unit", so that any where in the subtree my Python sources may depend on anything in src/example/*. If I spell it out (in the BUILD file using the path rather than .), that's what it will do, and may be clearer.

So this long winded discussion leads to my question: are relative paths treated correctly, wrongly or should it be optional which way to treat them?

(I've been so emerged in making all this work, so this nuance didn't really occur to me until now.. 😬 )

[benjyw]

Hmm, I commented on this on another PR, but now I'm not sure.

Yeah, I think saying that ./ means "the location of the file the rule is being applied to" and build_file_dir() means "the location of this BUILD file" is a good distinction.

[benjyw]

But we should not require build_file_dir(), build_file_dir() / "*" all over the place, would build_file_dir() / "**" / "*", say, not do the trick?

[benjyw]

And maybe we should have some syntactic sugar for build_file_dir()...

[kaos]

The double star is not a thing here. A single star is already recursive.
But perhaps we should treat src/foo/* as implicitly also accepting src/foo.

(I attempted to highlight that in the design doc.. )

[kaos]

And maybe we should have some syntactic sugar for build_file_dir()...

It is pretty clear though, and helps illuminate ppl of its existence when used a lot more than before.. :P

Other idea, using <here> as a substitute for build_file_dir(): "<here>/sub/*"..

[kaos]

Note to self to include in upcoming docs:

A neat trick to help have rules apply to a subset of 3rd party deps, is to split them out into a separate requirements.txt file in a dedicated directory.

Before:

# 3rdparty/BUILD
python_requirements()

After:

# 3rdparty/common/BUILD
python_requirements()

# 3rdparty/subset/BUILD
python_requirements()

With the above, you may have rules like:

__dependencies_rules__(
  (python_sources, "3rdparty/common", "!3rdparty/subset", ...),
)

But keeping the two requirements targets in the same resolve will still produce a single lockfile, awesome!
h/t to @Eric-Arellano for that.

[kaos]

The common stuff can remain in 3rdparty/ and doesn't have to move out to a sibling directory.. that was merely for the example.

[kaos]

This will no longer be required post #17620

[jriddy]

Can I make a UI consistency nit? We use the word dependees for the goal, so I think we should prefer __dependees_rules__ over __dependents_rules__. I think consistency across the API matters for making easier for users to interact with this.

Unless we want to change the goal and other references to dependees (in a backwards compatible way, of course 😉 ). "Dependents" does have the benefit of being an actual word in the dictionary, but this is a much bigger change.

[kaos]

Thanks @jriddy !
See #17395 #17397 #17399 (maybe there're more..) :)

[kaos]

We had a long thread of this over in Slack, and it boiled down to abandoning "dependee" in favour of "dependent"

[jriddy]

Oh sorry I missed that. I guess I'm fine with that decision, although you know me, I'll hope we keep the old naming as an alias 🙃

[kaos]

No worries, I'm sure the old name is going to be supported behind the scenes for a long time to come. (much thanks to your efforts Josh)

[benjyw]

Yeah the old name is deprecated but not slated for possible removal for another year or so of releases.

[kaos]

Syntax proposal for extended rule spec selection syntax.

When looking up which rules that apply for a given dependency link between two targets, we currently only use a set of target type globs followed by a list of rules.

Now, if we want to have different rules based on other properties of the target then it's type, we need some syntax for how to declare those properties.

With target types it looks like:

# A rule spec:
(
  # Target type selection globs
  ("target_a", "target_b", "wild_*"),
  rules...
)

So the overall structure for the "rule spec" is a n-tuple where the first element is the "selector" and the rest are simply rule globs.
The selector for target types is either a single string, or a tuple of strings to support multiple targets sharing the same set of rules.

Now we may either make the string syntax for the target type a little bit fancier to support selecting based on target filename or tag etc, or switch to a structured format that will be slightly more verbose but crystal clear.

I'm a little bit in the camp to go with a structured syntax, but with some sugar to support a more concise string representation of the more common uses.

Example:

# Rule spec:
(
  ( # Selectors
    {"type": "target_a", "path": "myfile.lib"},
    {"type": "resource*", "tag": "libs"},
  ),
  rules...,
)

Now I may be off, but I think that path and tag will cover a majority of use cases, so support for them in text form could look like:

# Rule spec:
(
  ( # Selectors
    "target_a[/myfile.lib]",  # specific target type for a specific file in this directory (i.e. same anchor rules as for rule globs)
    "resource*(libs)",  # resources tagged `libs`
    "file*(all-txt)[files/**/*.txt]",  # all file and files targets tagged `all-txt` for all .txt files
    "[res/*.png]",  # short cut for `*[res/*.png]` i.e. any target type for all png files in res/
  ),
  rules...,
)

[kaos]

Regarding 3rdparty deps, this is exactly the case I was working on, which led to this note: #17389 (comment)

[kaos]

I noticed when I wrote the tests for file based rules, that I was unable to sensibly exclude a certain file where it was defined. Depending on the nature of your rules, it works well to have the exclusion "on the other end", but then if you have many such cases it doesn't scale well.

[danxmoran]

In an example case:

module1/
    foo.py
    bar.py
module2/
    baz.py
    qux.py

Say I want to forbid module1/* from depending on module2/*, but foo.py already imports qux.py, and I don't want to break that import before applying the rule to prevent further dependencies.

Ideally (to me) I could write rules in module1/BUILD like:

__dependencies__rules(
    ("python_source*", "!//module2/*"), # "default" rule
    ("foo.py", "//module2/qux.py"),     # 1:1 exception
)

Without the ability to use file paths / target names on both the left- and right-hand sides of rules, I think(?) I could achieve the same effect with something like:

# module1/BUILD
__dependencies__rules(
    ("python_source*", "!//module2/*"),
    ("foo.py", "//module2/*"),
)

# module2/BUILD
__dependents_rules(
    ("python_source*", "!//module1/*"),
    ("qux.py", "//module1/*"),
)

Apart from the extra boilerplate, I think the UX in this case is worse because the definition of a single "conceptual" rule is spread across multiple files, and if you look at just one of the files you are likely to get the wrong impression.

[kaos]

I miss the point as you have file paths on both sides in both examples..

I am however +1 to support file paths on both sides. Although, with some additional syntax on the left side to disambiguate it from a target type.

[kaos]

This would work using my proposal above, but you'd have to put the exception before the general rule:

__dependencies__rules(
    ("[foo.py]", "//module2/qux.py", "*"),     # 1:1 exception
    ("python_source*", "!//module2/*", "*"), # "default" rule
)

Edit: in case of many rules, the repetition may be reduced by using variables.. and this also makes the exception rule required rather than just sticking a "*" in there that would've served just as well..

py_src_rules = ("!//module2/*", "*")
__dependencies__rules(
    ("[foo.py]", "//module2/qux.py", py_src_rules),     # 1:1 exception
    ("python_source*", py_src_rules), # "default" rule
)