RFC: ternary assignment style in transformers code revisited 
Author: stas00
Pull request: no

I run this by @patrickvonplaten and he encouraged I post it here, and he wrote a follow up which I will post next.

---------------------------

As `transformers`'s mandate is to be super user-friendly code-wise I wanted to ask whether very frequently used in the `transformers` code ternary assignment `a = x if z else y` actually supports that mandate.

It is used a lot:
```
grep -Ir if src/transformers | grep else | grep = | wc -l
1043
```

As I was just writing some code where I had:

```python
        if args.deepspeed:
            self.model = model.module
        else:
            self.model = model
```
I then rewrote it in the `transformers` style of:
```python
        self.model = model.module if args.deepspeed else model
```
and then I realized that my original code is a way more readable and my "internal" compiler instantly gets it and moves on, whereas the ternary style is super slow to ingest. It could be just me, for me vertical alignment helps reading code a lot!

Surely, that's 1 line vs 4. So each of them has their pros and cons.

I'm sure it won't be too hard to find much less readable nested ternary assignment code in `transformers`, e.g.:

```python
self.device = device if framework == "tf" else torch.device("cpu" if device < 0 else "cuda:{}".format(device))
```

as compared to rewriting it as:
```python
if framework == "tf":
    self.device = device 
elif device < 0:
    self.device = torch.device("cpu")
else:
    self.device = torch.device("cuda:{}".format(device))
```

- Does it take many more lines and fits less code into the screen - hell yeah
- Is it much more readable - IMHO absolutely! Especially due to the vertical alignment
- Is it less error-prone - very likely.

I'd have even split the `elif` to clearly see that a different group of conditionals is being tested in the second part, but that's just personal style.

Binary search always beats linear search, even for a small number of items.

Another advantage of unwrapped ternary op is very noticeable during interactive debug sessions - you can't easily break or step through such one-liners, especially when the juggled values aren't variables but function calls. 

I just find it somewhat inconsistent that this developer collective tries hard to avoid `map`, `filter` and `reduce` as more difficult to read, yet ternary style is used very often. To me the 3 mentioned operators are in the same category as ternary operators readability-wise since they require horizontal reading. That's just my perception of course.

This is not a critique but rather a question of whether this part of style is intentional or just came about because someone likes vertically compact code and is good at reading horizontal logic. At the end of the day, this is not a deal breaker, it just takes me much longer to get such code.

Thank you.

@LysandreJik, @sgugger 

Comments:
here is @patrickvonplaten's follow up posted with his permission (we initially did it over gist):

------------------


Thanks for the write-up!
I guess everybody has a slightly different opinion regarding ternary assignmens . My opinion is:

1) I would never write a if-elif-else statement in one line (I don't think we have many lines like this in transformers)
2) I do use the ternary assignment quite a lot, but only for IMO "simple" statements like:
```do_sample = do_sample if do_sample is not None else self.config.do_sample``` or 
```all_attentions = () if output_attentions else None```
I would also use it for your example above ``` self.model = model.module if args.deepspeed else model``` I guess, but I wouldn't use it for complexer statements, such as this one: https://github.com/huggingface/transformers/blob/61443cd7d917ef323a799ee27bb4abc4344f0d11/src/transformers/models/t5/modeling_t5.py#L887 *e.g.*.

3) I do look quite a bit into how the coding style is in the respective file. E.g. `generation_utils.py` has a slightly different style from the `modling_...py` files from `trainer.py` IMO -> so I try to adapt here. *E.g.* I think Quentin likes to use statements like ```dataset = self.manual_data or self.default_data``` in datasets which I then adapt to, but that's something we don't do in transformers (not really sure why)
4) I think code design also depends quite a bit on the coding environment someone has set up for him/herself. I'm using a rather special neovim+tmux+zsh setup, which I think shows the code differently to all the vscode users. *E.g.* I don't mind having long lines (>119) because it's nicely displayed in Vim for me, but Sylvain is not a big fan of it afaik.

But I'm happy to have some stricter rules on code design! We should probably include Sylvain and Lysandre then as well.

Some other conventions we could/should standardize:

- Only use f-strings => Sylvain really wants us to use f-strings and I also think that they are the nicest design
- stricter ordering of where docstring, helper functions and classes should be in the `modeling_....py` files
- I don't really like nested if-else statements. E.g. I prefer:
 ```python
 if a and b:
   # ...
elif a and not b:
   # ...
elif not a and b:
   # ...
else:
   # ...
```

very much over

```python
if a:
    if b:
       # ...
    else: 
       # ...
else:
     if b:
       # ...  
     else:
        # ...
```

- I don't like it when variable have one-letter names. I working quite heavily with search and search-and-replace patterns in vim to understand/refactor code and I think they are not very readable => so I think it's always better to have at least some what understandable variable names. *e.g.* even if everybody knows the q,k,v logic in Transformers IMO, `query_states`, `value_states` and `key_states` are better names.

I personally don't think the ternary style is harder to read when it's for a very simple condition, on the contrary. Like Patrick said, I would not use it for a `if-elif-else statement` as it then is harder to read and understand, but for something like 
```
        if args.deepspeed:
            self.model = model.module
        else:
            self.model = model
```
is typically the situation where I would encourage a ternary line. 

The only other situation I don't use them is if the formatter gets in the way because the line is long, as it's then clearer in the unrolled version.

> Another advantage of unwrapped ternary op is very noticeable during interactive debug sessions - you can't easily break or step through such one-liners

If only used for simple tests like I mentioned above, this should be a no-problem.

I agree with patrick all other comments (especially the f-strings! if there is one thing my brain has trouble parsing, it's the `.format(...)`. And I would extend one-letter variable names to non-standard abbreviations in general, as it makes the code harder to read for non-native English speakers.

However, fixing current files to fix those guidelines is not a priority IMO. I would put finishing the decoupling of the models (removing things like the `Summary` class in modeling_utils) above and it's already low priority for me. We're not many core maintainers and there is lots to do!

Thank you for sharing your preferences

Oh, in no way I was suggesting that we need to change anything. I'm just observing the readability impact for myself and was curious to hear whether others find it the same. But so far clearly it's not the case.

Having coded most of my programming life in Perl I guess I got used to aligning things vertically the way that made them most readable, since Perl has no indentation requirements, so I have always aligned assignments and branches for the fastest possible reading.

But, I have no problem with the ternary assignment since that's the style of this project and you seem to prefer it, and so that it's important to remain consistent.

I am totally with you on the f-strings, - I was trying to keep this focused to just one subject matter, but if you'd like to expand it to other style issues, we can easily do that.

We can also close it this rfc at any time, if you feel there is nothing else that needs to be said or done, as the two of you expressing that you like ternary style is sufficient to not needing to continue.


Thanks for bringing this issue! Like Sylvain, I personally don't think the ternary style is harder to read. As you've said @stas00, I think it depends on the language one is used to; since I like to say that Python can nearly be "read" like prose or natural language, this is the case where it shines:

The following statement is way closer to natural language
```py
self.model = model.module if args.deepspeed else model
```

than the following
```py
if args.deepspeed:
    self.model = model.module
else:
    self.model = model
```

even if the latter will probably be easier to read to users coming from different languages than Python.

Regarding the `map`/`filter` and other lambda methods, this is a very personal choice but they're (usually!) harder to read than the list/dict comprehensions that can replace. Once again, this is a very opinionated statement.

Thank you all for your feedback. 

It's loud and clear ternary ops are the norm at this project, with the recommendation to avoid nested ternary ops in the new future code.