Antipatterns in open source research code with Jariullah Safi

Key Takeaways

so yeah so this is about anti-patterns and it's targeted at open-sourced ml research code and and i know that's very um it's a very narrow view at it a lot of these things that i'm going to talk about are actually anti-patterns in other pieces of code as well if you just look at open source code or just in general if you work at a programming company you're probably going to see you're probably going to see a lot of these a lot of this stuff pop up but in my experience it shows up sort of with a with an increased frequency in code that's open source by researchers particularly phd students and you know i i want to start this with a with a very big very important disclaimer is that no matter how sort of quote-unquote ugly somebody's code might be it is still worth releasing and this is this is a worry that came up a lot when i first sort of presented uh these anti-patterns in the reddit thread that lavonia mentioned is a lot of people were like well you know we don't want to create a situation where somebody would feel like their code would be judged or they might not release their code because they want to take the time to clean it up and so i want to make a big disclaimer here like release code no matter how janky had no matter how unclean is always always always better than unreleased code having said that there is there's a number of things that can be done to make the code better without additional effort and one of the arguments that i'm going to try to make that i i fear this presentation might not support as well as i could in the future is that uh trying to avoid some of these empty patterns would also make your research life a little bit easier and the life of those that that come after you and maybe inherit your code base and go from there um this presentation is a bit like this drawing uh because i sort of ran out of time i got slammed with work so bear with me here a little bit about myself uh i work at symbio robotics i do computer vision for them a lot of deep learning work uh and a lot of the a lot of the stuff that i'm gonna talk about today is sort of coming from adapting open source code basis for use in symbiotics and and sort of seeing these patterns repeatedly uh as mentioned before i also are on the youtube channel uh it's mostly about things like python and and deep learning and most recently i've been doing a series of streams on reinforcement learning and then uh if you want you can follow me on twitter and i'll combine tensorflow compilation blows with memes on there so i'm gonna try to cover four anti-patterns uh this was supposed to be five any patterns and i ran out of time so it's four now and uh in order these are monolithic configuration objects and we'll sort of discuss what exactly that means in a second but not just having monolithic configuration objects but also nesting them deep inside your code this is the actual anti-pattern not just having configuration objects that's fine but nesting it deep inside your code is the anti-pattern i've also seen people use arc parse for configuration management in lieu of a configuration file and so that's also something that i'm going to discuss and what are some of the alternatives and what's what's wrong with that um there's also this notion that because you know ultimately you're trying to run your code or give somebody to run your code so you make a command line interface to make it easy but then the code is only really accessible from that command line interface so that's another andy pattern that i see a lot and i'm going to discuss that and then sometimes people will will write a a function that is that takes in file names as inputs and then outputs uh some data in some output file and that's it that doesn't actually return anything and then people adapt that code and they start using it that way so they just sort of look for the file that should be created and so that's another anti-pattern that i that i'm going to discuss um as discussed there was a fairly robust discussion on reddit about these anti-patterns so if anybody's interested just sort of search this and read it and you'll you'll find the thread there's a lot of opinions there and the very first any pattern that i'm going to talk about the monolithic configuration object also seemed to be the most contentious so overall it's it's generally a good idea to encapsulate the the various parameters of your machine learning model or your simulation or whatever you're doing into a configuration file configuration files themselves are really really good uh yaml makes them also very readable um i'm not a huge fan of yaml in general but for for this purpose i think yaml is a very good fit because you know they sort of read off you can sort of just look at them and go okay i understand what these things mean um and and what they're supposed to be and then you can parse them really easily so having a configuration file even if it's a very large configuration file is a great idea and it also makes it very easy for somebody who's reading your code to sort of look at the file and go hey hello i heard somebody said something yeah we can hear you okay somebody but okay it's fine um so as i was saying having a configuration file is uh is a great idea however actually there's not a however yet um usually what people will do then after that the sort of the the lowest common denominator way of using them is that you can just say hey open this file and pass it through a yaml parser and then you can you can access it uh like you access a field from a dictionary so the pros here are this is very easy and that's why you also see this usually most often the cons are there's no validation here so if your configuration needs to be some minimum set of fields and parameters nothing in your code here is able to verify that and because of that if somebody's trying to adapt your code or maybe somebody maybe you made a new configuration and you forgot a field you won't find out about it until you try to use it which potentially can be much later in the run and so that can be very painful um this is also immutable may accidentally be changed in the code and accidental mutation is is is unfortunate and then there's uh intentional mutation where people uh will change the the parameters or the the fields of this uh dictionary or add or subtract fields from it in the code that's just devious and i would say like i know that feels easy sometimes to just pass a dictionary to a function and have it uh get a new a new field when it comes back uh but but please don't do it it's tracking that can be very very difficult especially as your code base becomes more bigger it's also a bit clunky to reference this is a this is really a personal taste thing but in in a dictionary in python you have to like write it in as uh bracket access with you know passing the field as the field name as a string and it's a little bit a little bit tiresome and then if you're working in jupiter or if you're working in a text editor usually you don't get uh or at least immediately you don't get auto completion for these because you know it has no way of knowing what's in the dictionary so those are some of the cons for this in practice the way i see this used is and i apologize for these slides they're a little bit busy but i'll try to go over them in detail in practice i usually see them used like this where you have somebody has a main function they're opening the configuration yaml uh they're getting the object and then they're just passing it straight through to a bunch of functions so in this case the configuration is being passed to make model make model uses a field from that but then also calls make model base and passes the configuration there here there's also a mutation instead of having some way of setting a um a default for this uh the default is is just none and the default is set in the code uh and then you know there's a bunch more code here and we have no idea how it uses configuration just by reading this function um and anyways so then this returns a model and then yet again we do the same thing passive to train model and so on and so forth so you know these functions make optimizer construct loss function there's there's not a lot that we can learn about what they're doing or what fields they're using by just looking at this we kind of have to dig into the code i really dig into the code to figure out what are the um what are the dependencies of make optimizer what are the dependencies of construct loss function even this function train epic is uh you know you can kind of see oh train epic is supposed to train for one epic and it takes in the model and the optimizer and the loss function but then all of a sudden there's this this configuration what what is it using from here is there like uh something to do with loss function or learning rate decay that's in there or what's going on probably it's using the learning rate but it's not clear so that's one of the biggest cons that i i would say of this approach is what parameters are important to any of these functions and also any function can silently change this configuration and we'd never know and that can potentially become problematic if you have if that introduces a silent bug and you kind of don't know where it's coming from it is very easy to adapt and in the reddit thread and in other conversations that i had with people this was the the biggest pro that everybody presented for this big enough to where all of these other problems were were worth it um which is that if make optimizer needed two or three additional parameters now you can just add them to the configuration file and you don't have to change the function call at all you don't have to change trained model you don't have to change make optimizer et cetera and i think that's a valid uh argument for research code but at the same time like i said at some point one of these problems especially the one where silent bugs can introduce is gonna outweigh this and i'll talk in a second about how some of the pain of refactoring can be made easier with modern tools so three main problems in summary there's three main problems there's clunky access to the configuration fields there is mutability and then function dependencies become very unclear so how can we make it better um so as i said before i think a more reasonable usage for any kind of configuration is to not pass the entire configuration object wherever possible so in this case you know train model now takes um the model as an input and from configuration it takes learning rates so now it's incredibly clear what's important train model and if somebody's trying to adapt this code or just understand what it's doing before you know they use it in their paper uh it's a lot more straightforward for them to understand what each of these models are what each of these functions actually hold uh as a importance this dictionary itself is still mutable but at the very least it's not mutable by these nested functions at least in this example it's also clear what the functions take as parameters but as i said before this may not scale to more parameters like if uh save model now takes like two or three additional parameters for whatever reason uh this is not gonna necessarily it it gets a little bit more more difficult to do that so let's talk about the refactoring then that argument that you know if make model base is going to change i don't want to have to change this line uh and yes the left side that i'm showing here is definitely easier to change than the right side when you introduce new parameters that the function needs to depend on this is strictly true but the latter is only slightly more work if you become best friends with your editor so here i'm showing an example of pycharm which is a fairly popular ide for python and the example here the gif of what it's showing is that you had a function that had one argument and then the ide sort of helps you figure out where this function is being used and then you can add more fields to it and maybe give them default values or not however you want to do it and it changes the it changes the usage of it all throughout your code here it's in only in one file but in pycharm this will happen throughout your code base for that like semantically for that specific function and so this is a very powerful tool so anybody that uses pycharm you know you already have access to this uh and so if we make best friends with our editors then this this argument becomes a lot less weighty um i don't personally use uh pycharm i use uh sometimes vs code and sometimes emacs and pretty much all text editors have some uh some facility for you where you can search for a particular pattern uh throughout a code base and then and then make changes to that pattern uh and so with the varying degrees of uh of uh finesse you can do this in pretty much any text editor as as well and if you wanted to you can do this from bash as well if you're really interested in that um so that to me like if you uh if you make your functions so that they accept only their dependencies as different arguments uh then this this point three gets solved and you know there's i'm sure a lot of people are still going to be very resistant to this uh and may not see the benefit of it but at the very least i've shown a uh a potential way out where the the burden of maintaining a good api is not is not as much as it seemed to be so moving on uh before i tackle these these other two points i kind of have to move on to point two some of these kind of get muddled and and bleed into one another and there's there's good reason for that so the second one uh is about using arc parse as configuration management and so to to set the stage this is actually from a an actual repo and i've tried to hide the identity of the person who wrote this but the you know he's he's a well-respected ai researcher anyway so it's it's fine even if somebody recognizes who this is from um arc parse i've seen this pattern a fair amount where art process uses a configuration management system and what that ultimately means is that people will have uh in in the best cases they'll have some sort of a function that parses all these arguments for you uh in the worst case this would just be straight up out in in the main script and what they're doing is they're using this add argument as a way to define the parameters by using the default value right and the nice thing here is that first of all this gives you dot access so as soon as you have the parsed args you can use them with it with with the dot operator so you get rx.seed and etc and then the way you've put it together you might actually also get autocomplete here because your editor can now figure out what the arguments are supposed to be by by following the you know argument parser object and so you get dot access you probably get autocomplete and you get an obvious command line interface from this you know as soon as you've implemented this you get a command line interface for free so what are the problems well imagine this getard's function the the repo that i took this from this was like 300 lines and those 300 lines are just encoding something that could be as simple as like a 15 20 line yaml file because they're just encoding the configuration so the configuration becomes very difficult to understand at a single glance the second problem here is all changes that you would make to how you're running this particular file or how you're running this particular script they only happen through the command line interface now so if you're using a tool like uh weights and biases i know there's there's other ways to lock this configuration in weights and biases but you know if having a configuration file that maybe gets committed with your code or you know you can sort of make copies of that helps a lot with reproducibility of what you're doing whereas this and then uh you know combining that with uh command line uh sort of just a call to this file isn't necessarily very reproducible because you might forget what command you used you have to sort of go through your bash history and whatnot uh on top of that i'm reasonably sure i didn't quite confirm this but i'm pretty sure this is still mutable those those are some of the cons of doing this um i am going to just assert this i i think i've shown enough of reason to do this but i think that when choosing between our parse and configuration files you should prefer configuration files and i'll show some alternatives where you can get some of the same kind of benefits of arc parse but with configuration files so one of the ways that you can gain dot access back for your arbitrary configuration dictionaries is by just using weights and biases uh so when you run the init in weights and biases you can actually pass it uh oh this the code here is wrong but you can pass it the the configuration dictionary you can also pass it the uh arguments from from arc parse and then once you've done this you can you can take the return value of the inet and either look at the config field or the config static field uh the the only difference here is that if you get config it will be mutable but if you get config static it'll be immutable and there's a disclaimer here determined programmers in python always always find a way to make something that is immutable mutable uh so you know if if somebody like that is using your code then you don't have a prayer but most of the times that's not going to be the case um so yeah so you can then set config equals to this config static and now you'll have something that has uh dot access to all your fields and you also have optional immutability i don't think that you get autocomplete in your editor but i'm not entirely sure i i doubt you would uh the other potential way that you can get access back is my personal favorite this is what i like to do and that's maybe you know i can see this being potentially more work but you also get data validation for free which is which can save a lot of time and headache and then you also get optional mutability and you definitely get autocomplete and everything so this library called pedantic and what pedantic does is you can you can take this base model class from them and then if you subclass that you can write your configuration very very easily in this sort of data class-like fashion in python and you can specify what the what the types of these are supposed to be and uh then if you add this config class underneath it you can you can set this parameter uh for allowing mutation and so now i'm showing on the right you can just take your configuration dictionary and pass it to the to the initializer of this configuration object by just using this double star operator which is a standard python way of mapping a dictionary to a bunch of arguments and when you do this it will validate both the the the fields inside your configuration make sure they all exist and then it will also validate their um their types up to a point and so you get pretty much all the benefits with an approach like this and then on top of that you get you get free autocomplete so if you if somebody's interested in sort of cleaning out their configurations a little bit uh give a look to pedantic one of the cool things also is that you can you can create an empty object once you've written out your configuration you can create an empty version of this and then you can actually uh dump it out to a dictionary or dump it out to a yaml from this object directly so that that way you can create your empty configuration and then fill it out and go from there okay so going back to the issues that we talked about um for the first point uh now we've sort of presented alternatives to all of these and and what what are some of the ways that you can get around it so moving forward let's talk about the uh the any pattern that i mentioned where uh code when you have a situation where code is only accessible through a command line interface so what that what i mean by that uh is and i don't know maybe nobody here actually does this but i have definitely seen this uh in a number of repos where you have some entry point script right so in this case this is the file.pi or just main.pi and somebody will be parsing the arguments up top and uh maybe loading some configuration uh same as you know the first any pattern and then you just sort of start doing all the work so you take the input file name from the configuration do a bunch of different steps and then take the result file name from the configuration and save it out so the only pro here is that you are now set up to be called from a command line and there's a lot of cons so first off it's very hard to tell in this file what is going on it's very hard to tell what the dependencies of these functions are various functions are and then your main entry point just sort of does like is all of your your your main code um there's no abstraction so this is sort of the only way that you can you can go through and and and use these functions uh and then inputs and outputs are files i'll talk in a second about why that's a problem uh this becomes very difficult to adapt into some other code uh some of it is just because all the cons that i mentioned with the configuration being passed everywhere and then some of it is with the fact that you know especially when the when these kind of files get like really complicated it's very hard to tell when something starts and when something ends uh this might have multiple sub steps that make that a little bit more confusing or sometimes if else statements and so a lot of these make you know sort of create more mental burden on the person that's that's reading or adapting the code so a slightly better version of this is uh you start introducing some functions so now i can say hey i'm going to separate it out into a function and that function takes in an input file name and result file name and the configuration and then it's sort of doing the same kind of things but now it's an it's a separate function on its own and it makes a little bit more sense now you can see where the business logic is and you can see where the argument parsing is and then finally where the function is being called so it's a little bit less hard to understand what's going on here we can make this the whole thing a little bit more usable by changing and refactoring the code a little bit so that now the the first step so in the past two examples this first step function was accepting an input file name so it's it wasn't clear what the what the ultimate use here was but now what we can do is we can make it so that it accepts an image rather than the file name of an image and then abstract this whole thing out into a separate function that takes in an image as an input and then outputs the whatever the output of this is which presumably is its text i just sort of made it up on the fly so let's say it's text and then we can create a second function which is specifically for exposing your command line interface and in that function you can accept a file name because from a command line that's easier thing to pass and then as well as an output file name and now it's incredibly clear you can see okay the only thing that this function needed was the image so you can read the image and then you can open uh you know the output file name as a normal text file and write to it and so the whole thing is a little bit more straightforward now and if now if somebody wants to use this function the do the thing function uh it's it's easier for them they don't necessarily have to rely on whatever library you are using in first step to to load the image uh they don't have to rely on the fact that you would be saving this maybe they just wanted to commit it to you know some some uh some cloud bucket or something like that so it's just it it's somewhat more clear and then now you have a library function and so it's a lot easier to adapt to somebody else's code and it was a very small change in in the grand scheme of things this was supposed to have an image that it does not currently have it's the day of technical difficulties it's okay um i've messed up this slide so we'll just you're doing great what happened we'll just pretend that this light never happened and we'll go to the next one so removing inputs and outputs from the configurations the last thing and this is sort of a me putting my my own personal sort of touch on this but if i were writing this then i would not put the image file name and the output file name in the configuration and this goes back to you know like the the actual meat of your run the its own configuration should not really include what data you were fitting you know if you're because most of the times this is usually given for inference or something like that where you're just trying to get the output for some for some image the the input there should not really depend on your configuration the configuration it makes more sense for it to be separate and then your command line interface can expose you know what the input image is supposed to be and where the output is supposed to be saved so this is not so much you know like i think it's fine if people uh prefer putting the uh the input sort of folder and the output folder or something like that in the configuration but if i if i were approaching making this i would i would make those separate command line parameters it just makes it a little bit easier and then for processing different files or different uh you know different sets of data from the command line you don't have to keep changing the configuration so that's that's the only change here and it's uh easier with with a bold face now to adapt to somebody else's code uh the actual command line call if you see on the bottom becomes a little bit longer but i think that's to the benefit not necessarily to the to the detriment a final bit of uh advice here is i really don't like uh arc parse in general uh and the reason for that is you know arc parse is a great great bit of a great module uh it's very it was very helpful very useful in my in my early career but i started seeing these patterns with arc parse and sometimes the unintentionally the way a library is put together becomes uh its own worst enemy so in in the case of arc parse it kind of encourages even without intel without intending this it kind of encourages behavior like this where you just have uh arc parse in the global scope of a file and you know people use the the resulting object is in in weird ways so instead i i prefer you know anytime all of my all of my engineers as well as people that i that i ever advise i always tell them to use something like arg or there's a number of other repo libraries like that and what what arg does is uh it doesn't it basically makes it so that you can take any function and you can expose it directly to the command line so long as it's using uh input types that can be represented in the command line so in this case we just took the do the thing cli function and if you pass it through this function called dispatch command it just immediately turns this file into something that you can access from the command line in the same way that you were doing it before so it's usually a lot less code to do this it's a lot cleaner uh and there's uh there's a few more things that you can do with it if you want to give more help messages you can add a decorator here but the best thing that in my opinion arg does is that it forces people to write their codes as functions and i think that has sort of a hidden benefit that goes a long way okay so the fourth anti-pattern is implicit intra-process communication via files now there was some confusion about this in the reddit thread i don't mean this as saying like hey caching to a file is bad caching is a different story what i'm referring to specifically is if we go back to that intermediate form when we were sort of working through this code and making it better you had we had this function called do the thing and it takes as an input an input file name as an output a result file name it doesn't return anything if this is your function if you ship code like this and this is the the the function that you had as sort of the main entry point and it's not uh calling some like sub function like we set up later what's going to happen is that some poor downstream researcher is going to use it like this where they're going to write their own function then they're going to call this function with their input file name which they may have actually saved in their own function prior to to this file name and then they're going to take whatever this output file name was supposed to be they're going to open it read it and then do the rest of the thing that they were trying to do and i've seen this multiple times and you know nest this kind of approach two or three times as more and more people adapt that code and you you get into such a hairy mess uh where it's it becomes very hard to tell what function is doing what and what files is touching where it's saving them uh and yeah so just please don't do this um it's it's very easy to take a function like this and just separate it out into a sub function that is not relying on file names at all it's just sort of taking in something from memory and returning something in memory uh anything like that becomes a lot cleaner to use and a lot more easier to adapt and just like the sopranos this uh presentation is going to come to an abrupt ending because i i kind of ran out of time um so that that's that's what i got um what if you use a config.pi and put all the config variables there and access them from that file uh and something like import config i think if you have a really really good reason for it like like i said in my production stack i do have a really good reason for it which is that my configuration is dynamically constructed uh based on some other parameters that come from environment variables that i don't have any control over so in that case i think it makes a lot of sense but in in the examples that i was giving in in open source code or in situations where your configuration represents things like uh learning rates and stuff like that that people might want to change to iterate over i think it's better to have a configuration file because you can always copy uh and paste a configuration file somewhere else and and make a new one and and track those in version control whereas with the config pi uh the burden of doing that becomes a lot more