| *** alatiera has quit IRC | 01:04 | |
| *** alatiera has joined #buildstream | 01:29 | |
| *** alatiera has quit IRC | 01:30 | |
| *** nimish has joined #buildstream | 02:13 | |
| *** nimish has quit IRC | 03:24 | |
| *** mohan43u has quit IRC | 04:22 | |
| *** mohan43u has joined #buildstream | 04:23 | |
| *** mohan43u has quit IRC | 04:42 | |
| *** mohan43u has joined #buildstream | 04:52 | |
| *** tristan has joined #buildstream | 06:47 | |
| *** bochecha has joined #buildstream | 06:53 | |
| juergbi | hi tristan, do you have any comments on !1182 ? | 07:48 |
|---|---|---|
| *** mohan43u has quit IRC | 07:48 | |
| *** ChanServ sets mode: +o tristan | 07:49 | |
| *** mohan43u has joined #buildstream | 07:50 | |
| tristan | That looks pretty huge | 07:51 |
| tristan | juergbi, So is this the same filter callback thing across the board ? it is hard to see what it is supposed to do looking at the utils.copy_files() functions | 07:54 |
| juergbi | tristan: the first 12 commits are from the underlying MR !1181, no need to review that as part of !1182 | 07:56 |
| tristan | Ah so it is supposed to be called on a path and returns true if it is supposed to be included | 07:56 |
| juergbi | yes, exactly | 07:57 |
| juergbi | i.e., similar to the predicate in python filter() | 07:57 |
| tristan | I suppose since that is pretty straight forward, the same filter functions are usable regardless of which API is used (the utils stuff or abstract directory stuff) | 07:58 |
| juergbi | the actual code changes in this MR aren't that big, if the underlying MR is ignored | 07:58 |
| juergbi | exactly | 07:58 |
| juergbi | same interface for all | 07:58 |
| juergbi | there is a follow up MR !1183 that optimized the abstract directory implementations based on this MR, but that can be reviewed separately | 07:59 |
| gitlab-br-bot | MR !1183: WIP: Directory.import_files() improvements https://gitlab.com/BuildStream/buildstream/merge_requests/1183 | 07:59 |
| juergbi | with the whole MR series, I'm seeing about 40% reduction in staging time and 5% reduction in overall build time of freedesktop-sdk | 07:59 |
| juergbi | and with BuildBox / remote execution (pure CAS virtual staging), the staging performance improvement is much higher | 08:01 |
| tristan | juergbi, How does this Directory.import_files() work, i.e. the directory is already an object and represents a location so I suppose the directory itself is the destination where files should be added ? | 08:15 |
| juergbi | tristan: yes, 'self' is the destination | 08:15 |
| tristan | In which case, what "Directory" do the files come from ? | 08:15 |
| juergbi | the first argument | 08:15 |
| tristan | And what is the path given to the filter function relative to ? | 08:15 |
| tristan | Oh the pathspec | 08:15 |
| *** toscalix has joined #buildstream | 08:16 | |
| juergbi | path relative to that pathspec | 08:16 |
| tristan | So in this change (compose element) https://gitlab.com/BuildStream/buildstream/merge_requests/1182/diffs?commit_id=0bddc2b7f2a923d1b5673429fcddb1ca70cc977c | 08:16 |
| juergbi | i.e., identical to what would have been in the file list | 08:16 |
| tristan | We are passing a filter callback which expects a path which is relative to vbasedir | 08:17 |
| tristan | And putting the files in installdir | 08:17 |
| tristan | juergbi, It all makes sense, but I would like all the internal API to be more thoroughly documented | 08:17 |
| juergbi | i.e., the split helpers? | 08:18 |
| juergbi | will improve doc, thanks for the review | 08:18 |
| tristan | Like the signature of the filter callback (already commented in one place), and what the file path is, is it a string ? will it have a leading slash ? | 08:18 |
| tristan | That kind of stuff | 08:18 |
| juergbi | filter_callback (callable): Optional filter callback. Called with the | 08:19 |
| juergbi | relative path as argument for every file in the source directory. | 08:19 |
| juergbi | The file is imported only if the callable returns True. | 08:19 |
| juergbi | If no filter callback is specified, all files will be imported. | 08:19 |
| juergbi | tristan: does this sound clear? | 08:19 |
| tristan | That is much better yes :) | 08:19 |
| tristan | juergbi, Anyway for the rest it all looks very straight forward, even the splits change seems pretty straight forward | 08:22 |
| juergbi | yes, used small commits for refactoring to make it easier for review | 08:22 |
| tristan | while returning a function with 'partial' is a funky little trick, its not black magic :) | 08:22 |
| juergbi | :) | 08:25 |
| gitlab-br-bot | juergbi opened MR !1185 (juerg/unlink->master: utils.py: safe_link(): Unlink only if target already exists) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1185 | 08:50 |
| *** phildawson has joined #buildstream | 09:21 | |
| *** raoul has joined #buildstream | 09:26 | |
| benschubert | tristan: am I right if I say that the only variable we have to track in _update_state() is "__cache_key", meaning, if it didn't change, we don't need to update parents? Is there other variables to check for that? | 09:36 |
| tristan | benschubert, Ummm no I don't think that is right | 09:42 |
| tristan | benschubert, there are a few different keys, the weak and the strong key basically | 09:43 |
| tristan | lemme check what is __cache_key | 09:43 |
| tristan | :-S | 09:43 |
| tristan | element.py has gone out of sync and no longer declares all instance variables in __init__() ? | 09:44 |
| tristan | __strict_cache_key and __weak_cache_key are missing and undocumented | 09:44 |
| juergbi | self.__weak_cache_key = None # Our cached weak cache key | 09:46 |
| juergbi | self.__strict_cache_key = None # Our cached cache key for strict builds | 09:46 |
| juergbi | it's all there | 09:46 |
| juergbi | (I think lint would complain otherwise) | 09:46 |
| tristan | Ah you are right | 09:47 |
| tristan | Looking at how cache keys are resolved, only __cache_key and __weak_cache_key serve as input for reverse dependency cache key calculation | 09:47 |
| tristan | benschubert, ^^ | 09:48 |
| benschubert | so checking if both have changed would allow me to know if reverse deps needs to be re-calculated correct? | 09:48 |
| tristan | actually no :( | 09:48 |
| tristan | __strict_cache_key is also used | 09:49 |
| benschubert | but __cache_key will change is __strict_cache_key changes no? | 09:49 |
| tristan | that appears to be true in strict mode | 09:51 |
| tristan | and otherwise it would seem to be ignored | 09:51 |
| juergbi | I think __cache_key alone would actually be sufficient | 09:53 |
| juergbi | if the others are set but __cache_key not yet, it should be ok to defer reverse dependency update | 09:53 |
| juergbi | but it's tricky... | 09:53 |
| benschubert | I will see if that breaks any test | 09:56 |
| benschubert | thanks! | 09:56 |
| tristan | benschubert, As I suspected, I think you are only recording reverse build-only dependencies | 09:57 |
| benschubert | Also tristan, could you explain to me why "If I am not mistaken, I believe that when an element is pulled from the remote and BuildStream is running in non-strict mode, the reverse dependencies need to also be updated." ? I'm not sure I get why this should be the case :) | 09:57 |
| juergbi | benschubert: btw: not sure whether you've missed my small comment https://gitlab.com/BuildStream/buildstream/merge_requests/1070#note_144660034 | 09:57 |
| benschubert | tristan: if I remember well, reverse runtime dependencies keys are not affected by the cache key no? | 09:58 |
| juergbi | tristan, benschubert: in non-strict mode, while pull is still pending __cache_key won't be set yet, so I don't think this needs any special handling for non-strict mode | 09:59 |
| tristan | benschubert, regarding only recording reverse build dependencies: This will fail to update the reverse dependency states when (A) We discover a cache key for an element, (B) The element is a runtime-only dependency of another element, (C) That other element is a build dependency of another element (final element misses the update, but still *requires* the original runtime-only dependency) | 09:59 |
| benschubert | juergbi: I saw it, thanks! I will however very likely remove the it :) | 09:59 |
| tristan | benschubert, build dependencies != Scope.BUILD | 09:59 |
| benschubert | tristan: ah, so I need to get all with Scope == Build, I see | 10:00 |
| juergbi | I agree, runtime dependencies are relevant as well | 10:00 |
| *** jonathanmaw has joined #buildstream | 10:02 | |
| *** alatiera has joined #buildstream | 10:05 | |
| benschubert | tristan: however, no tests were exercising this :/ | 10:07 |
| benschubert | juergbi: " in non-strict mode, while pull is still pending __cache_key won't be set yet, so I don't think this needs any special handling for non-strict mode" so after pulling, we should update all reverse deps again correct? | 10:08 |
| juergbi | yes, however, __cache_key won't be set before anyway, so it shouldn't be 'again' | 10:08 |
| benschubert | right! | 10:09 |
| benschubert | we do it anyways before we run, not doing it might be more complex | 10:10 |
| tristan | benschubert, yeah we're missing some coverage in that area | 10:13 |
| tristan | not the easiest test to write but probably not *too* hard :-/ | 10:13 |
| benschubert | tristan: I think it would be worth writing an issue listing all tests we are missing around this, we would then be able to bridge the gaps more easily and direct people on tests | 10:14 |
| tristan | benschubert, well maybe - I mean that kind of issue has a tendency of never being closable | 10:17 |
| tristan | on the other hand if one were to do a concerted effort to find out what tests we should have and are missing, then might as well also write the tests | 10:17 |
| benschubert | right | 10:17 |
| * tristan personally likes wikis for this kind of long term efforts | 10:20 | |
| tristan | if they end up being long term that is | 10:20 |
| tristan | or roadmapping of efforts in general, wiki pages with links to issues | 10:21 |
| *** tristan has quit IRC | 10:27 | |
| *** tristan has joined #buildstream | 10:32 | |
| *** lachlan has joined #buildstream | 10:32 | |
| *** toscalix has quit IRC | 10:34 | |
| *** nimish has joined #buildstream | 10:36 | |
| juergbi | small staging performance improvement ready for review: !1185 | 10:49 |
| gitlab-br-bot | MR !1185: utils.py: safe_link(): Unlink only if target already exists https://gitlab.com/BuildStream/buildstream/merge_requests/1185 | 10:49 |
| gitlab-br-bot | juergbi merged MR !1185 (juerg/unlink->master: utils.py: safe_link(): Unlink only if target already exists) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1185 | 11:01 |
| *** lachlan has quit IRC | 11:28 | |
| *** nimish has quit IRC | 11:36 | |
| *** nimish has joined #buildstream | 11:36 | |
| *** lachlan has joined #buildstream | 11:37 | |
| *** raoul_ has joined #buildstream | 12:01 | |
| *** raoul has quit IRC | 12:02 | |
| gitlab-br-bot | marge-bot123 merged MR !1181 (juerg/directory->master: Virtual directory improvements) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1181 | 12:06 |
| gitlab-br-bot | juergbi opened (was WIP) MR !1182 (juerg/import-filter->master: Replace file lists with filter callback for file import) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1182 | 12:07 |
| *** nimish has quit IRC | 12:14 | |
| benschubert | tristan: , juergbi I'm struggling to understand why build and pull jobs need the CACHE resource, but push jobs don't. Is it documented somewhere? | 12:32 |
| *** bochecha has quit IRC | 12:44 | |
| *** nimish has joined #buildstream | 12:53 | |
| juergbi | benschubert: I wasn't really involved in that aspect, however, my guess would be it's for cache write access | 12:59 |
| juergbi | push only needs read access | 13:00 |
| benschubert | juergbi: why do we need to lock write access? Knowing that the lock is only between all jobs and the cache cleaning | 13:04 |
| juergbi | to avoid issues between write and clean? | 13:05 |
| juergbi | we can hopefully simplify this anyway when moving to object-based expiration (with partial CAS support in place) | 13:06 |
| juergbi | tlater[m] or tristan probably know more about the CACHE scheduler resource | 13:07 |
| tlater[m] | benschubert: cache cleanup is not atomic in any way. If we write when we're cleaning, the cleanup functions might remove things as they are being added. | 13:12 |
| jonathanmaw | tristan: do I understand correctly from https://gitlab.com/BuildStream/buildstream/merge_requests/1070#note_145007411 that Element._update_state was created _because_ there was a lot of confusion around external callers controlling the element state, and so any optimisations in this area should not reintroduce this confusion? | 13:14 |
| *** nimish has quit IRC | 13:14 | |
| tlater[m] | Unless someone has changed how the cache works since I last looked at it, of course :) | 13:16 |
| benschubert | tlater[m]: ah because the reference is what is done last, so we have dangling stuff, correct? | 13:18 |
| tlater[m] | Yep, exactly. | 13:18 |
| *** raoul_ has quit IRC | 13:21 | |
| gitlab-br-bot | martinblanchard opened MR !1186 (mablanch/799-RE-optional-TLS->master: Optional TLS support for remote-execution storage service) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1186 | 13:29 |
| benschubert | tlater[m]: ok! that explains also why we would get problems with the cache getting full and the cache cleaning only kicking in afterwards x') | 13:31 |
| tlater[m] | Yep | 13:31 |
| tlater[m] | We really need to get someone to walk through every location an out of memory error can occur and make those invoke cleanup instead. But that almost feels impossible :| | 13:32 |
| tlater[m] | Our solution so far was the headroom thingie but it's clearly not appropriate for all use cases. | 13:33 |
| juergbi | tlater[m]: casd is likely the best approach to solve this | 13:34 |
| juergbi | all writes will be handled by that, assuming we switch to casd at all | 13:34 |
| tlater[m] | juergbi: Yep, that does sound better. | 13:36 |
| tlater[m] | Anything that can guarantee cache atomicity would be great. | 13:37 |
| juergbi | tpollard: are you in process or planning to review !1182? if yes, I'll wait for that. otherwise, I'll merge it as it has already been reviewed by others | 13:38 |
| gitlab-br-bot | MR !1182: Replace file lists with filter callback for file import https://gitlab.com/BuildStream/buildstream/merge_requests/1182 | 13:38 |
| tpollard | juergbi: planning to take a look asap | 13:39 |
| juergbi | ok, thanks | 13:39 |
| coldtom | is anyone from bst-external around to take a look at https://gitlab.com/BuildStream/bst-external/merge_requests/70 ? | 13:43 |
| jonathanmaw | lachlan: I've commented on your benchmarks MR | 13:53 |
| *** raoul_ has joined #buildstream | 13:53 | |
| jonathanmaw | it looks like there's been a deliberate decision for that command to succeed even if it fails to write the output to file | 13:54 |
| jonathanmaw | since you're catching the exceptions and calling logging.error() | 13:55 |
| gitlab-br-bot | tpollard approved MR !1182 (juerg/import-filter->master: Replace file lists with filter callback for file import) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1182 | 14:00 |
| gitlab-br-bot | juergbi merged MR !1182 (juerg/import-filter->master: Replace file lists with filter callback for file import) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1182 | 14:01 |
| 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 | 14:02 |
| lachlan | Thanks jonathanmaw | 14:23 |
| lachlan | jonathanmaw: Have responded | 14:40 |
| jjardon | laurence: is there a MR to update the website about what versions of best we recommend? I remember I have to update the README | 15:14 |
| Kinnison | Does anyone know what could cause the WSL runner to get stuck like this: https://gitlab.com/BuildStream/buildstream/-/jobs/167863148 ? | 15:18 |
| gitlab-br-bot | danielsilverstone-ct opened (was WIP) 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 | 15:19 |
| *** lachlan has quit IRC | 15:28 | |
| laurence | jjardon, not yet, still on my todo, sorry | 15:29 |
| *** lachlan has joined #buildstream | 15:44 | |
| *** jonathanmaw has quit IRC | 18:02 | |
| *** raoul_ has quit IRC | 18:22 | |
| *** lachlan has quit IRC | 18:36 | |
| *** slaf has quit IRC | 19:18 | |
| *** slaf has joined #buildstream | 19:21 | |
| *** alatiera has quit IRC | 20:13 | |
| *** sebastian has quit IRC | 21:10 | |
| *** nimish has joined #buildstream | 21:16 | |
| *** sebastian has joined #buildstream | 21:21 | |
| *** alatiera has joined #buildstream | 21:43 | |
| *** tristan has quit IRC | 22:39 | |
| * SotK finally gets round to sending https://gitlab.com/BuildStream/bst-external/merge_requests/71 | 23:09 | |
| *** alatiera has quit IRC | 23:32 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!