| *** ctgriffiths has quit IRC | 00:49 | |
| *** ctgriffiths has joined #buildstream | 00:50 | |
| *** xjuan has quit IRC | 01:01 | |
| *** bochecha has quit IRC | 01:41 | |
| *** kolbe has joined #buildstream | 04:42 | |
| *** Prince781 has quit IRC | 04:59 | |
| *** catonano has joined #buildstream | 05:00 | |
| *** catonano has quit IRC | 05:30 | |
| *** catonano has joined #buildstream | 05:56 | |
| *** obre has joined #buildstream | 06:21 | |
| *** ItzExor has joined #buildstream | 07:21 | |
| *** warweasle has joined #buildstream | 08:13 | |
| *** tristan has joined #buildstream | 08:13 | |
| *** toscalix has joined #buildstream | 08:43 | |
| *** catonano has quit IRC | 08:47 | |
| *** alatiera_ has joined #buildstream | 09:15 | |
| laurence | mablanch, I think this ticket can be closed? https://gitlab.com/BuildStream/buildstream/issues/630 | 09:25 |
|---|---|---|
| mablanch | laurence: Yep, I think so! | 09:28 |
| laurence | mablanch, cool, thanks ! | 09:29 |
| *** tristan has quit IRC | 09:31 | |
| *** raoul has joined #buildstream | 09:35 | |
| toscalix | valentind: and I went over some tickets today. There is plenty of them still on the backlog. We will have another session on Monday as usual. | 09:39 |
| *** benschubert has joined #buildstream | 09:39 | |
| toscalix | laurence: I just did close it | 09:41 |
| benschubert | Any maintainer with bandwidth for a quick review of https://gitlab.com/BuildStream/buildstream/merge_requests/886? I just want to make sure I'm not breaking any feature I might have overlooked :) | 09:47 |
| *** jonathanmaw has joined #buildstream | 09:50 | |
| *** tiagogomes has joined #buildstream | 09:54 | |
| juergbi | benschubert: it seems fine to me but I'm not a pytest/setuptools expert | 10:00 |
| juergbi | benschubert: maybe explain the reason for the change in the commit message itself. dropping the only setup_requires entry | 10:01 |
| benschubert | ok sure I will do that, rebase and then merge! Thanks a lot! | 10:01 |
| WSalmon | could some one how knows more than me about cli.run from tests have a look at https://gitlab.com/BuildStream/buildstream/issues/736#note_113732200 Many thanks, tlater[m] juergbi tristan skullman jonathanmaw | 10:09 |
| *** tristan has joined #buildstream | 10:09 | |
| * jonathanmaw has a poke | 10:09 | |
| WSalmon | ta | 10:10 |
| jonathanmaw | WSalmon: I'm fairly sure that nothing in cli.run explicitly interacts with the Platform, and the reason why it's persisting between invocations of buildstream is because we directly execute buildstream's main(), rather than having a separate python context | 10:17 |
| WSalmon | but isnt the issue that it is not! surely cli.run should be resetting stuff like that? | 10:17 |
| jonathanmaw | it sounds like the appropriate solution to me is either finding some way of separating out python contexts (I tried forking once, it went badly), or manually resetting the context between invocations | 10:18 |
| skullman | resetting the context would be the least awful IMO | 10:18 |
| jonathanmaw | s/resetting the context/resetting the platform/ | 10:18 |
| * skullman would prefer an explicit reset of the context to cli.run automatically doing it | 10:18 | |
| WSalmon | why? | 10:21 |
| *** bochecha has joined #buildstream | 10:22 | |
| benschubert | What about a fixture that does only that, and is autoused in all the integration tests? | 10:23 |
| skullman | WSalmon: don't want to do something abnormal unless it's necessary for that test, and caching is the default behaviour | 10:24 |
| benschubert | So using an explicit fixture in that specific case? I'm not sure about it, we might have unknown dependencies between tests because of this behavior and I'd rather be safe, at least resetting it between every integration test | 10:25 |
| skullman | ¯\_(ツ)_/¯ it's a weak preference to be given less weight than the opinion of whoever's doing the work | 10:27 |
| juergbi | global pytest setup function that resets this sounds acceptable to me | 10:30 |
| juergbi | that would simply be one step closer to the ideal world where we have a separate python context for each test | 10:31 |
| *** lachlan has joined #buildstream | 10:31 | |
| benschubert | Should we do it at the global level (also for unit tests) or just for the integration one? I'll submit a PR asap :) | 10:31 |
| juergbi | most of our non-integration tests are much more than unit tests and also use cli.run | 10:31 |
| juergbi | I'd do it for all, don't see why not | 10:31 |
| benschubert | sure! | 10:32 |
| juergbi | or do you expect a big performance issue? | 10:32 |
| juergbi | might be worth checking whether a full test run gets significantly slower | 10:32 |
| benschubert | I'll do both! | 10:33 |
| juergbi | ta | 10:33 |
| benschubert | first check with master then the change | 10:33 |
| *** ChanServ sets mode: +o tristan | 10:46 | |
| tristan | Gah, gitlab is broken again | 10:46 |
| * tristan works around | 10:46 | |
| *** jonathanmaw_ has joined #buildstream | 10:51 | |
| *** jonathanmaw has quit IRC | 10:52 | |
| Kinnison | juergbi: I think your last open discussion point on https://gitlab.com/BuildStream/buildstream/merge_requests/786 has been addressed by tpollard. If that was the last of the review cycle, it'd be awesome if you'd resolve that and approve so we can get this MR landed :-) | 11:23 |
| *** ahmed89 has joined #buildstream | 11:31 | |
| Kinnison | skullman: Has anyone offered any reviews of !909 for you yet? | 11:31 |
| skullman | no, but I've not pushed and am behind in reviewing other stuff for the goodwill to deserve it | 11:32 |
| * Kinnison will see if he can review at some point today, though I'm not the best person for the job :-) | 11:32 | |
| jmac | I'll take a look too | 11:33 |
| Kinnison | Does anyone here have a good understanding of the use of the ChainMap in _yaml.py ? | 11:36 |
| tristan | Kinnison, Yep | 11:37 |
| tristan | Probably could use a rename | 11:37 |
| Kinnison | tristan: aah you're online - awesome | 11:37 |
| tristan | ShadowMap ? ShallowMap ? | 11:37 |
| tristan | Kinnison, ChainMap, the original python one, basically implements the Mapping type (like a dictionary), with a list of Mappings internally | 11:38 |
| Kinnison | tristan: The reason I'm asking is because profiling `bst build` to the point just before `run()` on the scheduler has resulted in ChainMap.__{contains,getitem}__ showing up as serious hotspots (totalling a significant proportion of pre-scheduler time) | 11:39 |
| tristan | We use this construct to optimize the load time and avoid creating copies of dictionaries | 11:39 |
| tristan | Our version, in addition to pythons version, not only combines the keys/values of the underlying mappings, but also keeps track of deleted keys | 11:40 |
| Kinnison | tristan: roughly 20% of CPU time seems to be in those methods | 11:40 |
| tristan | Kinnison, Sure, but pre-scheduler time is going to blow up a *lot* if we do copies instead :) | 11:40 |
| Kinnison | tristan: I appreciate that, I'm just wondering if there's some way we can improve matters | 11:41 |
| Kinnison | tristan: if this is a dead-end then I need to look at other options | 11:41 |
| Kinnison | step 1 is always "Can we improve the obvious stuff" :-) | 11:41 |
| tristan | Yeah, I'm not sure | 11:41 |
| * Kinnison feared that :/ | 11:42 | |
| tristan | Kinnison, Looking at contains/getitem there... it does look like *possibly* we can improve the surrounding calls | 11:42 |
| tristan | For instance... in the patch which does the YamlCache optimization, the first iterations were doing things like: if "foo" in dictionary: ... foo = dictionary[foo] | 11:43 |
| Kinnison | nod. | 11:43 |
| tristan | Instead of: try: foo dictionary['foo'] except: ... | 11:43 |
| tristan | Rooting out any of that might significantly half the lookups | 11:43 |
| * Kinnison basically noticed that since the optimisation patches he landed last Friday, around half of pre-scheduler time in build is spent constructing element instances, and a large proportion of that ends up in _yaml.py::node_get() which is dominated by the ChainMap | 11:44 | |
| Kinnison | (note, I'm assuming the yaml is already in the cache for these profiles) | 11:44 |
| tristan | Yeah, the YamlCache is not the point, it's just a place where I noticed double-lookups occurring in hot codepaths | 11:45 |
| tristan | (which I believe was fixed before merging anyway) | 11:45 |
| tristan | But, if lookups are dominating load time, there is a good chance that other code segments also have this problem | 11:46 |
| tristan | Maybe even node_get() | 11:46 |
| Kinnison | I'll continue to focus efforts at node_get() for now then | 11:46 |
| Kinnison | Though if you spot any potential gains, I'd be happy to try them out in my profiling rig | 11:46 |
| * Kinnison is running a 25,000 element profiling project because that's similar in size to a client of mine's projects | 11:47 | |
| tristan | looks like that is using the .get(key, default_return) accessor; which I would presume to be optimized by python | 11:48 |
| tristan | Not sure if that is faster or if try / except is faster, but they should be equal | 11:48 |
| Kinnison | def get(self, key, default=None): | 11:50 |
| Kinnison | return self[key] if key in self else default | 11:50 |
| Kinnison | from collections/__init__.py | 11:50 |
| Kinnison | Assuming you're interested in the ChainMap base from python | 11:50 |
| tristan | Well | 11:50 |
| tristan | That is amusing | 11:50 |
| tristan | Kinnison, Try changing _yaml.node_get() to use try / except and see if that halfs the time of contains/getitem | 11:51 |
| Kinnison | would overriding def get() in _yaml.py's ChainMap to be: | 11:52 |
| Kinnison | def get(self, key, default=None): | 11:52 |
| Kinnison | try: | 11:52 |
| Kinnison | return self[key] | 11:52 |
| Kinnison | except KeyError: | 11:52 |
| Kinnison | return default | 11:52 |
| Kinnison | be easier? | 11:52 |
| juergbi | Kinnison: !786 I've resolved that discussion. however, tpollard is asking tristan for a review, in which case a re-review from my side should not be necessary (all my original points were addressed but because there were a few iterations I'd have to do a re-review to some extent) | 11:52 |
| Kinnison | juergbi: aah righty | 11:52 |
| tristan | Kinnison, maybe, I'm not sure that we want to rely on everything always being a ChainMap, though | 11:53 |
| Kinnison | tristan: I'll try the above addition of an overridden get() for now and see if that knocks down how much ChainMap.__{getitem,contains}__ shows up | 11:53 |
| tristan | Kinnison, Also, if this turns out to really save time, we have a while surface of .get calls all around the codebase which are worth investigating | 11:53 |
| * Kinnison nods | 11:53 | |
| tristan | s/while/whole | 11:54 |
| Kinnison | tristan: since I have your attention, any chance you can look over !786? | 11:54 |
| Kinnison | tristan: given juergbi's comment above | 11:55 |
| tristan | Kinnison, Ah I see in collections that is *only* for ChainMap | 11:55 |
| Kinnison | tristan: yeah, I think further .get()s might be cleaner | 11:56 |
| tristan | Sure, lets see which one of the dozens of MRs that one is about... | 11:56 |
| Kinnison | tristan: don't always pull buildtrees | 11:56 |
| tristan | Ah, the important one | 11:56 |
| Kinnison | :-) | 11:56 |
| Kinnison | bahaha | 11:58 |
| Kinnison | tristan: excellent call. That replacement .get() method knocked 50% off node_get's cost | 11:58 |
| * Kinnison will take a further look and submit a clean MR later today | 11:58 | |
| Kinnison | In the meantime I'll grab some lunch | 12:00 |
| tristan | Kinnison, Nice ! | 12:09 |
| ironfoot | tristan: do you want the bot to answer whenever a mention to a MR / Issue appears in channel? | 12:15 |
| ironfoot | "<tristan> Sure, lets see which one of the dozens of MRs that one is about..." | 12:15 |
| jmac | That could make standups interesting | 12:17 |
| ironfoot | true _o_ | 12:17 |
| jmac | Unless it just responds to "!666" and not "[[!bsissue 666]]]", I suppose | 12:17 |
| jmac | Wait, standups have no meaning here | 12:17 |
| ironfoot | in anycase it will only respond to !xxxx and #xxxx | 12:18 |
| jmac | Yes, sorry, I'm on the wrong channel - we don't use the bot on our internal channel so it won't matter | 12:19 |
| tristan | ironfoot, YES !!!! | 12:19 |
| raoul | Anyone want to review https://gitlab.com/BuildStream/buildstream/merge_requests/900? Just checked it worked with an update to buildgrid | 12:20 |
| tristan | ironfoot, I'm so jealous that the bots in #gnome-hackers and #gtk+ do that :) | 12:20 |
| ironfoot | tristan: I will make a note, and do it once I get the current situation fixed (bot broken) | 12:20 |
| tristan | ... Now if only we could avoid premature creation of merge requests, we could also cut down on the spam ... <whistle to self.../> | 12:21 |
| ironfoot | *if* they are marked as WIP maybe that's something I could fix in the bot side of things | 12:22 |
| tristan | Oh yeah, that would be cool too ! | 12:22 |
| ironfoot | will keep you updated | 12:23 |
| benschubert | I've made initial tests for resetting the platform between each test. The results are: without change: 559s, 570s, 574s, with changes: 587s, 583s. This hints to a 2-5% performance hit. What are your thoughts? juergbi ? | 12:27 |
| benschubert | My take is, it would be an acceptable slow down for better isolating tests | 12:30 |
| tristan | juergbi, Are you confident in the CAS/ArtifactCache level changes in !786 ? E.g. this conversation appears resolved but is unclear to me whether it really is: https://gitlab.com/BuildStream/buildstream/merge_requests/786/diffs#note_109951635 | 12:31 |
| tristan | benschubert, I agree, proper tear down is worth it | 12:32 |
| Kinnison | jennis: If you have time, !919 might be of interest to you | 12:46 |
| tristan | tpollard, Kinnison, juergbi; a ton of comments posted on 786 - including a couple of things which are are not strictly related to the patch but are obviously broken and it makes sense to fix instead of growing on top of the breakage | 12:59 |
| tristan | juergbi, I did not take the exhaustive time to understand all of the low level CAS related changes and would appreciate that you look at only that, since it is much closer to home for you | 13:00 |
| tristan | To elaborate on the above: (A) There are missing API documenting comments in element.py, this patch extends their arguments but doesnt fix them... (B) These same functions are declared with core visibility (single underscore), but are internal private details to Element (should be moved to the internal/private section, and have double underscores) | 13:01 |
| tristan | (C): There is a wild assumption currently living in `Element._stage_sources_at()`, which makes the dangerous assumption that well ... if we have a build tree then let's go ahead and stage it, who cares who is asking ! | 13:02 |
| tristan | This patch builds on that wildly dangerous assumption but should fix it instead. | 13:02 |
| * tpollard notes that (C) was very recently merged, this MR just mitigates it not being available | 13:03 | |
| tristan | Other comments relate mostly to API interactions of modules | 13:03 |
| mablanch | raoul: I can have look [at !900]. | 13:22 |
| *** ahmed89 has quit IRC | 13:22 | |
| *** tristan has quit IRC | 13:34 | |
| * Kinnison hrfs. !919 says it'll close #466 but I never said that to it. I wonder if it's possible to unmake that relationship. If not, I'll just have to reopen #466 once !919 gets merged | 13:37 | |
| *** Prince781 has joined #buildstream | 13:38 | |
| Kinnison | Fixed | 13:43 |
| Kinnison | sigh | 13:43 |
| *** WSalmon has quit IRC | 13:43 | |
| *** WSalmon has joined #buildstream | 13:43 | |
| benschubert | thanks Tristan! I'll make a MR then | 14:00 |
| *** lachlan has quit IRC | 14:01 | |
| *** tristan has joined #buildstream | 14:03 | |
| *** lachlan has joined #buildstream | 14:17 | |
| *** ChanServ sets mode: +o tristan | 14:30 | |
| tristan | jmac, There is a problem with SandboxRemote, it redeclares it's own brand of SandboxError, instead of using the one from _exceptions | 14:30 |
| tristan | jmac, Looking at the first errors it raises, they seem to be intended to be user facing | 14:31 |
| tristan | The way it is implemented, they will show up as BUG messages | 14:31 |
| tristan | which should normally just be triggered by assert statements | 14:31 |
| jmac | Ok, thanks for raising an issue about it, I'll take a look before the end of the week | 14:55 |
| lachlan | Kinnison: Thanks for the review of !917 | 15:31 |
| *** jonathanmaw_ has quit IRC | 15:31 | |
| *** jonathanmaw has joined #buildstream | 15:32 | |
| lachlan | Does anyone on channel have experience of gitlab ci triggers and have time to review one for benchmarking? | 15:32 |
| *** brlogger has joined #buildstream | 16:08 | |
| Kinnison | jennis: thanks | 16:23 |
| Kinnison | skullman: I've skimmed through !909 for you. Only one thing for me to raise, but there's plenty of code there I'm not in a position to grok right now | 16:24 |
| tpollard | tristan: I've replied to the bulk of your comments, some of them are more problematic than others in the context of the MR | 16:44 |
| adds68 | If a project junctions another project, will the junctions data be available via the "public_data" when writing a plugin? | 16:51 |
| *** xjuan has joined #buildstream | 16:59 | |
| skullman | juergbi: I've got https://gitlab.com/BuildStream/buildstream/merge_requests/920/diffs?commit_id=d614987bd098cb4d7eb0578b62bb64a3ecb58654#1c09d8ba42b5e54906be5cb0a4042b329f2b8104_855_896 for showing artifact logs. It's using CASCache private methods because the API doesn't have anything public for handling refs, everything takes elements and cache keys. | 17:05 |
| skullman | I'm guessing I'll need to add or promote existing method to APIs that take those | 17:06 |
| juergbi | skullman: yes. the untangling of ArtifactCache and CASCache should help here. MR for this should follow shortly | 17:07 |
| skullman | cool, looks like I'll be rebasing next week then, I'll make a note to watch out for it | 17:07 |
| juergbi | with the untangling, the public API of CASCache will always use refs, never elements/cachekeys. and the public API of ArtifactCache will accept elements/cachekeys | 17:07 |
| *** jonathanmaw_ has joined #buildstream | 17:12 | |
| *** jonathanmaw has quit IRC | 17:13 | |
| *** abderrahim1 has quit IRC | 17:23 | |
| *** abderrahim2 has joined #buildstream | 17:23 | |
| jmac | I've added some review comments to !909. | 17:25 |
| skullman | ta, I'll get on them on monday | 17:27 |
| *** ChanServ sets mode: +o tristan | 17:31 | |
| * tristan winks at juergbi and looks forward to embracing a bright future of untangling ;-) | 17:31 | |
| juergbi | hehe | 17:31 |
| tristan | The build trees MR would be easier to review in that world, I think | 17:31 |
| juergbi | probably a bit, yes. I'm looking forward to also getting rid of extract directories, which would have simplified that MR as well, but that's further into the future... | 17:32 |
| tristan | well, we can agree that even the patch would have been easier to write, as well :D | 17:33 |
| *** Prince781 has joined #buildstream | 17:45 | |
| *** jonathanmaw_ is now known as jonathanmaw | 17:50 | |
| jonathanmaw | Is there a way in buildstream's tests to make tmpdir and datafiles point to different directories? | 18:02 |
| jonathanmaw | the only way I can find is to use tmpdir_factory and do tmpdir_factory.mktemp() | 18:08 |
| * jonathanmaw will see how well that goes | 18:08 | |
| lachlan | I'm finding gitlabs caching approach to be unreliable and would like to canvas opinion on how and where benchmarking data should be stored? | 18:08 |
| *** raoul has quit IRC | 18:24 | |
| WSalmon | Hi tristan & tlater[m], could you please add 'check my implementations of your review points' to your respective todo's, many thanks (https://gitlab.com/BuildStream/buildstream/merge_requests/897) | 18:27 |
| *** lachlan has quit IRC | 18:30 | |
| *** jonathanmaw has quit IRC | 18:44 | |
| *** catonano has joined #buildstream | 19:09 | |
| *** toscalix has quit IRC | 19:40 | |
| *** Prince781 has quit IRC | 19:51 | |
| *** Prince781 has joined #buildstream | 19:55 | |
| *** benschubert has quit IRC | 20:10 | |
| *** Prince781 has quit IRC | 20:10 | |
| *** Prince781 has joined #buildstream | 20:17 | |
| *** toscalix has joined #buildstream | 20:28 | |
| *** toscalix has quit IRC | 20:30 | |
| *** Prince781 has quit IRC | 20:39 | |
| *** Prince781 has joined #buildstream | 20:43 | |
| *** catonano has quit IRC | 20:54 | |
| *** tristan has quit IRC | 21:26 | |
| *** alatiera_ has quit IRC | 22:32 | |
| *** Prince781 has quit IRC | 22:49 | |
| *** Prince781_ has joined #buildstream | 22:49 | |
| *** Prince781_ is now known as Prince781 | 22:50 | |
| *** Prince781 has quit IRC | 23:17 | |
| *** Prince781 has joined #buildstream | 23:19 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!