#rm buildkit effects crap

1 messages · Page 1 of 1 (latest)

zinc storm
#

@lost gate (whenever you time, I'm not blocked)

As part of removing the buildkit solver, the whole telemetry effects system is highly impacted obviously.

For now I'm trying to find the shortest path to just getting the solver rm'd without having to refactor the whole universe. So my idea atm is to retain the effects system most as is but just send the spans in a different place.

Mostly caught up on how it works but couple questions:

  1. I can't see where this DagInputsAttr is actually used in practice
    • We set this field to its value, but then my ide is telling me that field is never used otherwise. Can we just remove it entirely from telemetry? Would save me from having to reimplement it right away 🤞
  2. I'm not totally following why we need to set a Completed for the whole recursive deps tree here. I'm guessing it's some inane buildkit thing where it didn't always emit a span for every dep? I'm hoping there's a way I can avoid having to port this too
lost gate
#
  1. It's not currently used, but it could/should be - for incremental trace loading (handling 1.7GB of span data), that's what will let us find all the call inputs, and fetch the spans corresponding to them, otherwise you get those 'missing digest' placeholders for the receiver/arguments to a call
  2. Yeah you pretty much guessed it. Basically when you have a DAG like x -> (a, b) -> c, and c is a cache hit, we need to know that we can consider a and b completed, otherwise x will still seem "pending" (installed effects that never ran)
zinc storm
#

It's not currently used, but it could/should be - for incremental trace loading (handling 1.7GB of span data), that's what will let us find all the call inputs, and fetch the spans corresponding to them, otherwise you get those 'missing digest' placeholders for the receiver/arguments to a call
Cool yeah it's not that it would be impossible or even that difficult to re-add, just seeing what the mvp-est possible path is

#

thanks!

zinc storm
#

@lost gate got an updated version of this that works by tracking effects via call.IDs (other than still relying on the ⁨load cache:⁩ span from buildkit, which can't be rm'd until the whole solver is gone). Allows me to delete ⁨PBDefinitions()⁩ methods, which was the main goal since it was a remaining dependency on LLB 🎉

There's actually more stuff showing up with CACHED status than before, but it's all stuff that I actually think should be cached, so I think this simplification actually results in an improvement too?

There's just one last diff in the ⁨TestGolden⁩ suite: https://dagger.cloud/dagger/traces/05cb3959485aa3b7072a735fc1b3a261?span=6f1d0e7a511ae19a

However, from what I can tell the new output actually seems more accurate? The function that's called (⁨DockerBuildFail⁩) never errors itself (code), it's the dockerfile that gets unlazied later which errors out.

That makes logical sense to me, but want to check if you agree or if the previous appearance of the output was intentional

zinc storm
lost gate
#

interesting, will take a closer look next week / on the flight! looking forward to all these simplifications, the broader cache hits sounds great (cc @ebon siren who's been looking into that)

zinc storm
#

~~Had a semi-related realization that the amount of telemetry we send back to clients is O(n^2) in space, seems like potentially a very easy way to save a lot of bandwidth/memory/storage https://github.com/dagger/dagger/pull/11775~~

Nevermind, massive brain fart

lost gate
#

going back to when (still?) a somewhat common practice is for a test running function to return a ⁨Container⁩ that runs the tests, without having to explicitly sync it within the test function itself

#

i say 'maybe we don't care' because if it's ⁨Dockerfile⁩-specific behavior, meh

zinc storm
#

Dockerfiles are weird because there's more laziness involved right now I think

lost gate
#

⁨fail-effect⁩ should do it