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`

--- 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):

@@ 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):

-  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):

-    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):

@@ -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.