Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register synthetic targets for subsystems. #17653

Closed
wants to merge 2 commits into from

Conversation

kaos
Copy link
Member

@kaos kaos commented Nov 27, 2022

This is the first step towards having proper dependencies to subsystem configurations.

Planned next step is to add a dependency on synthesized configuration targets based on option value sources per subsystem.

Can eventually be used to address things like #14724

Example config source info as available already:
image

This adds quite a number of targets to the root, which can easily be filtered out however:

╰─❯ ./pants list //
//:buildgrid_remote
//:docker_env
//:subsystem-anonymous-telemetry
//:subsystem-auth
//:subsystem-auth-acquire
//:subsystem-auth-token-check
//:subsystem-auth-token-info
//:subsystem-autoflake
//:subsystem-black
...

╰─❯ ./pants list --filter-target-type=-_subsystem //
//:buildgrid_remote
//:docker_env
//BUILD_ROOT:files
//cargo:scripts
//conftest.py:test_utils
//pants:scripts
//pants.toml:files
╰─❯ 

@kaos kaos requested a review from stuhood December 1, 2022 02:04
@stuhood
Copy link
Member

stuhood commented Dec 12, 2022

Sorry that I haven't reviewed this yet: will check it out by EOD.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. How will the dependency rules be implemented?

For --changed-since, are you trying to get changes in the subsystem to result in changes to impacted targets to show up? How would we know a subsystem has changed? We don't have access to the prior values of the options

@kaos
Copy link
Member Author

kaos commented Dec 12, 2022

Interesting idea. How will the dependency rules be implemented?

After adding synthetic targets for config files, such as pants.toml and others,
my idea was to look at the config sources for each option value, and have the subsystem depend on each of those sources. Initially crude, I'm sure, but we may refine from that. (as an improvement over today where we have no dependency at all from a subsystem to its config sources)

Hmm, this gets me thinking, aren't the subsystem values part of the hash value, so if there is a config change that would already invalidate the graph depending on that subsystem?

For --changed-since, are you trying to get changes in the subsystem to result in changes to impacted targets to show up? How would we know a subsystem has changed? We don't have access to the prior values of the options

Similar here I think, rather crude that if you make any change in pants.toml for instance, then all subsystems that have any value coming from that file will be invalidated. Just to get started some where.

@stuhood
Copy link
Member

stuhood commented Dec 13, 2022

Hmm, this gets me thinking, aren't the subsystem values part of the hash value, so if there is a config change that would already invalidate the graph depending on that subsystem?

Only at runtime: that's the reasoning behind runtime graph awareness: #11206

@kaos
Copy link
Member Author

kaos commented Dec 13, 2022

Hmm, this gets me thinking, aren't the subsystem values part of the hash value, so if there is a config change that would already invalidate the graph depending on that subsystem?

Only at runtime: that's the reasoning behind runtime graph awareness: #11206

Oh.. ok. Let me see if I understand this correctly. So, this would mean that if we invalidate target x due to a change in pants.toml then if that change didn't affect subsystem y, we'll get the cached result from a previous run?

If so, I feel that it's less bad to have these coarse invalidations going, as it may report more targets as changed, but the actual work performed will be correct.

@Eric-Arellano
Copy link
Contributor

The effect of what you're describing is that adding a new blank line to pants.toml will invalidate almost every single target, since most targets have a relationship to subsystems. I'm skeptical we want that so coarse grained.

@kaos
Copy link
Member Author

kaos commented Dec 14, 2022

The effect of what you're describing is that adding a new blank line to pants.toml will invalidate almost every single target, since most targets have a relationship to subsystems. I'm skeptical we want that so coarse grained.

It would only invalidate for those subsystems you have configuration for in your pants.toml, and that is likely not every subsystem out there.

Also, more often than not, the change you make will be relevant to some part of the project, rather than simply being just a formatting change, so to me it would make sense to invalidate a lot in order to pick up the change properly rather than not at all. This is no worse than when you regenerate your lockfile and it gives you a pretty big blast radius.

@Eric-Arellano
Copy link
Contributor

It would only invalidate for those subsystems you have configuration for in your pants.toml, and that is likely not every subsystem out there.

How so? How will you know the subsystem is defined in pants.toml? Parse the TOML?

@kaos
Copy link
Member Author

kaos commented Dec 14, 2022

It would only invalidate for those subsystems you have configuration for in your pants.toml, and that is likely not every subsystem out there.

How so? How will you know the subsystem is defined in pants.toml? Parse the TOML?

It's in the PR description screen shot and text ;)

(and further discussed in my comment here #17653 (comment) )

@stuhood
Copy link
Member

stuhood commented Dec 14, 2022

Hmm, this gets me thinking, aren't the subsystem values part of the hash value, so if there is a config change that would already invalidate the graph depending on that subsystem?

Only at runtime: that's the reasoning behind runtime graph awareness: #11206

Oh.. ok. Let me see if I understand this correctly. So, this would mean that if we invalidate target x due to a change in pants.toml then if that change didn't affect subsystem y, we'll get the cached result from a previous run?

No: it would mean that we'd be able to precisely report the files that are consumed in order to run, say, test, without the need for synthetic targets to be generated for them. Targets would just represent metadata, rather than "the unit of dependency", per-se: the unit of dependency (at least for the purposes of --changed) would be a file instead.

we'll get the cached result from a previous run

Actual cache keys would be unaffected by #11206, same as they are not affected here: the cache key is still based on what @rules request at runtime, and which Process structs they construct. #11206 (and the synthetic targets work) are both about making --changed match what @rules actually request... but #11206 proposes to try and do it by introspecting what they do request, rather than by creating synthetic targets to correspond to each file that is consumed.

@kaos
Copy link
Member Author

kaos commented Jan 13, 2025

Closing as stale.

@kaos kaos closed this Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants