| *** tristan has quit IRC | 04:07 | |
| *** tristan has joined #buildstream | 05:27 | |
| *** ChanServ sets mode: +o tristan | 05:27 | |
| *** hasebastian has joined #buildstream | 05:34 | |
| tristan | INTERNALERROR> coverage.misc.CoverageException: Conflicting file tracer name for '/builds/BuildStream/buildstream/src/buildstream/_variables.pyx': 'Cython.Coverage.Plugin' vs '' | 05:42 |
|---|---|---|
| tristan | :-S | 05:42 |
| juergbi | tristan: python 3.8? | 06:39 |
| *** tristan has quit IRC | 07:13 | |
| *** benschubert has joined #buildstream | 07:35 | |
| *** phildawson has joined #buildstream | 08:00 | |
| *** traveltissues has joined #buildstream | 08:02 | |
| *** tristan has joined #buildstream | 08:29 | |
| *** ChanServ sets mode: +o tristan | 08:29 | |
| tristan | juergbi, CI | 08:30 |
| tristan | juergbi, I probably messed something out, and think benschubert might have an answer | 08:31 |
| tristan | cython is full of little secrets and surprises | 08:31 |
| juergbi | is this for the job with updated dependencies? | 08:31 |
| juergbi | I thought this was an issue only with coverage 5 and/or python 3.8 | 08:31 |
| tristan | Like for example, sometimes a test fails, and you look in the log, and the exception which your test expects is actually printed there in the stderr | 08:31 |
| benschubert | tristan: which build? | 08:32 |
| tristan | But cython doesnt say: "Here is the exception we *would* have raised, if you had declared this cdef function without a return value, so we're just printing it to stderr instead" | 08:32 |
| tristan | sec | 08:32 |
| tristan | benschubert, essentially INTERNALERROR> coverage.misc.CoverageException: Conflicting file tracer name for '/builds/BuildStream/buildstream/src/buildstream/_variables.pyx': 'Cython.Coverage.Plugin' vs '' | 08:32 |
| benschubert | Ah yeah | 08:32 |
| benschubert | odn't update to coverage 5 | 08:33 |
| tristan | benschubert, I didn't, this is in CI | 08:33 |
| benschubert | link? | 08:33 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/pipelines/165483205 | 08:34 |
| tristan | I may have messes something up but I don't think I've changed setup.py or tox.ini or setup.cfg | 08:34 |
| tristan | benschubert, would be cool by the way if you can benchmark the changes I threw up over the weekend | 08:35 |
| benschubert | it's coverage 4.4 indeed, I have no idea why it's failing | 08:35 |
| tristan | benschubert, tristan/partial-variables-manual-string-join (tip: 87added5f and before tip: 1757a42d4) | 08:36 |
| tristan | those appear to make a change in profiling | 08:36 |
| tristan | essentially the dent we discussed on friday | 08:36 |
| tristan | caching the array and struct-izing ResolutionStep | 08:37 |
| benschubert | tristan: let me start a benchmakr | 08:41 |
| tristan | I'll try to figure it out, should be something silly | 08:41 |
| tristan | benschubert, beware of raising exceptions and return values in cython btw | 08:42 |
| tristan | https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#error-return-values | 08:43 |
| * tristan got bitten hard | 08:43 | |
| benschubert | I know :D | 08:43 |
| tristan | they should really prefix the error | 08:43 |
| tristan | I'll file an issue against cython | 08:43 |
| tristan | "This is the error which was not raised: {}".format(exception) | 08:43 |
| tristan | otherwise you have no hint to go by | 08:44 |
| *** santi has joined #buildstream | 08:50 | |
| benschubert | tristan: https://gitlab.com/snippets/1995316 | 09:03 |
| benschubert | tristan: also, for show only it's quite straightforward to run it yourself: git clone https://gitlab.com/Buildstream/benchmarking/bst-benchmarks && cd bst-benchmarks; # replace https://gitlab.com/BuildStream/benchmarking/bst-benchmarks/-/blob/master/benchmark.py#L35 byt "origin/juerg/bst2" and then you can run: tox -- --config normal --show-only your_branch_or_commit | 09:05 |
| tristan | benschubert, ah thanks !\ | 12:29 |
| tristan | I can do them myself then | 12:30 |
| tristan | benschubert, starting/continuing to think this is a huge wasted time sink, may be better off axing this whole variables journey and living with the recursive algo | 12:31 |
| benschubert | Yeah it might be better for now at least | 12:33 |
| benschubert | keeping some of the documentation/refactor you did in the early rework | 12:34 |
| benschubert | that helped a lot :) | 12:34 |
| tristan | I don't think there is a performant version of the early rework at all, as it immediately aimed to remove `raise RecursionError` | 12:42 |
| tristan | But I can add some renames/docs along with valentind's original patch reworked a bit | 12:42 |
| tristan | benschubert, if you do find time to give the branch a lookover (the code, not the history, that's too much fiddling), and if you can spot anything obvious along the lines of "we're doing more work", I'd love to circle back and nail it | 12:44 |
| tristan | not seeing it myself | 12:44 |
| benschubert | tristan: Sure, I've got other cats to handle right now that sadly require way more focus than I'd like but when I have my brain more free I'll do that :) | 12:45 |
| tristan | I'm not sure also that it's not worth landing at some point, I have a feeling that resolving variables is not the bottleneck of loading 6K elements, and the recursion-proofing can be worth it for an already reasonably performant code path | 12:45 |
| tristan | Yes of course | 12:46 |
| benschubert | tristan: Agreed, I'd like to test it on some internal projects first to see if I can see a difference, I do need to do a few changes first though :) | 12:46 |
| tristan | Interestingly the last two benchmarks do show some improvement, and prove that allocating structs manually is measurably more performant than instantiating PyObjects | 12:48 |
| tristan | valentind, are you around ? I'm wondering if you can help me reason about https://gitlab.com/BuildStream/buildstream/-/issues/1359 | 12:49 |
| tristan | valentind, specifically the two stages of the load process | 12:50 |
| valentind | I am around | 12:53 |
| tristan | I'm confused about when there is an ElementFactory available | 12:56 |
| valentind | I would like to see the junction. | 12:56 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_loader/loader.py#L396 | 12:56 |
| tristan | See the junction ? | 12:57 |
| tristan | So at this point, we do some conditional resolving of options for a junction, otherwise we ensure fully loaded | 12:57 |
| tristan | and then we proceed to create the LoadElement | 12:57 |
| tristan | Sec, lemme look where we instantiate junctions | 12:58 |
| valentind | Note that Project.create_element calls the right factory depending on whether it is marked as first or main pass. | 12:58 |
| tristan | Yeah, that's where I don't have any factories in either case available | 12:59 |
| tristan | And I presume it's my fault a load of commits ago when I introduced `link` element | 12:59 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_loader/loadelement.pyx#L127 | 13:00 |
| tristan | valentind, instantiating the LoadElement for a link results in instantiating the `link` element itself | 13:01 |
| tristan | which "worked", until proven to not work | 13:01 |
| valentind | Not sure what link does exactly. But if you want it to be alias a junction it has to behave like one. | 13:01 |
| tristan | Right, it's an alias for anything, most usefully a junction | 13:01 |
| tristan | So where do we instantiate junctions ? | 13:01 |
| tristan | in _get_loader() I see | 13:02 |
| valentind | So the few places where have '== "junction"' we should probably do something. | 13:02 |
| tristan | Element._new_from_meta() | 13:02 |
| tristan | I think the only meaningful place is in _load_file_no_deps() | 13:03 |
| tristan | Aha | 13:04 |
| valentind | in metalement.py also | 13:04 |
| valentind | setting self.is_junction | 13:05 |
| valentind | And in _frontend/widget.py | 13:05 |
| valentind | Ah no, not the last one. | 13:05 |
| valentind | Because it would never be a link element. | 13:05 |
| tristan | So I think I found that something here is circular | 13:05 |
| valentind | Directly. | 13:05 |
| tristan | If we look at the stack trace on https://gitlab.com/BuildStream/buildstream/-/issues/1359, and concentrate on this area: https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_loader/loader.py#L396 | 13:06 |
| valentind | Also metaelement.py is probably not important either. Might never go there. So you are right, I think that place you showed in loader.py. | 13:06 |
| tristan | We can see that what's happening is that we first try to load an element, and we call self._includes.process(node) | 13:07 |
| tristan | self._includes.process(node) results in trying to load the link, because the problem occurs when performing an include across a linked junction | 13:08 |
| tristan | ensure_fully_loaded() would have given us a factory though, I think | 13:09 |
| tristan | Ok I think I'm on the right track, I thought you might have an obvious comment about why it was a terrible idea to instantiate an Element from the LoadElement constructor and tell me how to do it properly :) | 13:10 |
| tristan | But otherwise I think I got it :) | 13:10 |
| douglaswinship | Has anyone used the "--force" option in bst artifact checkout? I think it might be broken. | 13:11 |
| valentind | In ensure_fully_loaded, we should move "self._fully_loaded = True" to the end. | 13:11 |
| valentind | And assert when we are re entrant. | 13:12 |
| tristan | benschubert, shoot, the circular reference error reports are something I don't want to live without :'( | 13:14 |
| tristan | valentind, interesting, I just tried that and it incidentally reports a circular reference hehe | 13:14 |
| benschubert | tristan: right | 13:14 |
| tristan | valentind, maybe by fixing that properly first I will be able to resolve the rest | 13:15 |
| valentind | I think that would be helpful yes. | 13:15 |
| tristan | yeah, my circular reference is: https://gitlab.com/snippets/1995390 | 13:17 |
| tristan | Which shows the same provenance of the include | 13:17 |
| tristan | project.conf [include line]: circular reference loading subsubproject.bst... already searching for subsubproject.bst at [same line] | 13:18 |
| tristan | valentind, When you say we should assert not reentrant, you think we should remove the early return for an assertion ? | 13:32 |
| tristan | I dont know | 13:32 |
| tristan | Ah, no I get it | 13:33 |
| tristan | We want to be tolerant of that, but we don't want it to be triggered by the same function | 13:33 |
| tristan | ok that helps catch the error a bit earlier indeed | 13:35 |
| valentind | We should set make sure for example that "self.__is_being_fully_loaded" is False. And then set it right away to true. | 13:43 |
| valentind | That will detect cycles. | 13:43 |
| valentind | partially_loaded is for something else I think. | 13:44 |
| tristan | valentind, that's exactly what I did :) | 13:45 |
| tristan | trying to figure out how this runs now, with a dash of tracing | 13:45 |
| tristan | We load the toplevel target -> This causes the toplevel project to get "fully loaded" -> This triggers an include processing of the toplevel project at the beginning of _load_second_pass() | 13:48 |
| tristan | -> This include processing includes something across a linked junction boundary `(@): subsubproject-link.bst:file.yml` | 13:49 |
| tristan | by processing the first component of that include path, which is a link, we wind up back in _load_file_no_deps() calling self.project.ensure_fully_loaded() again | 13:50 |
| tristan | I suppose it makes sense for options to be resolved "weakly" when processing a link | 13:51 |
| tristan | like we do for junctions: `self._first_pass_options.process_node(node)`, however, that still leaves us without an ElementFactory at this stage | 13:52 |
| tristan | valentind, I think we need to move self._load_element_factories() out of the generalized Project._load_pass() | 13:54 |
| tristan | valentind, it looks like much of the _load_pass() body would want the options resolved before hand, but I don't know if that is valid for loading the element factories | 13:54 |
| valentind | What if you configure plugins in an included file? | 13:55 |
| tristan | Are we allowed to load different plugin types based on conditionals in the plugin origins section depending on options ? | 13:55 |
| tristan | Hmmmm | 13:55 |
| tristan | Ok so I guess it would be ideal to not do that :-S | 13:56 |
| tristan | I mean, I think we don't want/need factories to be conditional, but includes could be valid | 13:56 |
| tristan | maybe the include processing should be on demand for that node ? | 13:56 |
| tristan | There is a simpler, but more brute force approach | 13:57 |
| tristan | Which is, yucky | 13:58 |
| tristan | valentind, Never instantiate link elements | 13:58 |
| tristan | Oh no shouldn't do that I guess | 13:58 |
| tristan | Need to have link's resolve variables and conditional statements like any other element | 13:59 |
| tristan | this is a trickier problem than I expected | 13:59 |
| * tristan adds test case to !1993 for #1359 | 14:04 | |
| valentind | tristan, well, that depends, do you want to be able to include through a link element? | 14:04 |
| tristan | valentind, Yes | 14:05 |
| valentind | Well, then they need to have the same restrictions as junctions. | 14:06 |
| tristan | definitely a valid use case, but... the more I think about this, the more I think that; if it works for a junction it should also work for a link | 14:06 |
| tristan | Exactly | 14:06 |
| tristan | valentind, problem is the code flow, junctions get instantiated elsewhere | 14:06 |
| tristan | Lemme double check, but this is what happened last week | 14:07 |
| tristan | I changed the line to shallow resolve to `if kind in ("junction", "link")`, and there was still no factory in sight | 14:07 |
| tristan | ahhhh | 14:08 |
| tristan | of course | 14:08 |
| tristan | valentind, The element also decides not to fully load if it's a junction | 14:09 |
| * tristan missed that part | 14:09 | |
| tristan | ok so we can solve it that way | 14:09 |
| * tristan wonders what kind of `if meta.is_junction` conditions we have around, and if we can change code from "If (you_are_a_pony) { I decide to stroke your fur as a special case }" to: "If (you_want_to_be_stroked) { I shall stoke you thusly }" | 14:10 | |
| tristan | i.e.: 's/is_junction/requires_fully_loaded_project/g' would be more sensible ? | 14:11 |
| valentind | I do not remember. | 14:12 |
| tristan | nope | 14:13 |
| tristan | There are a ton of conditions all different | 14:13 |
| tristan | (all in element.py) | 14:13 |
| tristan | maybe rename it to something like `is_virtual` | 14:13 |
| tristan | All of the cases for is_junction appear to make sense for a link as well | 14:14 |
| tristan | Hmmmmmm | 14:26 |
| tristan | Infinite recursion loop | 14:26 |
| tristan | sorry infinite *include* recursion loop | 14:26 |
| tristan | (caught by valentind's reentrancy guard, though) | 14:26 |
| tristan | Ok, got a fix | 14:40 |
| tristan | valentind, we cannot move `self._fully_loaded = True` to the bottom of ensure_fully_loaded(), we need to explicitly avoid reentrancy introduced from _load_second_pass() | 14:41 |
| tristan | while the added check avoids self reentrancy from the same project | 14:41 |
| tristan | Let's see how this goes: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1993 | 14:47 |
| tristan | valentind, I may rename is_junction there, but I'd appreciate you gave that MR a look please :) | 14:48 |
| tristan | I don't know if all the other tests will pass but this one for #1359 appears to be fixed by treating links like junctions | 14:48 |
| valentind | tristan, that looks fine. But I do not understand why self._fully_loaded = True cannot be moved at the end of the function. | 15:12 |
| tristan | valentind, So there is a reason which flows much like the arrows before; i.e. processing nodes triggers loads; I bet this is the same for straight junctions as it is for links | 15:45 |
| valentind | I think I would need to spend time to understand what is happening there. | 15:46 |
| tristan | https://gitlab.com/snippets/1995440 | 15:47 |
| tristan | This is some trace I added to loader.py and _project.py | 15:47 |
| tristan | basically shows what's happening when things are successful | 15:48 |
| tristan | ensure_fully_loaded needs the if at the beginning to cancel out recursive reentry | 15:48 |
| *** hasebastian has quit IRC | 15:49 | |
| tristan | The case where fully_loaded is set at the very end: https://gitlab.com/snippets/1995443 | 15:51 |
| tristan | Right | 15:52 |
| tristan | valentind basically fully loading the thing causes one to descend into junctions, because of cross junction includes | 15:52 |
| tristan | valentind, and then fully loading a project ensures the invariant that it's parent project is fully loaded, bubbling back up | 15:53 |
| tristan | valentind, it's quite simple when you look at it, but I think it's important that I add a comment to that part | 15:53 |
| tristan | valentind, commented version: https://gitlab.com/snippets/1995446 | 15:56 |
| tristan | I'll repost a patch tomorrow with something along those lines included | 15:56 |
| *** santi has quit IRC | 17:32 | |
| *** santi has joined #buildstream | 17:36 | |
| tristan | douglaswinship, to fix the `format-check` error in https://gitlab.com/BuildStream/buildstream/-/pipelines/166012901, you just need to run `tox -e format` and squash the result into your commit | 18:03 |
| tristan | that picky linter is a result of adopting `black` for formatting consistency | 18:03 |
| nanonyme | tristan, I'm curious as to what the exact rules are. Would it have been okay with parameters starting from previous line and ) not being on a separate line? | 19:18 |
| *** benschubert has quit IRC | 19:41 | |
| *** santi has quit IRC | 21:24 | |
| *** xjuan_ has joined #buildstream | 22:43 | |
| *** toscalix has joined #buildstream | 22:50 | |
| *** toscalix has joined #buildstream | 23:14 | |
| *** toscalix has quit IRC | 23:17 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!