IRC logs for #buildstream for Thursday, 2019-06-20

*** tristan has quit IRC05:32
*** tristan has joined #buildstream05:45
*** persia has quit IRC05:57
*** persia has joined #buildstream05:58
*** samkirkham has joined #buildstream07:29
*** bochecha has joined #buildstream07:44
*** adds68 has joined #buildstream08:03
*** toscalix has joined #buildstream08:11
*** bochecha has quit IRC08:11
*** bochecha has joined #buildstream08:11
*** raoul_ has joined #buildstream08:27
*** phildawson has joined #buildstream08:29
*** ikerperez has joined #buildstream08:36
*** benschubert has joined #buildstream08:40
*** Becky has joined #buildstream08:43
juergbitristan: I've been working on moving subproject fetching to Stream as preparation for casd support08:50
juergbidoing that I've noticed that there seems to be an inconsistency in master with regards to the decision whether subprojects should be fetched08:51
juergbiStream enables this for commands such as build, fetch, and a few others, but not for commands such as show, as far as I can tell08:52
juergbihowever, subproject fetching seems to be always enabled as part of include processing (for include directives that use junctions)08:52
juergbiI assume this is not intentional08:53
juergbiI'd suggest to move the decision whether to fetch subprojects or not to the frontend08:53
juergbiand then apply this also to include directive processing08:53
juergbidoes this sound reasonable?08:55
juergbithis would also allow CLI options to override the default, if we want that08:55
benschubertjuergbi: would it be possible to use the '--fetchers = 0' to disable fetching? That would seem the most straightforwards to me, and thus, always fetch is fetchers > 0 and we need to fetch?08:58
juergbibenschubert: this would mean that by default we would implicitly fetch (subprojects) even for bst show09:00
juergbiI think we wanted to avoid that09:00
*** phildawson_ has joined #buildstream09:01
juergbiusing --fetchers=0 to disable fetching for other commands might make sense, though09:01
benschubertI remember it going forwards and backwards on it, definitely, but I must admit, as a user, I'm always annoyed when bst fails and tells me to fetch first, when it knows exactly what to do to execute the command I asked :)09:01
*** phildawson has quit IRC09:01
juergbiyes, I can understand that. I'm not sure what's best overall. however, I'd like to first focus on fixing the internals to be at least consistent per command09:03
juergbichanging defaults would probably require more discussion but could be done later09:03
*** jonathanmaw has joined #buildstream09:04
benschubertjuergbi: sure, let's fix things and once it settles, let's review all the defaults before 2.0 :)09:04
juergbi+109:05
juergbicould potentially also be something for user config. however, we probably want to avoid adding too many knobs09:05
*** ChanServ sets mode: +o tristan09:10
tristanjuergbi, Not intentional... sounds sensible... reading rest of backscroll09:10
tristanOk so... I think that definitely if --fetchers is 0, we should handle that case everywhere gracefully09:11
tristanAnd return with an error message, instead of running an infinite scheduler that doesnt queue any jobs09:11
tristanAlso agree that the decision of "whether to automatically fetch subprojects" is a different decision09:12
tristanHowever... juergbi... do you remember off hand why we have that as optional at all ?09:12
tristanI think that subprojects are source like anything else, why would we want that to be optional ?09:12
tristanAlso... *if* it is going to remain optional, I certainly think it should have user config... as a user I don't want to tell bst on the command line every time, if my preference is not the default09:13
tristanjuergbi, unless I'm missing something though, I really think it aught to not be optional at all09:14
tristanbenschubert, any thoughts why we would want fetching of subprojects to be optional ?09:14
benschuberttristan: I cannot remember either, maybe cs-shadow might remember?09:15
tristanIf it were not optional, and were logically treated as any other source of any other element (which I think it is), then we can *still* disable it with `--fetchers 0`09:15
tristanI mean it really is a source of a junction element after all09:15
juergbitristan: it's not optional from the user/CLI point of view right now09:15
tristanjuergbi, Curiously, does your branch use the scheduler to run the fetches ?09:16
tristan(/me hopes...)09:16
juergbiit's enabled for some commands but not for others09:16
tristanjuergbi, Yes I understand this part... I am largely in favor of treating junction sources in the same way as every other element's sources from a user perspective09:16
juergbithe issue is that subproject fetching may be required as part of project/element loading, so commands that normally don't fetch anything might require fetching (or bailing out)09:16
tristanand removing the knob completely09:16
tristanRight09:17
juergbiI have a WIP branch to use the scheduler for the subproject fetching, yes :)09:17
tristanBut those commands *actually* require fetching09:17
juergbi(probably not ideal yet, but hopefully going in the right direction)09:17
tristanjuergbi, I.e. `bst show` does not normally require fetching any more than `bst build` normally requires fetching09:17
tristanbst build only requires fetching if the sources are not already present09:18
tristanin exactly the same way that bst show only requires fetching if it's sources (the subprojects) are not already present09:18
tristanjuergbi, YAY \o/ (for using the scheduler)09:18
juergbiI think the argument was that bst show is not expected to start a scheduler session / take a long time09:18
juergbiI thought you were actually arguing for avoiding implicit fetching for bst show09:18
juergbibut I might misremember09:18
tristanIt may be indeed09:19
tristanI could very well have argued that... but this was in a time when junctions were brand new :)09:19
juergbiI don't have a strong opinion on this at the moment09:19
juergbias mentioned above, I want to fix the internals first before worrying about changing defaults09:19
juergbias this blocks casd09:20
tristanI don't understand how this blocks casd, but I'm thankful that it does !09:20
tristanhaha09:20
juergbiunless we can all agree on a change without further discussion09:20
juergbicasd means that we need to use grpc for importing blobs into CAS, that includes source cache09:20
juergbiwe must avoid using grpc in the main process09:20
tristanI feel like, it might be the same cost to you (or even less), if we can right now agree that optionality of fetching of subprojects can rest in peace09:21
juergbiso scheduler it is :)09:21
juergbiyes, I'd be happy to just always fetch subprojects09:21
tristanjuergbi, I would suggest this brutish move: Write an email to the list, explaining that junction sources are the same as sources of any other element, and if you really want to avoid fetching them, you can specify `--fetchers 0` anyway, just like any other source09:22
tristanjuergbi, and say "Unless there are any objections in the next few days... this optionality is disappearing, speak now or forever hold your peace !"09:22
tristanhaha09:22
juergbihehe09:22
juergbifair enough09:22
*** benbrown has joined #buildstream09:23
tristanjuergbi, I *think* that a side effect of this is that `--fetchers 0` currently doesnt do what we would like it to do... but I think that that is safely orthogonal from your branch09:23
tristanIt should be easy enough to get a better failure mode for a `--fetchers 0` or `--builders 0` or whichever scenario09:24
juergbiyes, probably have to improve the UX around that09:24
tristanas a separate patch09:24
juergbithat sounds good09:24
tristanbut anyone can do that09:24
tristanjuergbi, Your using the scheduler... I expect will "mostly just work", you will probably run into a little trouble in the App side of things, due to it's expectations and notions of "sessions" and such09:25
tristanBut that is all really easy stuff to shuffle around09:25
juergbiyes, I suspect that as well. I actually haven't manually tested it yet, only via pytest, so not sure how bad the UX is right now09:25
tristanI think there are some callbacks from Stream about when the scheduler starts and ends which the frontend uses to print summaries09:26
juergbiwill look into that when the basics are in place09:26
tristanand that's probably all that conflicts with running the scheduler multiple times09:26
*** raoul__ has joined #buildstream09:38
*** raoul_ has quit IRC09:39
*** raoul__ has quit IRC09:44
*** lachlan has joined #buildstream09:47
*** raoul__ has joined #buildstream09:48
*** tpollard has quit IRC09:50
*** tpollard has joined #buildstream09:50
*** lachlan has quit IRC09:59
juergbijonathanmaw: "This job is stuck because you don't have any active runners online with any of these tags assigned to them: wsl"10:02
juergbiknown problem?10:02
jonathanmawthe machine seems to have locked up10:05
jonathanmawI'll sort it out10:05
*** lachlan has joined #buildstream10:06
juergbita10:21
*** raoul__ has quit IRC10:26
gitlab-br-botBenjaminSchubert opened (was WIP) MR !1401 (bschubert/node-api-noget->bschubert/new-node-api: introduce a Node api, and remove node_get) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/140110:42
benschuberttristan, juergbi would you have time to have a look at this part of the rework for the new node API? It gets rid of 'node_get'. Once we have an agreement on this part, I'll attack the rest10:43
*** CTtpollard has joined #buildstream10:47
*** tpollard has quit IRC10:48
tristanbenschubert, No replacement for Plugin.node_get_project_path() ?10:49
tristanI'll try to take a more thorough look later... kind of in the middle of testing my initrd10:51
tristanbenschubert, I'm also curious, do the docstrings work well with sphinx, will we get the new Node APIs nicely formatted in our API docs ?10:51
tristanhaving that linked all together well will be an attractive part of this change10:52
tristanPlugin.configure() takes a `Node`, click on `Node` in the docs brings you to the docs in how to get data from it10:52
tristanregarding the docstrings comment... I was referring to cython (forgot to mention that)10:57
benschuberttristan: I was planning in tackling documentation in a second phase before we merge to master, so that we are sure the API is ready to go10:57
benschubertoh, I would need to check this10:57
tristanSurely there would be a workaround but would be nice if it "just works" well :)10:58
benschubertagreed!10:58
*** bochecha has quit IRC10:58
*** bochecha has joined #buildstream10:59
benschubertAnd for node_get_project_path, I went under my radar, I'll have a look at it10:59
benschuberttristan: seems like this function actually does a lot of stuff, might be better to refactor later :)11:00
tristanYeah it polices that a path doesnt escape the project directory11:05
tristancause we expect project paths to be revisioned with the project, they cannot point to external things11:05
*** raoul__ has joined #buildstream11:05
tristanSo basically, the Node just requires knowledge of the related project and where that project is staged11:05
benschuberttristan: since that is quite tied to the project, I think it would be best to keep it there, in order to not move everything to 'node', and end up with a huge API surface there11:06
tristanWould it be a huge API surface ?11:07
*** bochecha has quit IRC11:07
*** bochecha has joined #buildstream11:07
tristanI thought it made good sense that the core hands over a Node to Plugin.configure(), and the plugin uses that API to load things in a safe way11:08
tristanThere may be other cases too11:08
tristanFor instance, expanding aliases11:08
benschubertI think 'Node's shouldn't know about the project at all, they are, after all, a node in a yaml structure, and doesn't really know if it's a path11:08
benschubertSo you would rather be leaning into moving more knowledge of variables, aliases, etc into nodes?11:08
tristanI'm not convinced, I mean I would lean towards a very clean and luxurious API surface for plugins every time11:09
tristanKeep that surface nice, even if it means a bit of contortion behind the scenes11:10
benschubertI would be leaning to11:11
benschubert1) Have a small but understandable and easy to work with Node api, with Provenance information errors handled for you11:11
benschubert2) Have helpers on the Plugin, to do things like getting a proper path from the root of the project or other things that require more knowledge of how BuildStream is architectured11:11
benschubertand have all the api in 2 work on Nodes from 111:12
tristanRight, we need to limit the knowledge a plugin needs to know about how BuildStream is architectured11:12
tristanIf we tell people: This is a Node, and go and fetch your config from it... they are unlikely to notice that there is a completely separate API in another location for policing project paths11:13
tristanAs a user of the API... I am likely to think: BuildStream gave me this Node in the first place... Why is it asking me to give it to this other API ?11:14
tristanShouldn't I be able to just ask the Node ?11:14
tristanHeh11:14
*** bochecha_ has joined #buildstream11:15
benschubertWe coudl also tell:11:15
benschubertThis is the plugins public API, it contains helper to ensure you access your configuration correctly.11:15
benschubertIf you want to access your configuration directly, you can access it through the Node's API11:15
tristanBut we sort of already have that with node_get()11:16
*** bochecha has quit IRC11:16
*** bochecha_ is now known as bochecha11:16
benschubertand explain that Node's API maps to the underlying yaml of their configuration11:16
benschubertwe already have part of it with node_get/set/del, but those operations become very painful and blur the limit between lists/dicts/etc and Nodes11:17
benschubertmaking it unclear what we are working with11:17
benschubertsuch that some code paths are sanitizing nodes multiple times in a row because we have no idea what is given to us11:17
tristanI think the current very painful part is *constructing* the nodes, as we need to do when dynamically setting public data, and I agree it would be nicer to have all of this on a separate Node interface11:18
tristanI don't think that it's nice to split the domain of "how to extract configuration data" into two separate locations, though11:18
tristanbenschubert, You are basically asking the plugin author to understand something about the core - to understand that different entities have different knowledge11:19
tristanI think it's more lightweight if that is assembled neatly in one place11:19
tristanAgain, this mostly amounts to *not* coding what makes sense because the implementation and underlying architecture dictates that it makes sense11:20
tristanand instead coding what makes sense in terms of convenience for the plugin author, which I think we should favor every time11:20
benschubertI would disagree with that, because we end up with many, subtly different, functions on Plugin, some referencing methods in Element, on how to handle the data.11:21
benschubertTherefore the current dataflow is almost impossible to grasp correctly. Having too many helper functions around an otherwise simple concept might make things11:21
benschubertharder to grasp11:21
benschubertI do understand that having everything in a single place would be nice11:22
benschubertbut I also do think that if users have a clear separation in mind between configuration data and project-aware data, it would make things simpler11:22
benschubertI am not talking about making the variables stuff public. That should be entirely hidden to users11:22
benschubertbut telling them "This is how you access your data" and "these are helpers when you need to access something in a context" would make sense I believe11:23
benschubertAlso, for node_get_project_path, renamign this to something like 'sanitize_path(Node)' would also make things more understandable11:24
benschubertIt also seems that 'node_get_project_path' would be the only method that would be contentious here, do you agree or do you see other ones?11:27
tristanI don't think 'sanitize' is really the right word here... rather we want to get a path for the purpose of loading data from inside the project11:27
tristanand we want to enforce that it's there (we don't transform the path I think... we police it here)11:28
tristanbenschubert, I didnt read through the patches but... substituting variables is indeed another thing11:28
tristan*actually*11:29
*** CTtpollard is now known as tpollard11:29
tristanbenschubert, I've been wondering if we should abolish completely the distinction between getting strings, and getting substituted strings11:29
tristanbenschubert, right now Sources don't have variable substitution capability, however it would be nice if they did, for one11:29
tristanAnd also... it would be simpler if extracted strings were just always substituted, and the plugin didnt have any choice11:30
tristanthat is a different change of course11:30
tristanbenschubert, there is also extraction of URLs11:30
tristanbenschubert, For instance... I think currently we have a *separate* API for substituting the URLs11:31
tristanwhich means the core does not have a focal point for raising a warning when a source tries to load an aliased URL but the project did not provide an alias ?11:31
tristanOr... that is not exactly true, I think we at least have this focal point11:31
tristanbut the plugin is forced to extract the string first and then do the aliasing separately11:32
benschubert(I'm off to eat, I'll answer after)11:32
tristanMe too !11:32
*** lachlan has quit IRC11:33
*** tristan has quit IRC11:37
*** bochecha_ has joined #buildstream11:39
*** bochecha has quit IRC11:41
*** bochecha_ is now known as bochecha11:41
*** tristan has joined #buildstream11:43
*** lachlan has joined #buildstream11:51
jennistristan, regarding the discussion we had the other day of caching/not caching junctioned elements, are we ok with implementing this with just "local elements only", rather than "local push only" and "local pull only"?11:54
jennisBut this boolean should be defined within "artifacts:" configuration, not just a project level setting11:55
jennis(This is what the latest branch accomplishes)11:55
*** lachlan has quit IRC11:57
*** raoul__ has quit IRC12:23
benschuberttristan: right, I'm not sure having a single point (the Node) for everything would be best, but we should at least not have more than 2. Same for substitution, that should be done by the plugin initialization, before we give the node to the plugin12:32
*** raoul__ has joined #buildstream12:37
benschuberthey raoul__ I think I adressed all your comments on https://gitlab.com/BuildStream/buildstream/merge_requests/1401, could you confirm? :)12:37
*** raoul__ is now known as raoul12:38
raoulShall have a look in a bit :)12:39
*** phildawson_ has quit IRC13:00
*** lachlan has joined #buildstream13:04
*** phildawson_ has joined #buildstream13:28
*** lachlan has quit IRC13:46
jjardonHi, what is the latest bst format version bst master supports?14:15
jjardonI think bst should fail if you try to build something = or < that version14:17
cs-shadowit's at 24 currently (https://gitlab.com/BuildStream/buildstream/blob/master/src/buildstream/_versions.py#L26)14:20
cs-shadowI thought the CLI already bailed out when the required format version was more recent than what the BuildStream package came with. Are you not seeing that behavior14:21
jjardoncs-shadow: does that mean current master supports 24 and all the previous versions?14:21
cs-shadowthat's the idea in general14:22
cs-shadowI don't think we've made any backwards-incompatible changes to the format, but I'm not 100% sure14:23
jjardoncs-shadow: ok, I will recheck  and come back; thanks!14:24
cs-shadowcool, np14:25
*** lachlan has joined #buildstream14:34
*** lachlan has quit IRC14:51
*** bochecha has quit IRC15:06
*** toscalix has quit IRC15:07
*** tpollard has quit IRC15:59
*** ChanServ sets mode: +o tristan16:44
tristanjjardon, master should probably reset to 0 and not care about format-version until 2.0 honestly, there is no relation between format versions of 1.x and 2.x16:44
tristanas cs-shadow says, there is no guarantee of compatibility, once 2.x exists we can identify what incompatibilities there are and create a porting guide16:45
tristanjjardon, fwiw, I've not made any effort *whatsoever* to bump format version for the addition of *any* YAML features in master since it has been decided that we would break API16:48
tristanand I don't think format version bumps have been happening there16:48
tristanAlso, I should write a patch which *removes* all of the "Since: 1.2" and "Since: 1.4" notations in all of the plugin facing python APIs16:49
tristansince they are no longer relevant, as bst2 is a new and separate API16:49
tristanthey will come back with "Since: 2.2" notations, in a couple of years or whenever the second stable release of the 2.x version comes along16:50
benschuberttristan: concerning our previous discussion, I would propose finishing the yaml rework, once we have an API, look how we can make the url/path handling neater and then land everything in one go, do you have any concern with such a plan?16:50
tristanbenschubert, I still didnt look deeply at the patch honestly, just got home from dinner in a distant neighborhood (2:00am here)16:51
persiaI have a concern with any plan that includes "land everything in one go" as having a potential to involve massive amounts of rebasing and rework.  If it can be broken into parts (and still meet others' concerns), I think that would be preferable.16:51
tristanbenschubert, the next thing I was going to look at was what the plugins are doing with the API, i.e. I hope that they are still one liners like they were with Plugin.node_get()16:52
benschubertoh sure, well, let me know if you are interested in looking at it and I'll wait before merging it to the other branch :)16:52
tristanand we don't have to like... "Get the integer node from the dict node, and get the integer value from the integer node" in separate steps16:52
benschuberttristan: sure, then let's get this rework done and then do a second phase to polish out the path/url part of it16:53
tristanpersia, This is an API change which deeply affects all existing plugins, hence it is desirable to land the API surface change once and reduce iterations and churn in external repos16:54
benschuberttristan: the way I was thinking about it, we have to do it, like 'node.get_scalar('key').as_int(), I think it's the most readable and less error prone16:54
persiatristan: Fair point :)16:54
tristanscalar16:54
tristansounds eccentric and esoteric16:54
tristanwhy not just get_int() ?16:54
tristananyway let me come back to it... I would really like it to be dead simple if possible... I also have an ingrained aversion to tail calling in general, but maybe that's just my personal matter of taste16:55
benschuberttristan: the advantage of it is that we could access both the node and/or the value in various part of the API, which, for example, allows us to trivially defer access to provenance information16:56
tristanin most cases, I avoid tail calling and write two lines of code, in the knowledge that one day in the future I might need to do something in between those two statements which got crammed into the same line of code16:57
tristanand rather not muck up history16:57
benschubertI can also add helpers on MappingNode/SequenceNode to get directly the correct value if needed, but I'd rather not have a too wide API surface, as it will not help users16:57
tristanyeah, I see that provenance being tied to the value and not the dict is valuable here, so probably the tail calling will become a thing and it makes sense16:58
benschubertAnyways, I'll let you review the code when you have time, and will continue the next steps in the meantime :)16:59
tristanI'll give it priority first thing in the morning... honestly I don't expect to have other criticisms than the one that I kind of feel like we shouldnt burden the plugin author with knowledge that accessors can come from multiple API surfaces17:01
tristanlike, Project and Context are not public, we manage all of that internally and give the plugin one surface17:01
tristanbut anyway, I'm not in a state to consider it deeply right now... the tail calling is probably right, lemme look at it in the morning :)17:02
benschubertsure, good night :D17:03
*** raoul has quit IRC17:54
*** jonathanmaw has quit IRC18:11
*** bochecha has joined #buildstream18:39
*** abderrahim has joined #buildstream20:35
*** abderrahim has quit IRC20:52
*** abderrahim has joined #buildstream20:53
*** bochecha has quit IRC21:24

Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!