Fix dirtying of uncacheable nodes #17079
Conversation
…pantsbuild#13199 behavior: now we stop dirtying at `!restartable` nodes instead. [ci skip-build-wheels]
[ci skip-build-wheels]
stuhood
force-pushed
the
stuhood/uncacheable-dirtying
branch
from
dea2a17 to
52bf5a0
Compare
|
While this bug has existed a while, it was only actually triggered for the first time by the |
|
I was toying mentally with the idea of removing the uncacheability from the Result type, with the motivation being that users would be able to deduce which tools actually ran and which were cached. This makes it seem that if we removed the uncacheability we'd be looking again at the "bug". If that's true, then A) woah thats tricky, and B) I think #17060 should also be considered a bugfix for this issue (even though I changed the motivation for the change) |
That would not be true, no. Cacheable nodes have ~always been dirtied correctly: the bug was only that uncacheable nodes were not dirtied as well. |
|
See https://github.com/pantsbuild/pants/tree/main/src/rust/engine/graph#invalidation for more information on the expected semantics. |
|
Ah ok that makes sense. The fact that it is uncacheable, ironically made it cached. And if it wasn't uncacheable it wouldn't have been cached. |
| @@ -43,13 +43,18 @@ pub trait Node: Clone + Debug + Display + Eq + Hash + Send + 'static { | |||
| /// If a node's output is cacheable based solely on properties of the node, and not the output, | |||
| /// return true. | |||
| /// | |||
| /// Nodes which are not cacheable will be recomputed once (at least, in case of dirtying) per | |||
There was a problem hiding this comment.
| /// Nodes which are not cacheable will be recomputed once (at least, in case of dirtying) per | |
| /// Nodes which are not cacheable will be recomputed once (or, in case of dirtying, possibly more than once) per |
There was a problem hiding this comment.
Mm, yea: that would be clearer: will follow up. Thanks!
As reported in pantsbuild#17060, the same `@rule` running within a single "session"/"run" of Pants was not correctly re-running due to changed inputs. The `@rule` is uncacheable (via its output type being `EngineAwareReturnType.cacheable = False`), for the purposes of a logging side-effect. The `@rule` was failing to re-run due to a bug in the dirtying of uncacheable rules that had likely existed since pantsbuild#13199 split `Node::restartable` out from `Node::cacheable`. When the `restartable` property was introduced, it took over controlling whether a Node could be interrupted while running. But the `Entry::dirty` codepath was still incorrectly preventing `uncacheable` `Node`s from being dirtied. [ci skip-build-wheels]
As reported in #17060, the same `@rule` running within a single "session"/"run" of Pants was not correctly re-running due to changed inputs. The `@rule` is uncacheable (via its output type being `EngineAwareReturnType.cacheable = False`), for the purposes of a logging side-effect. The `@rule` was failing to re-run due to a bug in the dirtying of uncacheable rules that had likely existed since #13199 split `Node::restartable` out from `Node::cacheable`. When the `restartable` property was introduced, it took over controlling whether a Node could be interrupted while running. But the `Entry::dirty` codepath was still incorrectly preventing `uncacheable` `Node`s from being dirtied. Fixes #17377.
As reported in #17060, the same
@rulerunning within a single "session"/"run" of Pants was not correctly re-running due to changed inputs. The@ruleis uncacheable (via its output type beingEngineAwareReturnType.cacheable = False), for the purposes of a logging side-effect.The
@rulewas failing to re-run due to a bug in the dirtying of uncacheable rules that had likely existed since #13199 splitNode::restartableout fromNode::cacheable. When therestartableproperty was introduced, it took over controlling whether a Node could be interrupted while running. But theEntry::dirtycodepath was still incorrectly preventinguncacheableNodes from being dirtied.[ci skip-build-wheels]