https://pantsbuild.org/ logo
Join Slack
Powered by
ohh I think we now end up running `./v2 fmt` multi...
# development
h
ohh I think we now end up running 
./v2 fmt
multiple times when files are changed?
to reproduce, intentionally mess up formatting such that Black will rewrite the file
a
Yeah, I’m pretty sure I raised that as something inherently sketchy about the console_rule rewriting files model back when we were working out how to do multiple formatters…
h
Oh I agree it’s sketchy. But I think it’s a crucial use case that we need to figure out through whatever gymnastics how to support. For example, the code to run each formatter sequentially is weird and more complex than V2 lint, but worth it
a
I guess we could do some kind of tracking in the 
Workspace
object to say “I’m materializing files for this 
@console_rule
, if you get a notification that the digest of this file has changed to this, don’t bother re-running invalidated stuff unless it’s needed for something other than this console_rule”? But that feels scary…
👍 1
I guess we could have console_rules return workspace modifications they should apply, and apply them outside of the graph?
w
it's possible that we're doing the wrong thing if it is invalidated while it is running: cc @early-needle-54791
h
it’s possible that we’re doing the wrong thing if it is invalidated while it is running
I think this is the issue! I noticed while running 
./v2 test --debug
that changing the file during the test caused it to run twice
e
Ok! Thats good info as well eric. Did you notice this before the fix to disable the engine fs watcher (my change) went out?
w
@hundreds-father-404: note that running twice might be expected regardless... it's just that the console rule should only "render its output" or "write files to disk" once.
so you might see double logging, for example, but should not see double console output.
@average-vr-56795: we discussed this here: https://github.com/pantsbuild/pants/pull/9015#discussion_r371281467 , and i kept the behaviour of not re-running uncacheable/sideeffecting nodes within a Session. i think that this is a bug around an invalidation arriving during the run of an Uncacheable node.
h
Yes, Henry, this was before the feature gate.
👍 1
e
I have a test where I can show that an uncacheable node will run twice if it is dirtied the while running the first time. The above discussion is saying that this is not okay even if the node isn’t completed the first time because it is invalidated while running, correct?
👍 1
I should mention it will run again with the same session_id
h
As a user, I would expect it to not run again, unless perhaps I was using 
./v2 --loop
. I don’t know if that’s wrong or right, and how much my expectation is from past experience with Pants, but that’s what I personally would expect
e
Okay, I’ll see If I can make a change so that is doesn’t run again
❤️ 1
w
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1585692632077100?thread_ts=1585375324.037900&cid=C0D7TNJHL Yea, that's correct. We're lumping together "side-effecting" with "uncacheable" right now. And side-effecting things cannot re-run.
If in the future we wanted to be more specific about those two classifications we could split them, but I don't anticipate that that will be necessary.
👍 1
e
How should I deal with the fact that there probably won’t be a result to set as the EntryResult for the uncacheable node if its dependecies were also invalidated?
I’m imagining that I will need to give the uncacheable node a result value so it isn’t re-run, even if it became dirty while running, is that the wrong way to handle this?
I could see also, re-running dependencies until we have a result.
w
not sure what you mean
uncacheable nodes already hold their value... they'll just only give it to the appropriate session
@early-needle-54791: mind if i take a look at the test, just to confirm?
e
forsure
want to pair for a bit?
w
in a call for 45
e
ah okay.
w
i understand what you're saying there. and yea.
Open in Slack
PreviousNext
Powered by