[docs] Enforce use of Args: over Attrs: in class docstrings

[smackesey]

Summary & Motivation

Currently in the codebase, some classes use Attributes: and others use Args: to list class parameters in docstrings. While the semantics between these are technically different (in principle class attributes need not correspond to constructor params at all), in practice I believe that everywhere we use "Attributes" we are really documenting constructor params. In any case we have no need to directly document attributes since we expose them through @property.

"Attributes" and "Args" cases render quite differently (see screenshots). This PR:

• converts all existing uses of Attributes to Args (which renders more compactly and consistently with functions)
• adds a check to the API doc build that causes a failure if use of Attributes: is detected.

"Args" style (preferred, enforced by this PR):

"Attributes" style:

How I Tested These Changes

Built docs.

[smackesey]

• [docs] Enforce use of Args: over Attrs: in class docstrings #15404 👈
• @deprecated_param and @experimental_param decorators #15305 : 4 other dependent PRs (#15303 , #15310 , #15429 and 1 other)
• @deprecated decorator warnings and general annotations/warnings refactor #15302
• dagster-pandas deprecated event_metadata_fn removal #15304
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @smackesey and the rest of your teammates on Graphite

[github-actions]

Deploy preview for dagster ready!

Preview available at https://dagster-gm5422dvj-elementl.vercel.app

Direct link to changed pages:

[sryza]

I agree that the way that "Args" gets rendered looks a lot nicer. However, I think that "Attrs" is more representative of what we're trying to communicate, i.e. that these are the attributes of this class. I wonder if there's some way to address the rendering issues.

Also see: https://stackoverflow.com/questions/46569163/sphinx-document-an-attribute-that-is-same-as-a-parameter

[smackesey]

I agree that the way that "Args" gets rendered looks a lot nicer. However, I think that "Attrs" is more representative of what we're trying to communicate, i.e. that these are the attributes of this class. I wonder if there's some way to address the rendering issues.

Disagree that "Attrs" is more representative of what we're communicating, because the way most of our public classes are written we don't expose attributes-- we expose properties, which are documented separately. At the level of the class docstring itself, the public API we are documenting is the constructor parameters, not the attributes, which are typically private. Also, often the constructor parameters are going to differ from the attributes-- this happens whenever there is a deprecated param, but also in other cases where there is some logic in the constructor. It's clear that we need to document such deprecated constructor params but it would be incorrect to call them "attributes".

Now for NamedTuple classes, we actually do expose attributes via PublicAttr. Most of the time these are identical to the constructor params, but not always. To be technically correct we would have separate documentation of the constructor params and the attributes, but that seems like overkill when they are the same 95% of the time. There is probably something we can do in sphinx to solve this, I have to think about it.

[sryza]

In a google search on "python properties vs attributes", most results claim that properties are a special kind of attribute.

In many cases, the types that a constructor accepts will be more permissive than the class's attributes, or constructors will include deprecated parameters that get converted to canonical attributes. It's useful to have documentation for the canonical attributes because they describe what the class is, rather than just what arguments are required to construct it.

[sryza]

Might be worth collecting other opinions from the team? Not a hill that I'm going to die on.

[erinkcochran87]

I'll leave it to others to stamp on code, but stylistically this is a great improvement. Thank you Sean!

[smackesey]

Might be worth collecting other opinions from the team? Not a hill that I'm going to die on.

pinging @dpeng817 @yuhan

[smackesey]

@sryza ping for response to above

AFAICT this is straightforwardly correct for non-NamedTuple classes for the reasons I wrote.

[yuhan]

im on team Args but im not a super python expert. my reason is mostly the api docs generated via args look a lot better.

however, i did a quick research in popular python libs:

• https://scikit-learn.org/stable/modules/generated/sklearn.covariance.EmpiricalCovariance.html#sklearn.covariance.EmpiricalCovariance
• https://pandas.pydata.org/docs/reference/frame.html
• https://docs.python.org/3/library/collections.html#collections.OrderedDict
• https://boto3.amazonaws.com/v1/documentation/api/latest/reference/customizations/s3.html#s3-transfers

where some list those as "parameters" and some are "attributes". wonder if we could leave things as "parameters" and not differentiate them?

im fine either way. if we want to be right, it's worth checking with other folks who have more core python understanding.

[dpeng817]

What's the status here? Requesting changes to get this out of my queue.

FWIW, I'm in favor of the args approach. Formatting is clearly a step function improvement over attrs.

[dpeng817]

back to q

[graphite-app]

Graphite Automations

"Label and add CE on all Docs" took an action on this PR • (09/28/24)

3 reviewers were added and 1 label was added to this PR based on Pedram Navid's automation.