YAPF config

[wookayin]

I'm modifying the acme codebase externally, but the YAPF auto-formatter I'm using keeps producing different formatting results, which is frustrating for me.

I am using yapf 0.32.0 (latest as of today), with the following .style.yapf file:

[style]
based_on_style: yapf
indent_width: 2

It would be great if ACME can add .style.yapf in the OSS repository. What yapf config exactly is being used internally?

I came across dm-haiku's style.yapf file: https://github.com/deepmind/dm-haiku/blob/main/.style.yapf but this is not compatible with the latest yapf as the style preset chromium is gone and renamed to yapf. I tried that one with chromium -> yapf name changes, but the resulting style is still very different.

[nikolamomchev]

I'm not familiar with YAPF myself, but looking at what other OSS projects from DeepMind and Google Research do, I'd suggest trying something like this:

[style]
based_on_style: yapf

Let us know if it works for. you and we can add it.

[nikolamomchev]

Actually, this should be equivalent to the config you proposed. Do you have examples on how formatting differs from what we use in Acme? From all I can find, the config you showed should be equivalent with what we use.

[wookayin]

So nearly all files are affected by yapf. I believe it is very easy to reproduce: just clone a OSS acme repository and run yapf:

$ yapf -ir acme/
$ git status | grep modified | wc -l
436

With the following yapf.config file:

# Uses Google-style: e.g., indent width 4 -> 2
[style]
based_on_style: yapf
indent_width: 2
#split_before_named_assigns: True
#column_limit: 80

Some trivial examples are, like acme/adders/__init__.py, where the blank line between the LICENSE headers and docstring:

Example: `acme/adders/__init__.py`

```diff --- acme/adders/__init__.py +++ acme/adders/__init__.py @@ -11,7 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - """Adders for sending data from actors to replay buffers.""" # pylint: disable=unused-import ```

This is a mismatch happened due to that the yapf-formatted codebase are exported to OSS via copybara, but from OSS contributors' perspective it can be a bit annoying as they would need to discard this additional hunk when submitting a patch. I am not sure where there is an option for yapf to suppress this.

But more importantly, there are significant differences in formatting style in many files (around 100 ones).

A file that seems not sanitized by yapf (e.g., acme/wrappers/video.py):

```diff @@ class MujocoVideoWrapper(VideoWrapper): # physics.model.ncam frames, and render all of them on a grid. num_cameras = physics.model.ncam num_columns = int(np.ceil(np.sqrt(num_cameras))) - num_rows = int(np.ceil(float(num_cameras)/num_columns)) + num_rows = int(np.ceil(float(num_cameras) / num_columns)) height = self._height width = self._width # Make a black canvas. - frame = np.zeros((num_rows*height, num_columns*width, 3), dtype=np.uint8) + frame = np.zeros((num_rows * height, num_columns * width, 3), + dtype=np.uint8) ```

Function/Method params (e.g., acme/agents/jax/actors.py):

```diff - def __init__( - self, - actor: actor_core.ActorCore[actor_core.State, actor_core.Extras], - random_key: network_lib.PRNGKey, - variable_client: Optional[variable_utils.VariableClient], - adder: Optional[adders.Adder] = None, - jit: bool = True, - backend: Optional[str] = 'cpu', - per_episode_update: bool = False - ): + def __init__(self, + actor: actor_core.ActorCore[actor_core.State, actor_core.Extras], + random_key: network_lib.PRNGKey, + variable_client: Optional[variable_utils.VariableClient], + adder: Optional[adders.Adder] = None, + jit: bool = True, + backend: Optional[str] = 'cpu', + per_episode_update: bool = False): """Initializes a feed forward actor. ```

Column width issues (e.g., acme/agents/jax/ail/gail_agents.py, L53):

```diff - ppo_agent = ppo.PPOBuilder( - config.ppo_config, logger_fn=logger_fn) + ppo_agent = ppo.PPOBuilder(config.ppo_config, logger_fn=logger_fn) ```

Split on function calls (after parenthesis) (e.g., acme/agents/jax/sac/learning.py):

```diff @@ -128,16 +125,15 @@ class SACLearner(acme.Learner): return q_loss def actor_loss(policy_params: networks_lib.Params, - q_params: networks_lib.Params, - alpha: jnp.ndarray, + q_params: networks_lib.Params, alpha: jnp.ndarray, transitions: types.Transition, key: networks_lib.PRNGKey) -> jnp.ndarray: - dist_params = networks.policy_network.apply( - policy_params, transitions.observation) + dist_params = networks.policy_network.apply(policy_params, + transitions.observation) action = networks.sample(dist_params, key) log_prob = networks.log_prob(dist_params, action) - q_action = networks.q_network.apply( - q_params, transitions.observation, action) + q_action = networks.q_network.apply(q_params, transitions.observation, + action) min_q = jnp.min(q_action, axis=-1) actor_loss = alpha * log_prob - min_q return jnp.mean(actor_loss) @@ -259,8 +255,8 @@ class SACLearner(acme.Learner): key=key) if adaptive_entropy_coefficient: - state = state._replace(alpha_optimizer_state=alpha_optimizer_state, - alpha_params=log_alpha) + state = state._replace( + alpha_optimizer_state=alpha_optimizer_state, alpha_params=log_alpha) return state # Create initial state. ```

Please click to expand to see some examples of diff outputs.

What I'd like to suggest is to run a pass of yapf throughout the entire codebase to fix inconsistent formatting. Or this could be just due to a discrepancy between internal and external yapf tooling/configuration. This will greatly help OSS contributors to make sure the formatting they would use will be compliant with what is being used internally.

[nikolamomchev]

Thanks for sharing those. The changes you show are something that our internal formatter does as well. I think this confirms that the YAPF config is the correct one.

I agree that we may want to be more strictly complying to the YAPF formatter. We will discuss auto-formatting the whole codebase but note that this is an invasive change. E.g. we are currently doing a series of non-trivial refactoring which touch many files. Re-formatting those in-between will cause a ton of merge conflicts.

FWIW, our internal workflow discourages auto-formatting the whole file on a change. Instead, the recommendation is to only auto-format the affected lines in the file. Is it an option for you to adopt a similar workflow?

[wookayin]

Thanks for confirming. I hope the entire codebase can be reformatted properly soon, to reduce such overhead, but I understand that there can be some difficulties in regard to potential conflicts.

Many of the tools available (editors, IDEs, foramtters, ...) outside Google does auto-formatting the entire file rather than affected lines. YAPF, for example, supports -l range flag but the IDE/formatter has to specify the range being affected whenever it triggers auto-formatting. Ideally auto-formatting only the affected lines should be technically possible, but it would require some non-trivial workflow/dev tools setup for many of the users including me.