Require goals to explicitly locate/choose an environment, and pin graph calculations to __local__
#17129
Comments
As a first step toward #17129, this change "masks" the `EnvironmentName` for relevant APIs in `graph.py` and `specs_rules.py`, and pins them to the `__local__` environment. The `@rule(.., _mask_types=[..])` argument triggers an assertion if the given type(s) are used in the identity of the `@rule` (i.e.: if the `@rule` "consumes" them from its scope). The impact of masking a type at an API boundary is an assertion that that type is not consumed by that API. As described in #17129: we're pinning graph computations to the `__local__` environment for now, although we may eventually allow graph computations to execute in a configurable environment. [ci skip-build-wheels]
As described in #17129: we would like `@goal_rule`s to eventually make their own decisions about which environments to use, generally by consuming targets to do so, but possibly also by pinning themselves to other environments via configuration. This change builds on #17179, and introduces a migration from `Goal.environment_migrated = {False => True}`. All graph-introspection goals consume only the APIs which are pinned to the local environment by #17179, and so are trivially migrated here. Other goals trigger a deprecation warning like the following: ``` DEPRECATED: Not setting `Goal.environment_migrated=True` for `Goal` `generate-lockfiles` is scheduled to be removed in version 2.17.0.dev0. See https://www.pantsbuild.org/v2.15/docs/plugin-upgrade-guide ``` Before calling #17129 done, we will migrate all internal goals to `Goal.environment_migrated=True`. [ci skip-rust] [ci skip-build-wheels]
|
Current status: Migrated:
Ready to migrate:
Yes/No questions:
Questions:
|
Yea, I think all of those should be pinned to local for now.
These should be consuming In order to avoid a huge flurry of added fields which we're not certain will be used yet, I think that for now these goals should partition their roots by
Ditto. |
|
@stuhood These should indeed be consuming environments off targets, there's just an ambiguity that I think we need to grapple with, which I think is worst for |
Yea, true. In that case, you could potentially add an option to the Eventually, I do think that there is a really, really neat feature possible behind the |
|
I suspect making it so that Pants generates one lockfile per resolve per environment would be extremely cool, in the long term, but yes, we have to make the choice. Re. |
A target cannot specify multiple |
|
This is a slight problem if e.g. different versions of Black are in separate environment, but for the moment, it sounds like it's not too concerning. |
|
@chrisjrn thanks for working on this! This is tough to reason about, and I bias towards doing the "safe" thing of pinning most goals to Also, it'd likely be good to split this up into many different PRs so that we can properly reason about each goal correctly. |
|
I agree on splitting up into many PRs. I suspect I'm going to do the obvious/ready goals first, and then follow up with the remaining categories. I think |
The main risk @stuhood and I identified with using |
|
One other consideration is that if enough goals are going to stay pinned to |
|
@stuhood I am in favour of migrating it to an enum for greppability: |
|
Yea, reasonable. |
|
Great, I'll add that in as part of the first PR. |
|
With #17247, we'll have I believe it should be possible to get everything except for
For 2.16, we should add a new lockfile schema that creates a separate lockfile per resolve per environment. |
For `count_loc`, `filedeps`, `peek`, `tailor`, `update-build-files`, the migration is trivial: no environment-specific processes need to be run. For `repl`, `run`, and `export`, a `rule_helper` is added that raises a warning if the user is asking to run against a target that specifies a non-local environment. This alerts the user to possibly unpredictable behavior, but for now, does _not_ inherently cause an explicit error. If we get to the point where we can run an `InteractiveProcess` in a foreign environment, we may wish to update this behaviour. Finally, this resolves a regression from a copy-pasta introduced in a previous PR. Oops. See #17129.
I think that that makes sense, yea? The proof will be in the pudding, but once we have "something" in place we can stabilize it after cutting the |
…ues working on actually addressing them (#17258) As requested by @Eric-Arellano. See #17129 for remaining migrations.
|
OK, now that I'm wading into how lockfiles are actually implemented -- effectively there's a "planning" phase where Currently my plan is make those planning rules execute in the It looks like there'll probably need to be a redesign of how lockfiles work in light of multiple environments, as the overall act of writing a lockfile depends on the underlying implementation (honestly, probably directories of files per resolve). |
|
I'm skeptical on the Related is #17276, which would allow you to give platform-awareness to resolves. |
|
@Eric-Arellano I think we're going to need to talk about this one in more detail then. |
That part is further down the road I think, and probably best discussed on a new issue. I think that @Eric-Arellano is right that So... that's cool. I think that that means that any sort of lockfile schema changes, or forking of building in multiple environments would only be necessary if we wanted to switch to |
|
What about JVM lockfiles? I'm not aware of how maven allows for JNI in packages, but that's likely to be relevant here. |
|
Depends on whether the resolver supports it by default. But because we have our own lockfile format for |
|
Re Python: The way I'm writing the environments support is to leave the onus of selecting the environments that belong to a Note that if, e.g. we allow interpreter constraints to be specified as part of an environment spec (possibly a useful addition at some point), then we can't guarantee that the resolves will be compatible across multiple environments. This may be a thing we need to factor into our thinking. |
|
Following #17300, #17313, and #17315, the only remaining goals are the |
This migrates `publish` to create the `BuiltPackage` asset in a correct environment, and run the `publish` executable in the local environment. Also adds a helper rule, `EnvironmentAwarePackageRequest`, which consolidates the `EnvironmentName` request and `BuiltPackage` request, since `BuiltPackage` is requested all over the place. See #17129.
`check` will now run each `CheckRequest` in each specified environment per the `field_sets` in the `CheckRequest`. It looks like the remainder of the `check` infrastructure already successfully handles dealing with multiple `CheckRequest`s. See #17129
|
Sure. |
…onment. (#17359) This adds a warning if a target-based `fmt`/`lint`/`fix` operation runs against a target that specifies a non-local environment. To property use environments, we'll need a way of making the partitioning rules a bit more pluggable, or decide that individual back-end implementations will be responsible for deciding where they run (which is probably also not great). Addresses #17129.
This is a least-effort attempt at getting `generate-lockfiles` into a position where individual backend implementations can select an environment in which to generate the lockfiles. The request generation phase runs in the default local environment, and is responsible for adding an `environments` field to the `GenerateLockfile` request object. If a request does not specify environments, the `GenerateLockfile` request is run in the default local environment. If a single environment is specified, that environment is to run the request. If more than one environment is specified, a warning is displayed, and the request runs in either the default local environment (if present in the request), or an arbitrary environment. This approach will give us an avenue in the future to run the request in _all_ the specified environments, and add behaviour to somehow merge the resulting lockfiles if different, since the eventual act of writing the lockfiles to disk happens in the rule goal code. Addresses #17129
|
Thanks @chrisjrn! |
Currently,
determine_bootstrap_environmentis called to compute a "default"EnvironmentNamebefore the rule graph is entered, such that all@goal_rules have one in scope automatically.While this was useful for getting things going and proving out our cross-compilation capabilities (demonstrated in #11148 and #13682), it is error prone. The environment design has been trending toward (eventually) giving all targets an
environment=field (even if the vast majority of the time it is only set via__defaults__). And in a world where ~all targets have anenvironment=field, ever accidentally using the default would definitely represent an issue.Instead, we'd like to put the onus on each
@goal_ruleto select the relevant environment (usually via targets). Additionally, to resolve the open question in Appendix A in the design doc, we will pin effectively all calculations ingraph.pyto the__local__environment (to eventually potentially be made configurable). This will mean that graph-introspection goals won't need to choose environments, and other@goal_rules won't need to select an environment while computing the graph.The text was updated successfully, but these errors were encountered: