5 ms·

Tangent rant. I'm skimming over some of the code at https://github.com/openai/gpt-2/blob/master/src/model.py https://github.com/openai/gpt-2/blob/master/src/mo

Tangent rant. I'm skimming over some of the code at https://github.com/openai/gpt-2/blob/master/src/model.py https://github.com/openai/gpt-2/blob/master/src/model.py and I can't help but feel frustrated at how unreadable this stuff is. 1. Why is it acceptable to have single-letter variable names everywhere? 2. There's little to almost no documentation in the code itself. It's unclear what the parameters of any given function mean. 3. There are magic constants everywhere. 4. Function names are so terse... ("gelu", "attn")

slimsag 7y ago
In my experience, this is the norm in the ML scene. Giant globs of unreadable and in no way understandable code -- unless of course you already understand everything. I think this is because the "product" so to speak is often the papers themselves, not the code, but I'm not sure.
fermenflo 7y ago
I agree, a lot of the code could be improved. But some of what you mentioned is fairly standard. Like "Gaussian Error Linear Units being GELU, w/b for weights/biases, etc...
- jsinai 7y ago
  Not sure how standard that is ...
  - steve_musk 7y ago
    It’s very standard ML abbreviations.
high_derivative 7y ago
My professional observation (as ml researcher at big tech): These companies hire a lot of engineers straight out of undergrad/master's degrees. The interviews test leetcode knowledge, and today lots of degrees are heavy on Python-scripted ML homework. The result is companies with billion dollar funding and world-changing goals having a lot of their code look like complete spaghetti. And this is the engineers who are meant to clean up research scientist code. Scientists generally don't feel like it's their responsibility to write strong code. Systems-side teams/orgs have better code, but essentially as soon as you enter the 'ml engineer/research engineer/research scientist' layer, it's doomed.
- gerash 7y ago
  I don't know why you excluded PhDs but their code aren't any better necessarily. Hiring straight from school doesn't necessarily mean bad S/W engineering skills. But generally the research code is either made hastily during exploration or by people not having enough Software Engineering background. Many research scientist I've seen at large tech companies don't have good CS background and write code that's either unreadable, unscalable, unmaintainable, or buggy at times. They often have good ideas but don't do much software design. Having been both at infrastructure teams and research teams, there are certain individuals in research orgs joining straight from school who think they are responsible with coming with a new and sexy thing and other engineers are responsible to run in production. It's like computing eigenvectors in Matlab or Python over a toy dataset and thinking you've done the bulk of the work of computing PageRank in production and should receive all the credit for a full search engine. That attitude is a red flag to me.
  - high_derivative 7y ago
    I don't see how what you are saying is contradicting me. The point is that the scientists generally don't view it as their job, whereas the ml engineers/research engineers typically have it as their job to do the software architecture/engineering side.
- codingslave 7y ago
  Yeah I'm waiting for the backlash when companies realize this
- gdb 7y ago
  (I work at OpenAI. Before that, I worked at Stripe. I've spent most of my software career thinking about how to build effective engineering cultures.) I think this code is actually well-written and maintainable. This is proven in practice because we've adopted it many places in OpenAI, and I've personally found it very easy to adapt to other use-cases (certainly much more so than the from-scratch Transformer implementations I've written!). As https://news.ycombinator.com/item?id=21456605 https://news.ycombinator.com/item?id=21456605 points out, the complexity of the code arises from the complexity of the underlying algorithm. Complexity due to software engineering concerns, like Tensorflow scopes, are elegantly handled. [edited for clarity:] Writing a Transformer in 174 lines of code requires a lot of deep thinking about the right underlying abstractions. > but essentially as soon as you enter the 'ml engineer/research engineer/research scientist' layer, it's doomed. We actually don't do this! Our only official technical title is "member of technical staff". (People sometimes choose to self-identify as an engineer or researcher, so you might see that on LinkedIn, but we don't have a distinction internally.) Everyone is responsible for their own code, and people care quite a bit about writing code that others can build on.
bredren 7y ago
Could these functions just be implementations of math with matching variable names?
TTPrograms 7y ago
It's a specification of essentially a complex graph of mathematical operations. If there's a function called def mult(a,b): return a*b it's not much more informative to write: def mult(activation_a, activation_b): return activation_a*activation_b Many of these functions are not much more complex than that, and the names along with their comments are more than sufficient given familiarity with the literature. If you think familiarity with the literature is unreasonable, it's still not clear what could improve code like this in reasonable space. "This is a linear function, which means that it satisfies f(x+a)=f(x)+f(a)"? "This is the attention head, it acts as a mask on the sequence input"? It would be like complaining that someone made a tree class and didn't put a comment explaining what a leaf node is. Code readability always assumes some reader context and minimum pre-existing knowledge (as do all forms of technical communication).
- macawfish 7y ago
  When math is involved, it's much easier to read code with short variable and function names.
JoeMayoBot 7y ago
Having worked with math/research folks in the past, this isn't surprising. That said, from a software engineering perspective, where a typical code review would identify this, it is immediately noticeable.
WnZ39p0Dgydaz1 7y ago
I actually disagree with you here. I don't think the code is unreadable, it follows standard notation used in Machine Learning. If you read scientific papers you will notice that e.g. variable names are the same as those used in mathematical formulas that everyone in the field is familiar with. The same goes for parameters, function names, and so on. They are standard notation/naming and only look confusing to people outside of the ML field. Giving them long uncommon names would actually be more confusing. As someone with experience in ML research I think this code is quite well written compared to what you typically see (a single function with hundreds of lines and dozens of if statements). I can immediately see what any of the functions does, and I haven't even read the paper.
buboard 7y ago
because a lot of it is meant to correspond to math equations so variables names like w, u, v , b ,g match the equations in the papers ? I actually think it's pretty readable, as long as you know what it i supposed to implement (i don't; but i imagine they are implementing a complex graph), and short names help figure out where things go in and out in one screenfull. Complex graphs are literally a spaggeti of arrows, and this format actually is pretty readable (even though in pytorch it would be more readable). I guess they leave comments out because it's not really possible to understand each line on its own (unless it's an implementation detail); you have to read the paper to know what s going on
make3 7y ago
I understand what you mean but please understand that this code is targeted at people which would at least have some background knowledge, like having read the seminal Transformer paper, "Attention Is All You Need", https://arxiv.org/abs/1706.03762 https://arxiv.org/abs/1706.03762 Most of the code becomes really straightforward once you have. A lot of the magic constants are the result of multi page proofs (like the GELU constant) that would be impractical to put in the code. Deep learning research really is a field that requires some amount of knowledge, and it's normal that you don't automatically understand state of the art code. Here is the GPT2 paper https://d4mucfpksywv.cloudfront.net/better-language-models/language_models_are_unsupervised_multitask_learners.pdf https://d4mucfpksywv.cloudfront.net/better-language-models/l...
moultano 7y ago
The notation in the code will be very familiar to anyone comfortable with the underlying research and math. The "conceptual" documentation is in the literature. What you're asking for is the rough equivalent of asking a C programmer to name their loop variables "index" instead of "i." Everyone familiar with the concepts of c programming knows what "i" means in the context of a for loop. Similarly, everyone familiar with transformers knows what "gelu" and "attn" mean.
- randomsearch 7y ago
  This isn’t a good comparison. “i” is used domain independently across an entire language, not in some other domain. In fact, it’s used across the entirety of computer science (and originated in maths), so it’s across an entire discipline and even inter-disciplinary. They should use proper variable names if they want to have the code understood by anyone non-specialist, and by people who use different terminology, and people looking back at the code in the future when terminology may have changed. I don’t know about this domain, but the single-letter-variable name etc AKA “match the equation” is a curse when non-CS engineers/scientists write code. It often breaks code conventions, leaving IDEs to light up like a Christmas tree when opening the source. There’s a good reason CS moved from register letters to something closer to natural language.
  - moultano 7y ago
    >I don’t know about this domain So then why don't you believe me when I tell you that all of these variable names are extremely standard, and will be familiar to anyone who has written deep learning code before?
    - tommit 7y ago
      I feel like you both have good points. Yes, a lot of the variables are very ML specific and often called that way. However, I feel like that encourages the same researchers (who are obviously not software engineers) to give the rest of their variables sub-par names as well. Why would you give any variable a name longer than a word even, if so many you regularly encounter are just `w`, `u`, `x`, `hparam` ... and so on. I'm a software engineer with a background in ML, so even though I somewhat know the domain language I still get mad at the blatant disrespect for PEP-8. That being said, this one is definitely one of the better codebases I have come across. This feels like it could be fairly easily worked with and understood. I have seen far, far worse code to go along research papers.
m463 7y ago
Mirrors my thoughts regarding all math textbooks and published papers. I remember reading a famous scientist (newton maybe) published a really accessible book on a subject, which was read by lots of lay persons and opened him up to lots of unwanted public attention. So publishing in a more inscrutable way might be a way of assuring peer-to-peer communication. Either that, or it's a labor of love where cleaning things up would detract from the forward momentum.

slimsag 7y ago

In my experience, this is the norm in the ML scene. Giant globs of unreadable and in no way understandable code -- unless of course you already understand everything. I think this is because the "product" so to speak is often the papers themselves, not the code, but I'm not sure.

fermenflo 7y ago

I agree, a lot of the code could be improved. But some of what you mentioned is fairly standard. Like "Gaussian Error Linear Units being GELU, w/b for weights/biases, etc...

jsinai 7y ago
Not sure how standard that is ...
- steve_musk 7y ago
  It’s very standard ML abbreviations.

high_derivative 7y ago

My professional observation (as ml researcher at big tech): These companies hire a lot of engineers straight out of undergrad/master's degrees. The interviews test leetcode knowledge, and today lots of degrees are heavy on Python-scripted ML homework. The result is companies with billion dollar funding and world-changing goals having a lot of their code look like complete spaghetti. And this is the engineers who are meant to clean up research scientist code. Scientists generally don't feel like it's their responsibility to write strong code. Systems-side teams/orgs have better code, but essentially as soon as you enter the 'ml engineer/research engineer/research scientist' layer, it's doomed.

gerash 7y ago
I don't know why you excluded PhDs but their code aren't any better necessarily. Hiring straight from school doesn't necessarily mean bad S/W engineering skills. But generally the research code is either made hastily during exploration or by people not having enough Software Engineering background. Many research scientist I've seen at large tech companies don't have good CS background and write code that's either unreadable, unscalable, unmaintainable, or buggy at times. They often have good ideas but don't do much software design. Having been both at infrastructure teams and research teams, there are certain individuals in research orgs joining straight from school who think they are responsible with coming with a new and sexy thing and other engineers are responsible to run in production. It's like computing eigenvectors in Matlab or Python over a toy dataset and thinking you've done the bulk of the work of computing PageRank in production and should receive all the credit for a full search engine. That attitude is a red flag to me.
- high_derivative 7y ago
  I don't see how what you are saying is contradicting me. The point is that the scientists generally don't view it as their job, whereas the ml engineers/research engineers typically have it as their job to do the software architecture/engineering side.
codingslave 7y ago
Yeah I'm waiting for the backlash when companies realize this
gdb 7y ago
(I work at OpenAI. Before that, I worked at Stripe. I've spent most of my software career thinking about how to build effective engineering cultures.) I think this code is actually well-written and maintainable. This is proven in practice because we've adopted it many places in OpenAI, and I've personally found it very easy to adapt to other use-cases (certainly much more so than the from-scratch Transformer implementations I've written!). As https://news.ycombinator.com/item?id=21456605 https://news.ycombinator.com/item?id=21456605 points out, the complexity of the code arises from the complexity of the underlying algorithm. Complexity due to software engineering concerns, like Tensorflow scopes, are elegantly handled. [edited for clarity:] Writing a Transformer in 174 lines of code requires a lot of deep thinking about the right underlying abstractions. > but essentially as soon as you enter the 'ml engineer/research engineer/research scientist' layer, it's doomed. We actually don't do this! Our only official technical title is "member of technical staff". (People sometimes choose to self-identify as an engineer or researcher, so you might see that on LinkedIn, but we don't have a distinction internally.) Everyone is responsible for their own code, and people care quite a bit about writing code that others can build on.

bredren 7y ago

Could these functions just be implementations of math with matching variable names?

TTPrograms 7y ago

It's a specification of essentially a complex graph of mathematical operations. If there's a function called def mult(a,b): return a*b it's not much more informative to write: def mult(activation_a, activation_b): return activation_a*activation_b Many of these functions are not much more complex than that, and the names along with their comments are more than sufficient given familiarity with the literature. If you think familiarity with the literature is unreasonable, it's still not clear what could improve code like this in reasonable space. "This is a linear function, which means that it satisfies f(x+a)=f(x)+f(a)"? "This is the attention head, it acts as a mask on the sequence input"? It would be like complaining that someone made a tree class and didn't put a comment explaining what a leaf node is. Code readability always assumes some reader context and minimum pre-existing knowledge (as do all forms of technical communication).

macawfish 7y ago
When math is involved, it's much easier to read code with short variable and function names.

JoeMayoBot 7y ago

Having worked with math/research folks in the past, this isn't surprising. That said, from a software engineering perspective, where a typical code review would identify this, it is immediately noticeable.

WnZ39p0Dgydaz1 7y ago

I actually disagree with you here. I don't think the code is unreadable, it follows standard notation used in Machine Learning. If you read scientific papers you will notice that e.g. variable names are the same as those used in mathematical formulas that everyone in the field is familiar with. The same goes for parameters, function names, and so on. They are standard notation/naming and only look confusing to people outside of the ML field. Giving them long uncommon names would actually be more confusing. As someone with experience in ML research I think this code is quite well written compared to what you typically see (a single function with hundreds of lines and dozens of if statements). I can immediately see what any of the functions does, and I haven't even read the paper.

buboard 7y ago

because a lot of it is meant to correspond to math equations so variables names like w, u, v , b ,g match the equations in the papers ? I actually think it's pretty readable, as long as you know what it i supposed to implement (i don't; but i imagine they are implementing a complex graph), and short names help figure out where things go in and out in one screenfull. Complex graphs are literally a spaggeti of arrows, and this format actually is pretty readable (even though in pytorch it would be more readable). I guess they leave comments out because it's not really possible to understand each line on its own (unless it's an implementation detail); you have to read the paper to know what s going on

make3 7y ago

I understand what you mean but please understand that this code is targeted at people which would at least have some background knowledge, like having read the seminal Transformer paper, "Attention Is All You Need", https://arxiv.org/abs/1706.03762 https://arxiv.org/abs/1706.03762 Most of the code becomes really straightforward once you have. A lot of the magic constants are the result of multi page proofs (like the GELU constant) that would be impractical to put in the code. Deep learning research really is a field that requires some amount of knowledge, and it's normal that you don't automatically understand state of the art code. Here is the GPT2 paper https://d4mucfpksywv.cloudfront.net/better-language-models/language_models_are_unsupervised_multitask_learners.pdf https://d4mucfpksywv.cloudfront.net/better-language-models/l...

moultano 7y ago

The notation in the code will be very familiar to anyone comfortable with the underlying research and math. The "conceptual" documentation is in the literature. What you're asking for is the rough equivalent of asking a C programmer to name their loop variables "index" instead of "i." Everyone familiar with the concepts of c programming knows what "i" means in the context of a for loop. Similarly, everyone familiar with transformers knows what "gelu" and "attn" mean.

randomsearch 7y ago
This isn’t a good comparison. “i” is used domain independently across an entire language, not in some other domain. In fact, it’s used across the entirety of computer science (and originated in maths), so it’s across an entire discipline and even inter-disciplinary. They should use proper variable names if they want to have the code understood by anyone non-specialist, and by people who use different terminology, and people looking back at the code in the future when terminology may have changed. I don’t know about this domain, but the single-letter-variable name etc AKA “match the equation” is a curse when non-CS engineers/scientists write code. It often breaks code conventions, leaving IDEs to light up like a Christmas tree when opening the source. There’s a good reason CS moved from register letters to something closer to natural language.
- moultano 7y ago
  >I don’t know about this domain So then why don't you believe me when I tell you that all of these variable names are extremely standard, and will be familiar to anyone who has written deep learning code before?
  - tommit 7y ago
    I feel like you both have good points. Yes, a lot of the variables are very ML specific and often called that way. However, I feel like that encourages the same researchers (who are obviously not software engineers) to give the rest of their variables sub-par names as well. Why would you give any variable a name longer than a word even, if so many you regularly encounter are just `w`, `u`, `x`, `hparam` ... and so on. I'm a software engineer with a background in ML, so even though I somewhat know the domain language I still get mad at the blatant disrespect for PEP-8. That being said, this one is definitely one of the better codebases I have come across. This feels like it could be fairly easily worked with and understood. I have seen far, far worse code to go along research papers.

m463 7y ago

Mirrors my thoughts regarding all math textbooks and published papers. I remember reading a famous scientist (newton maybe) published a really accessible book on a subject, which was read by lots of lay persons and opened him up to lots of unwanted public attention. So publishing in a more inscrutable way might be a way of assuring peer-to-peer communication. Either that, or it's a labor of love where cleaning things up would detract from the forward momentum.