| *** nimish has quit IRC | 02:18 | |
| *** nimish has joined #buildstream | 02:21 | |
| *** nimish has quit IRC | 02:27 | |
| *** nimish has joined #buildstream | 02:31 | |
| *** nimish has quit IRC | 06:43 | |
| *** alatiera has joined #buildstream | 06:43 | |
| gitlab-br-bot | juergbi opened (was WIP) MR !1183 (juerg/directory-import->master: Directory.import_files() improvements) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1183 | 07:08 |
|---|---|---|
| *** mohan43u has quit IRC | 07:17 | |
| *** mohan43u has joined #buildstream | 07:17 | |
| *** nimish has joined #buildstream | 08:04 | |
| *** tristan has joined #buildstream | 09:19 | |
| *** raoul_ has joined #buildstream | 09:48 | |
| *** jonathanmaw has joined #buildstream | 10:02 | |
| *** kapil___ has joined #buildstream | 10:02 | |
| *** lachlan has joined #buildstream | 10:32 | |
| *** ChanServ sets mode: +o tristan | 11:02 | |
| tristan | jonathanmaw, last night you asked an interesting question about _update_state() and https://gitlab.com/BuildStream/buildstream/merge_requests/1070#note_145007411 | 11:03 |
| tristan | Let me try to put it into different words to be more clear: Arbitrary code changes without consideration of maintainability are not really ever okay | 11:04 |
| tristan | Whether it is to fix a bug in functionality, or to optimize a codepath, it doesnt really matter | 11:04 |
| tristan | There is always a way to express a more optimized (or more functionally correct) code base intelligibly | 11:05 |
| tristan | jonathanmaw, for the specific case at hand, this is actually probably the single most complex part of BuildStream | 11:06 |
| tristan | Now, there are multiple ways to make it maintainable and not unbearable, currently _update_state() is obviously slated for refactor anyway | 11:07 |
| tristan | As I mentioned in the issue, juergbi and I differed initially on the approach - perhaps the event based approach would be better suited to doing less work less often, and still be able to handle state transition more or less centrally | 11:09 |
| tristan | jonathanmaw, That said, I think that in the mid-term, the forced state updates on reverse dependencies when a cache key is discovered is intelligible and safe if it works well | 11:10 |
| tristan | not an ultimate solution, but still not immensely complex | 11:10 |
| jonathanmaw | tristan: ok. I'll see how the state updates on reverse dependencies affects performance, and see where I can take it from there. Was there any logged discussion about your event-based approach? | 11:16 |
| *** lachlan has quit IRC | 11:22 | |
| *** lachlan has joined #buildstream | 11:24 | |
| tristan | Only on IRC afaik | 11:25 |
| tristan | jonathanmaw, it was quite a while back, but I suppose we could track it down do weeks before the appearance of _update_state() :) | 11:26 |
| tristan | jonathanmaw, just to be clear, my comment was not meant to block the branch - but rather as a warning that we should be especially careful to not make optimizations arbitrarily especially around _update_state() | 11:27 |
| jonathanmaw | tristan: okie doke | 11:27 |
| tristan | jonathanmaw, For more ideas... what this revolves around is reducing the amount of work which needs to happen to the minimal amount of times, while keeping cognitive complexity to a minimum | 11:28 |
| tristan | So | 11:28 |
| tristan | I was thinking that if we have some events which cause things to change, we should have strong assertions of invariants in place | 11:29 |
| tristan | Such that the logical transitions from one state to another always make sense | 11:29 |
| tristan | Another approach is to involve implicit invokation mechanisms | 11:30 |
| tristan | I'm not particularly a fan of implicit invocation because it can make things hard to debug when something goes wrong | 11:30 |
| tristan | jonathanmaw, I.e. what I mean by implicit invocation is that... When an event occurs on an Element, not only does the Element react to that by adjusting internal state, but state changes can fire notifications | 11:31 |
| tristan | Other elements, like reverse dependencies, might have interests registered to those notifications | 11:31 |
| tristan | Again, an approach which is straight forward and readable... EXCEPT when it comes to observing lines of code and considering what side effects it will cause and what code paths will run in which order | 11:32 |
| tristan | Anecdotal: There was a time when Glade had a signal called "update-ui", this was horribly inefficient for similar reasons, solution being to remove that and only ever do fine grained updates to any onscreen widgets | 11:34 |
| tristan | As a side note, *if* we refactored to use threads instead of forks; we could also eliminate any external callers informing the data model that something changed | 11:37 |
| tristan | I.e. the BuildQueue would never need to tell the element that "it's been assembled" | 11:37 |
| *** lachlan has quit IRC | 11:39 | |
| *** lachlan has joined #buildstream | 11:43 | |
| benschubert | tristan: "As a side note, *if* we refactored to use threads instead of forks; we could also eliminate any external callers informing the data model that something changed" -> not sure what you mean about that, but my guess is, it would be blocked by the GIL | 11:43 |
| benschubert | tristan: also, apprently, tracking only __cache_key changes is not enough, we need both __cache_key (or __strict_cache_key) and __weak_cache_key. Why bother with the 3 then? Couldn't we use only weak and strict for example? | 11:44 |
| tristan | The f**king GIL | 11:54 |
| tristan | We have to get around that | 11:54 |
| tristan | benschubert, What I mean by that is that the datamodel already knows when something is assembled or fetched, it's the one which exposes the function that does the work | 11:55 |
| tristan | benschubert, You have to ask juergbi more about why we have so many copies of the keys | 11:56 |
| tristan | I think that __cache_key is the active/chosen key and could possibly be replaced by a function, but I'm unsure at this point | 11:56 |
| tristan | Also I'm not sure why we distinguish between strong/strict key; I *think* it has to do with distinguishing the strong key which was downloaded in a pulled artifact | 11:57 |
| tristan | But I *suspect* that this could equally be implemented as a mutation of the "strong key" once an artifact is pulled, rather than having two separate variables | 11:58 |
| tristan | I can be wrong about that | 11:58 |
| juergbi | yes, the separate strict key is needed for attempting to pull the strict artifact from the remote CAS | 11:59 |
| juergbi | we do this even if we already have a locally cached artifact matching the weak key | 11:59 |
| juergbi | that locally cached artifact may have a different strong key, though | 11:59 |
| juergbi | I'm not really happy with this complexity either | 11:59 |
| tristan | I suppose we need it in order to influence the strong keys of reverse dependencies, when an artifact is pulled by its weak key | 12:00 |
| juergbi | yes. and we don't want to set the strong cache key field to the strict cache key just for the pull attempt | 12:01 |
| tristan | I'm not immensely bothered by the complexity in recognition that it serves a good purpose, but would probably prefer to have it implemented with more lines of code in a separate module (Artifact object abstraction candidate ?) | 12:01 |
| juergbi | otherwise reverse dependencies will already use that, and if it's not in the remote, it will be wrong | 12:01 |
| tristan | Yeah I get it, it's just really hard to remember :) | 12:01 |
| juergbi | the WIP Artifact class will likely help to some extent in this regard | 12:01 |
| benschubert | ah so we need __strong_key, strict_key and weak_key, and the update_state will pick the right one there | 12:02 |
| benschubert | ok, makes more sense now | 12:02 |
| benschubert | thanks! | 12:02 |
| gitlab-br-bot | BenjaminSchubert approved MR !1164 (danielsilverstone-ct/gc-play->master: _stream.py, _project.py: Manage GC during pipeline load) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1164 | 12:06 |
| benschubert | juergbi: and the strong/strict cache key of an element is dependent on the strong key of it's dependencies correct? not the strict one nor any other thing from its dependencies? | 12:11 |
| juergbi | benschubert: correct, the strict cache key is mainly relevant for pulling in non-strict mode | 12:12 |
| juergbi | in strict mode, strict and strong cache keys are always the same | 12:12 |
| benschubert | thanks! | 12:18 |
| tpollard | juergbi: do you have any ideas of what I could call a new exception class for the artifact abstraction class? We already have ArtifactError and ArtifactElementError | 12:24 |
| *** raoul_ is now known as raoul | 12:27 | |
| raoul | Maybe it's worth renaming ArtifactError to ArtifactCacheError and then using ArtifactError? | 12:29 |
| tpollard | I wouldn't be against that | 12:30 |
| tristan | raoul++ | 12:30 |
| tpollard | will roll with that then | 12:39 |
| jennis | Does ruamel.yaml always load dict values as strings, or is this something we're enforcing? | 12:40 |
| juergbi | raoul, tpollard: sounds fine to me, or possibly using ArtifactError for both? | 12:46 |
| juergbi | not sure whether we need to differentiate | 12:46 |
| juergbi | jennis: we do it in node_get() | 12:47 |
| *** raoul has quit IRC | 12:51 | |
| Kinnison | jennis: We enforce it | 12:58 |
| Kinnison | jennis: consider lines 32-34 of _yaml.py fr.ex. | 12:58 |
| jennis | ahh, looks like for {'a': 1}, ruamelyaml would load this as a string, pyyaml as an int | 12:58 |
| jennis | oh, because of those lines | 12:59 |
| jennis | thanks Kinnison | 12:59 |
| *** adds68 has joined #buildstream | 13:01 | |
| gitlab-br-bot | marge-bot123 merged MR !1164 (danielsilverstone-ct/gc-play->master: _stream.py, _project.py: Manage GC during pipeline load) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1164 | 13:18 |
| tristan | benschubert, What was the use case finally for making the coverage commands optional ? | 13:33 |
| tristan | benschubert, Ohhh you explicitly wanted to override the behavior by passing `tox -- --no-cov` | 13:33 |
| tristan | I remember now | 13:33 |
| benschubert | tristan: being able to run the code without coverage yup | 13:33 |
| tristan | I have a patch almost, which implements the separation, seems this person also had exactly the same use case https://github.com/tox-dev/tox/issues/899 | 13:34 |
| tristan | (a fixed bug, thought I was hitting it and then figured out how to use tox properly) | 13:35 |
| *** raoul has joined #buildstream | 13:37 | |
| tristan | Oh, I just realized I can do a simpler patch than teaching tox to do it, as the requirements for #916 are only for `python3 setup.py test` invocations (explicitly testing against installed dependencies) | 13:46 |
| gitlab-br-bot | Issue #916: loosen dev dependencies https://gitlab.com/BuildStream/buildstream/issues/916 | 13:46 |
| tristan | benschubert, The relevant parts of the resulting tox.ini looks like this: https://bpaste.net/show/e2db26cffe3a, and results in the ability to run `tox -e py35-nocover` (or any python env + -nocover) | 13:48 |
| tristan | benschubert, Do you think it's worth anything ? | 13:48 |
| tristan | Otherwise, I will just go for a simpler approach which simply removes coverage integration from the base setup.py layer | 13:49 |
| mablanch | Any change someone could have a look at this (remote-execution configuration): https://gitlab.com/BuildStream/buildstream/merge_requests/1186 | 13:49 |
| benschubert | tristan: just wondering if py* instead of {35,36,37} would work? it would make everything much nicer, otherwise it can be nicer indeed. I also think you don't need to protect lines 16-18 by "!nocover" not sure though :) | 13:52 |
| tristan | benschubert, I'm pretty sure it is required, from reading: https://tox.readthedocs.io/en/latest/config.html#complex-factor-conditions | 13:59 |
| tristan | i.e. positive factors (presence of py36) does not explicitly mean an absence another factor (indicated by !nocover) | 14:00 |
| tristan | it's a bit unintuitive to me but seems to work this way | 14:00 |
| tristan | And no, the wildcard doesnt work (just tried) | 14:02 |
| tristan | benschubert, But that doesnt answer the question really... I mean do we want these py*-nocover envs ? or are we happy with passing --no-cov ? | 14:03 |
| tristan | I'm pretty ambivalent now that I have a working patch, but it's technically not *really* needed | 14:04 |
| tristan | I'll throw up an MR first, I can split it in such a way that we can discard the last patch or not | 14:05 |
| benschubert | Yeah I'm not sure it's really needed and I'm happy with the --no-cov, let's see what others think | 14:18 |
| gitlab-br-bot | tristanvb opened MR !1189 (tristan/optional-coverage->master: Make coverage optional) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1189 | 14:22 |
| tristan | benschubert, I pinged cs-shadow on it as he has been in the loop for dependency pinning discussions too | 14:27 |
| benschubert | thanks | 14:27 |
| tristan | benschubert, ... can you take a look at #869, as laurence asked ? | 14:39 |
| gitlab-br-bot | Issue #869: Local cache size calculation is wrong https://gitlab.com/BuildStream/buildstream/issues/869 | 14:39 |
| * tristan is closing stale browser tabs | 14:39 | |
| tristan | benschubert, i.e. I believe !1106 fixed this | 14:39 |
| gitlab-br-bot | MR !1106: _artifactcache.py: Don't require the quota to be available on disk. https://gitlab.com/BuildStream/buildstream/merge_requests/1106 | 14:39 |
| benschubert | tristan: I still have dependencies to sort out before being able to do so :/ | 14:40 |
| benschubert | I'm not forgetting | 14:40 |
| tristan | heh, ok | 14:40 |
| tristan | Actually, #864 is arguably a duplicate of #660 | 14:43 |
| gitlab-br-bot | Issue #864: Local cache doesn't cleanup properly https://gitlab.com/BuildStream/buildstream/issues/864 | 14:43 |
| gitlab-br-bot | Issue #660: Stack traces when running out of disk space https://gitlab.com/BuildStream/buildstream/issues/660 | 14:43 |
| tristan | And now we start a cleanup job at startup if one is called for, so whatever part of #864 is not a duplicate, is also solved | 14:44 |
| tristan | benschubert, Would you agree with that ? And maybe close that one too ? | 14:44 |
| * tristan comments on issue | 14:44 | |
| benschubert | tristan: I'd argue that this one is likely not solved, since I'm pretty sure that still could be triggered :) But I'm happy to monitor our builds for a few days/weeks and close this if we don't trigger it anymore | 14:45 |
| tristan | benschubert, I am not saying we dont have crashes when we run out of physical disk space | 14:47 |
| tristan | benschubert, I am saying that that is what #660 is for | 14:47 |
| benschubert | oh right | 14:47 |
| benschubert | so basically saying that 864 is a duplicate and close it once the first part is handled. good to me! | 14:47 |
| tristan | benschubert, Right, so I believe that is the case, and have summarized it here: https://gitlab.com/BuildStream/buildstream/issues/864#note_145598353 | 14:51 |
| tristan | Would you care to just look over the comment and close 864 unless there is something you think I missed ? | 14:52 |
| * tristan is unsure if #737 is still relevant either | 14:55 | |
| benschubert | tristan: 737 seems good though :) | 14:58 |
| *** lachlan has quit IRC | 15:00 | |
| tristan | benschubert, "If the disk is full, buildstream should automatically cleanup, similar to what what happen when you set a quota" Doesnt that happen though ? | 15:01 |
| tristan | Lemme give it a quick look see again | 15:01 |
| tristan | Otherwise we are back in the territory of creating plenty of issues just because #660 is not yet fixed | 15:01 |
| *** lachlan has joined #buildstream | 15:02 | |
| tristan | Yes verified, raoul's 5e10e2e81 changed things around a bit | 15:06 |
| tristan | But ArtifactCache.full() still reports true when the remaining disk space is less than the headroom (now via CasQuota) | 15:07 |
| tristan | jjardon, Please check my last comment on #737 and contest or close :) | 15:14 |
| gitlab-br-bot | Issue #737: Buildstream local cache doesn't get cleanup automatically when the disk is full https://gitlab.com/BuildStream/buildstream/issues/737 | 15:14 |
| *** tristan has quit IRC | 15:18 | |
| *** tristan has joined #buildstream | 15:49 | |
| *** ChanServ sets mode: +o tristan | 15:49 | |
| lachlan | jonathanmaw: A priority merge request has been made in benchmarking (https://gitlab.com/BuildStream/benchmarks/merge_requests/30). Do you think you will have some time to review please? | 16:05 |
| jonathanmaw | lachlan: reviewed | 16:16 |
| lachlan | jonathanmaw: Thanks | 16:25 |
| aevri | Anyone know if we meant to support workspaces+remote builds? Am getting 'SandboxRemote does not support callbacks in command batches' - will open an issue if that's not expected, I can improve the error at least :) | 16:25 |
| juergbi | aevri: that's known to be broken | 16:26 |
| juergbi | however, a better error message or fallback to local build would definitely be better until this can be fixed up completely | 16:26 |
| juergbi | (incremental workspace builds via remote execution will require a solution for CAS missing timestamps) | 16:27 |
| aevri | aha yes | 16:27 |
| aevri | thanks, I might see about a better message for now; do you know of a super-issue that's tracking this? Perhaps I should open one explicitly for workspace+remote? | 16:28 |
| juergbi | I'm not aware of one. would be good, though | 16:29 |
| jmac | How do I test bst-external locally? I can't see any instructions in the readme. | 16:47 |
| jmac | I guessed at './setup.py test' as per the CI, but it throws lots of errors with current BuildStream | 17:02 |
| gitlab-br-bot | aevri opened issue #932 (Time spent in integration is only partially captured in logging) on buildstream https://gitlab.com/BuildStream/buildstream/issues/932 | 17:31 |
| gitlab-br-bot | aevri opened issue #933 (Support workspaces with remote execution (not incremental)) on buildstream https://gitlab.com/BuildStream/buildstream/issues/933 | 17:36 |
| tristan | benschubert, I just made a comment but scratch that, I think I overlooked something | 17:51 |
| gitlab-br-bot | aevri opened issue #934 (Support incremental builds with workspaces + remote execution) on buildstream https://gitlab.com/BuildStream/buildstream/issues/934 | 17:52 |
| benschubert | tristan: ok! | 17:54 |
| tristan | benschubert, updated comment | 17:57 |
| tristan | (or rather commented again) | 17:57 |
| benschubert | thanks! | 17:57 |
| tristan | benschubert, I think what threw me off is that the new recursive routine was not private; what I want to avoid is having external callers need to understand the internals, and have to choose what to do | 17:58 |
| benschubert | Oh right. What about having both private and have the element handle all the state itself? | 17:59 |
| *** alatiera has quit IRC | 18:00 | |
| tristan | I'm not sure it's currently feasible with what we do at load time until we do split it up | 18:00 |
| tristan | Or that it makes perfect sense yet | 18:00 |
| benschubert | It think doing it at initialisation would work | 18:01 |
| tristan | benschubert, i.e. if it means that we add some kind of function that currently exists in stream.py or pipeline.py as a part of the already complex loading process, I'm not sure we win anything by it | 18:01 |
| tristan | I am sure that doing _update_state() in Element.__init__() will cause things to break horribly, though | 18:02 |
| tristan | benschubert, Before state is ever actualized, the instantiated elements need to be told what the session plans to do | 18:02 |
| tristan | like which elements will be tracked, etc | 18:02 |
| *** jonathanmaw has quit IRC | 18:03 | |
| tristan | The order of things going on in Stream._load(), is *very* significant :) | 18:04 |
| *** nimish has quit IRC | 18:08 | |
| gitlab-br-bot | aevri opened MR !1191 (aevri/macos->master: _platform/darwin: fix missing space, say MacOS) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1191 | 18:10 |
| *** phildawson has quit IRC | 18:10 | |
| benschubert | Ah right | 18:17 |
| *** lachlan has quit IRC | 18:22 | |
| *** raoul has quit IRC | 18:27 | |
| *** tpollard has quit IRC | 18:33 | |
| *** tpollard has joined #buildstream | 18:33 | |
| *** tpollard has quit IRC | 18:40 | |
| *** tpollard has joined #buildstream | 18:40 | |
| *** tpollard has quit IRC | 18:42 | |
| *** tpollard has joined #buildstream | 18:42 | |
| gitlab-br-bot | marge-bot123 merged MR !1191 (aevri/macos->master: _platform/darwin: fix missing space, say MacOS) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1191 | 18:50 |
| *** tpollard has quit IRC | 18:52 | |
| *** tpollard has joined #buildstream | 18:53 | |
| *** lachlan has joined #buildstream | 19:00 | |
| *** mablanch has quit IRC | 19:13 | |
| *** valentind has quit IRC | 19:13 | |
| *** WSalmon has quit IRC | 19:13 | |
| *** laurence has quit IRC | 19:13 | |
| *** johnward has quit IRC | 19:13 | |
| *** bethw has quit IRC | 19:13 | |
| *** bethw has joined #buildstream | 19:14 | |
| *** johnward has joined #buildstream | 19:15 | |
| *** laurence has joined #buildstream | 19:15 | |
| *** mablanch has joined #buildstream | 19:15 | |
| *** valentind has joined #buildstream | 19:16 | |
| *** WSalmon has joined #buildstream | 19:17 | |
| *** adds68 has quit IRC | 19:17 | |
| *** jennis has quit IRC | 19:17 | |
| *** ikerperez has quit IRC | 19:17 | |
| *** mablanch has quit IRC | 19:33 | |
| *** bethw has quit IRC | 19:34 | |
| *** bethw has joined #buildstream | 19:34 | |
| *** mablanch has joined #buildstream | 19:34 | |
| *** benbrown has quit IRC | 19:36 | |
| *** jmac has quit IRC | 19:40 | |
| gitlab-br-bot | juergbi merged MR !1183 (juerg/directory-import->master: Directory.import_files() improvements) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1183 | 20:21 |
| *** benbrown has joined #buildstream | 20:42 | |
| *** benschubert has quit IRC | 20:55 | |
| *** lachlan has quit IRC | 21:04 | |
| *** adds68 has joined #buildstream | 21:31 | |
| *** tristan has quit IRC | 21:51 | |
| *** alatiera has joined #buildstream | 22:52 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!