| *** tristan has joined #buildstream | 03:28 | |
| *** ChanServ sets mode: +o tristan | 03:28 | |
| gitlab-br-bot | tristanvb opened issue #1397 (Incorrect `no-member` linter errors from cython modules) on buildstream https://gitlab.com/BuildStream/buildstream/-/issues/1397 | 04:30 |
|---|---|---|
| tristan | looking here: It *appears* that we do not support specifying a link as the key to a junction we want to override in a subproject: https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_loader/loader.py#L484 | 05:15 |
| tristan | But I've added a test for it, and it appears to pass :-S | 05:15 |
| tristan | Aha, ok so it does work part of the time | 05:19 |
| tristan | it wont work in a deeper context | 05:19 |
| *** tristan has quit IRC | 05:41 | |
| *** WSalmon has quit IRC | 05:42 | |
| *** tpreston has quit IRC | 05:42 | |
| *** WSalmon has joined #buildstream | 05:43 | |
| *** WSalmon has quit IRC | 05:48 | |
| *** WSalmon has joined #buildstream | 05:50 | |
| *** tristan has joined #buildstream | 07:59 | |
| *** ChanServ sets mode: +o tristan | 07:59 | |
| gitlab-br-bot | tristanvb merged MR !2026 (tristan/lazy-provenance->master: Refactor: Lazily instantiate ProvenanceInformation objects) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2026 | 08:01 |
| gitlab-br-bot | tristanvb opened issue #1398 (Junction `overrides` feature does not resolve links) on buildstream https://gitlab.com/BuildStream/buildstream/-/issues/1398 | 08:12 |
| gitlab-br-bot | tristanvb opened MR !2078 (tristan/fix-overriding-links->master: Fix junction overrides to resolve links) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2078 | 08:13 |
| tristan | juergbi, benschubert ... any thoughts on what to do for https://gitlab.com/BuildStream/buildstream/-/issues/1398 would be appreciated | 08:23 |
| tristan | I added a "proposed fix" there but I'm not sure it's great | 08:23 |
| juergbi | will take a look in a bit | 08:23 |
| * tristan also added (failing) tests to !2078, but no code towards fixing it | 08:23 | |
| benschubert | tristan: I'll look into it too | 08:27 |
| benschubert | tristan: also concerning !2070, what would you think of a test that validates the format of the logging output for each files (main log, and then elements logs) so we are sure not to regress? Would be a tad annoying to update, but I think it would be worht it on the long run | 08:28 |
| gitlab-br-bot | MR !2070: Restore task element name / element name distinction in UI https://gitlab.com/BuildStream/buildstream/-/merge_requests/2070 | 08:28 |
| benschubert | juergbi: concerning the asserts in the threaded scheduler, I tried on master first, turns out 'update_ready_for_runtime_and_cached is also called in jobs :( Do we want to fix that on master first or should I bundle in my MR? | 08:29 |
| juergbi | benschubert: if it's a simple fix, it's fine with me either way. if it's more delicate, a separate MR would probably be good | 08:33 |
| juergbi | if in the same MR, it should still be a separate commit, of course | 08:33 |
| benschubert | juergbi: ok I'll separate then, it's not a simple one afaict | 08:33 |
| benschubert | Also, __update_cache_key_non_strict is called in the jobs? Is that expected or should I remove that too? | 08:34 |
| juergbi | benschubert: in both cases this is about caching at the end of assemble, right? | 08:40 |
| benschubert | afaict yes | 08:41 |
| juergbi | as __update_cache_key_non_strict() already calculates the cache key on __assemble_scheduled, I don't see a reason why we need this at all | 08:41 |
| juergbi | i.e., maybe we can simply drop that call | 08:41 |
| benschubert | that would make sense | 08:41 |
| juergbi | and then add an assert also to __update_cache_key_non_strict() | 08:41 |
| juergbi | it might have been needed with the old workspace code | 08:41 |
| juergbi | and wasn't removed afterwards | 08:41 |
| *** santi has joined #buildstream | 08:41 | |
| benschubert | seems like it indeed | 08:42 |
| benschubert | I'll clean that up | 08:42 |
| benschubert | thanks! | 08:42 |
| juergbi | I might also be forgetting some complex non-strict case again, of course | 08:42 |
| juergbi | however, hopefully our test suite is complete enough for these cases | 08:42 |
| benschubert | mmh we however know both the strict and non strict key before running nowadays no? Or is that if we don't have the artifact locally and pull it? | 08:44 |
| benschubert | (In which case I could have the call put in a better place, not in the job) | 08:44 |
| juergbi | we should know all keys before we start assembling, yes | 08:45 |
| juergbi | the old workspace code was really the only case where we didn't | 08:45 |
| juergbi | however, we may not know the strong key in the non-strict case at the beginning of a build session (due to pulling) | 08:46 |
| juergbi | __schedule_assembly_when_necessary() calls __update_cache_key_non_strict() | 08:46 |
| benschubert | ah fair, yeah so that _should_ work | 08:46 |
| juergbi | I think this should be enough | 08:46 |
| benschubert | I'll try it out and put a PR up | 08:46 |
| juergbi | ta | 08:47 |
| benschubert | and then look on the last things that I need to fix on this branch, hope to get it done by end of week | 08:47 |
| *** tomaz has quit IRC | 09:18 | |
| *** tristan has quit IRC | 09:40 | |
| *** tristan has joined #buildstream | 09:40 | |
| *** ChanServ sets mode: +o tristan | 09:41 | |
| benschubert | juergbi: Ok I'm getting down the hole: https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/element.py#L1854 is currently required, as 'self.__cache_key' is None and 'self.__artifact._cache_key' too. However, since we' just pulled the artifact, would something like: 'if self.__cache_key is None: read_cache_key_from_artifact' there do? I am not sure what's the best there. We absolutely need cache keys to cache the | 09:46 |
| benschubert | artifact right? | 09:46 |
| juergbi | 'we just pulled the artifact'? | 09:46 |
| juergbi | I thought this is only called at the end of assembly | 09:47 |
| juergbi | we definitely need cache keys to cache the artifact | 09:48 |
| juergbi | however, we should also always be able to calculate them before even starting assembly | 09:49 |
| juergbi | (i.e., already in the main process) | 09:49 |
| benschubert | So all 4 tests failing are around 'no_strict' tests | 09:49 |
| benschubert | let me push a branch | 09:49 |
| juergbi | ah, we schedule assemble even when the element is not buildable yet | 09:50 |
| juergbi | so we might need a __update_cache_key_non_strict() call in _update_ready_for_runtime_and_cached() when it becomes buildable | 09:51 |
| benschubert | ah I can try that! | 09:52 |
| benschubert | Uh, update_ready_for_runtime_and_cached does call update_cache_key_non_strict | 09:56 |
| benschubert | so we really want a circular call there? | 09:56 |
| juergbi | benschubert: oh, that does seem a bit odd, although I don't think there is a real issue as _update_ready_for_runtime_and_cached() has a check to never execute the body twice | 10:19 |
| juergbi | actually, the new call would be on the `rdep` | 10:20 |
| benschubert | yeah, seems like it's actually passing the tests :) | 10:20 |
| juergbi | so it wouldn't be circular for a single element | 10:20 |
| benschubert | I'll update the PR and run a benchmark though | 10:20 |
| benschubert | interesting, didn't fix the tests | 10:21 |
| benschubert | wait, update_ready_for_runtime_and_cached doesn't compute keys | 10:23 |
| benschubert | ah! in update_cache_key_non_strict: | 10:24 |
| benschubert | if self._pull_pending(): | 10:24 |
| benschubert | # Effective strong cache key is unknown until after the pull | 10:24 |
| benschubert | pass | 10:24 |
| benschubert | so that's expected, what we need is to just calculate_cache_key in assemble_done afaict? | 10:25 |
| juergbi | benschubert: if pull is pending, we're not assembling (yet) | 10:33 |
| juergbi | so why would this be an issue? | 10:33 |
| benschubert | ah no right | 10:33 |
| benschubert | there's something odd happening | 10:33 |
| *** tristan has quit IRC | 10:48 | |
| *** coldtom has quit IRC | 10:49 | |
| benschubert | juergbi: https://gitlab.com/BuildStream/buildstream/-/jobs/767327513#L1887 this is what is happening. We do have the 'weak cache key' present at that point, not the strict, and computing the strict cache key and setting it on the artifact is not enough to allow caching it | 10:51 |
| *** coldtom has joined #buildstream | 10:51 | |
| benschubert | I am not familiar enough with that part of the code :/ do you have another pointer? | 10:51 |
| juergbi | let me take a look | 10:53 |
| benschubert | thanks | 10:55 |
| benschubert | https://gitlab.com/BuildStream/buildstream/-/merge_requests/2079/diffs?commit_id=7970101692e40cbc77fab752d84ba674ca47a0b2 is my last try juergbi if ever | 10:56 |
| benschubert | but doesn't work | 10:57 |
| *** santi has quit IRC | 11:04 | |
| *** santi has joined #buildstream | 11:04 | |
| juergbi | benschubert: test suite passes locally with https://gitlab.com/BuildStream/buildstream/-/snippets/2020959 on top of your branch | 11:05 |
| benschubert | ah right -_-' that makes more sense, thanks! | 11:05 |
| juergbi | however, buildable() is not really a good place as a trigger as it's not supposed to have side effects, imo | 11:05 |
| benschubert | so putting it inside the if block of update_ready_... ? | 11:06 |
| juergbi | possibly | 11:06 |
| juergbi | have to eat something now | 11:06 |
| benschubert | haha sure! I'll give it a go thanks! | 11:07 |
| gitlab-br-bot | abderrahimk opened MR !2080 (abderrahim/no-delete-extract->master: _context.py: don't delete bst1 extract directory) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2080 | 13:48 |
| benschubert | juergbi: https://gitlab.com/BuildStream/buildstream/-/merge_requests/2079 is slowing down a bit sadly, I am not sure I can make it much faster without diving much deeper. It is still an improvement in correctness and I added all the asserts I think are needed | 14:44 |
| juergbi | benschubert: the overhead might be slightly lower with the thread-based scheduler | 14:49 |
| juergbi | for the asserts | 14:49 |
| benschubert | Yeah I'd expect that too | 14:50 |
| juergbi | I don't think this is a blocker, especially given that it doesn't make sense profiling before switching to the thread-based scheduler | 14:50 |
| benschubert | ok! then it's ready to be reviewed :) | 14:51 |
| *** tpollard has joined #buildstream | 14:55 | |
| *** tpollard has quit IRC | 16:02 | |
| *** cs-shadow has joined #buildstream | 16:21 | |
| *** santi has quit IRC | 17:40 | |
| *** santix has joined #buildstream | 17:40 | |
| *** santix has quit IRC | 17:48 | |
| *** cs-shadow has quit IRC | 18:41 | |
| *** benschubert has quit IRC | 23:22 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!