Feature/metric with dataclass or attr #80

TedBrookings · 2023-11-27T23:19:46Z

Solving the issue was not particularly easy, but the real battle is with mypy. Reviewers should be aware that both dataclasses and attr modules have weird enough issues with type hints that they basically lie; and mypy has special code to figure out what's going on. Thus I had to make use of # type: ignore in multiple locations in actual code, and had to completely exclude test_metric.py from even being checked.

Also in an offline conversation with @tfenne I dropped changing attr from a package requirement to a dev/test requirement, because several attr-decorated classes in the main fgpyo codebase use the slots and kw_only keywords, which are only supported in dataclasses for python versions >= 3.10.

codecov · 2023-11-27T23:24:02Z

Codecov Report

Attention: Patch coverage is 85.85859% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 87.80%. Comparing base (305530c) to head (10e4689).

❗ Current head 10e4689 differs from pull request most recent head cf2a4dd. Consider uploading reports for the commit cf2a4dd to get more accurate results

Files	Patch %	Lines
fgpyo/util/inspect.py	81.08%	12 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
- Coverage   94.00%   87.80%   -6.21%     
==========================================
  Files          33       16      -17     
  Lines        3403     1738    -1665     
  Branches      694      370     -324     
==========================================
- Hits         3199     1526    -1673     
- Misses        135      144       +9     
+ Partials       69       68       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TedBrookings · 2023-11-27T23:26:04Z

I should add a few tests to cover a handful of helper functions I created in util.inspect. That should likely bring codecov back up to target (or close to it)

TedBrookings · 2023-11-28T17:26:49Z

I tried to address codecov. There were three categories of issue

Lines that should be covered by tests. I added tests to cover them.
Lines that probably shouldn't be covered. e.g. try/catch blocks for ImportError when attr is missing. I could drop these since the issue goal is no longer to remove attr. It feels like a plausible someday goal, and adds relatively little cost. I'm inclined to leave those lines unchecked, but am happy to remove them if that's the preference.
Lines that are in test files! Those should not be checked at all. I added a codecov.yml file to the project to exclude them.

The final math makes codecov unhappy. I'm inclined to leave things as they stand.

clintval

Only documentation requested in places where I couldn't quite understand what some of the variables were necessary for unless I scanned elsewhere in the code. I'm assuming attrs is used elsewhere in fgpyo so we can't make it an install optional can we?

clintval · 2023-12-26T13:22:00Z

fgpyo/sam/__init__.py


-    @length.validator
-    def _validate_length(self, attribute: Any, value: int) -> None:
+    def __post_init__(self) -> None:


Codecov says this branch is uncovered which makes me think attrs is not calling __post_init__ after initialization. Could you add a unit test to ensure this isn't now dead code?

It was a typo, should have been __attrs_post_init__. Good catch, thanks! I added a test.

As for your top-level question, attrs is used extensively throughout fgpyo. The original mandate was to replace attrs usages with dataclasses, but as I mentioned in the top comment, this can't be done until we drop support for python < 3.10.

fgpyo/util/metric.py

fgpyo/util/inspect.py

clintval · 2024-02-29T21:48:16Z

fgpyo/util/inspect.py

+    def get_attr_fields(cls: type) -> Tuple[dataclasses.Field, ...]:  # type: ignore
+        return ()
+
+    def get_attr_fields_dict(cls: type) -> Dict[str, dataclasses.Field]:  # type: ignore
+        return {}


Docstrings on all public functions please.

I added them. In this case these functions will never be called, and are basically there for static checking. I added an additional comment explaining this too.

clintval · 2024-02-29T21:53:50Z

fgpyo/util/inspect.py

+def get_fields(cls: Type[DataclassesOrAttrClass]) -> Tuple[FieldType, ...]:
+    if not (is_dataclasses_class(cls) or is_attr_class(cls)):
+        raise TypeError("cls must a dataclasses or attr class")
+    return (get_dataclasses_fields(cls) if is_dataclasses_class(cls) else ()) + (
+        get_attr_fields(cls) if is_attr_class(cls) else ()  # type: ignore
+    )


Should this return a set so no one relies on the order for anything meaningful?

It turns out that you can't mix attr and dataclasses classes (the constructors are not smart enough to pear into the superclass and set the fields, so you can't even initialize such a hybrid), so I refactored this to simply return one or the other or raise ValueError.

fgpyo/util/inspect.py

fgpyo/util/metric.py

fgpyo/util/tests/test_inspect.py

clintval

This looks like a masterclass in Python type trickery.

Thanks for your hard and careful work!

My only lingering concern is exposing more inspect code to the public API and then having users accidentally (or purposefully) import some of the code when we probably don't want that to happen. How do you feel about that? Do we have any easy options since the inspect namespace is already "public"?

fgpyo/util/inspect.py

clintval · 2024-03-04T21:34:56Z

fgpyo/util/inspect.py

+
+def is_attr_class(cls: type) -> bool:  # type: ignore[arg-type]
+    """Return True if the class is an attr class, and False otherwise"""
+    return hasattr(cls, "__attrs_attrs__")


Is this unit tested?

clintval · 2024-03-04T21:35:44Z

fgpyo/util/inspect.py

+MISSING_OR_NONE = {*MISSING, None}
+DataclassesOrAttrClass = Union[DataclassesProtocol, AttrsInstance]
+FieldType: TypeAlias = Union[dataclasses.Field, Attribute]


I would still document these. It makes sense what they are, but a reader will wonder what they are meant to be used for. Document like this so Sphinx picks it up:

AttrFromType = TypeVar("AttrFromType") """TypeVar to allow attr_from to be used with either an attr class or a dataclasses class."""

clintval · 2024-03-04T21:37:42Z

fgpyo/util/inspect.py

@@ -1,16 +1,12 @@
-import functools


It's too late to propose making inspect.py "private" (e.g. _inspect.py) but I'm wondering if you think any of the public API is too tempting or dangerous for users to accidentally import and use and rely on. If yes, then we could go and prefix lots of what you added with _ to hide more things.

TedBrookings · 2024-03-18T13:34:14Z

@nh13 I think I've responded to all the substantial issues. @clintval has requested that you give a thumbs-up before I merge.

nh13

Approve based on conversation to add a little more docs to the test metric module and perhaps a type hint or two in inspect, but don't spend more than 15 minutes, and if it doesn't work it doesn't work.

* Update util.metric and related util.inspect modules to work with dataclasses or attr * Update test_metric to test both dataclasses and attr classes Closes #45

TedBrookings force-pushed the feature/metric-with-dataclass-or-attr branch 2 times, most recently from a5bec02 to a1df504 Compare November 27, 2023 23:22

TedBrookings force-pushed the feature/metric-with-dataclass-or-attr branch 2 times, most recently from 569af29 to dbfb9b8 Compare November 28, 2023 17:23

TedBrookings requested a review from tfenne November 28, 2023 17:26

TedBrookings requested a review from clintval December 12, 2023 19:24

TedBrookings assigned clintval Dec 12, 2023

clintval approved these changes Feb 29, 2024

View reviewed changes

clintval assigned TedBrookings and unassigned clintval Feb 29, 2024

TedBrookings force-pushed the feature/metric-with-dataclass-or-attr branch 2 times, most recently from 0fa5e0d to d77edd7 Compare March 4, 2024 17:50

clintval approved these changes Mar 4, 2024

View reviewed changes

TedBrookings force-pushed the feature/metric-with-dataclass-or-attr branch from 3bfbdca to 10e4689 Compare March 15, 2024 17:27

TedBrookings requested a review from nh13 March 18, 2024 13:33

TedBrookings assigned nh13 and unassigned TedBrookings Mar 18, 2024

nh13 approved these changes Mar 26, 2024

View reviewed changes

Allow Metric to use dataclasses or attr

cf2a4dd

* Update util.metric and related util.inspect modules to work with dataclasses or attr * Update test_metric to test both dataclasses and attr classes Closes #45

TedBrookings force-pushed the feature/metric-with-dataclass-or-attr branch from 88be57f to cf2a4dd Compare April 8, 2024 16:53

TedBrookings merged commit f6f00df into main Apr 8, 2024
6 checks passed

TedBrookings deleted the feature/metric-with-dataclass-or-attr branch April 8, 2024 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/metric with dataclass or attr #80

Feature/metric with dataclass or attr #80

TedBrookings commented Nov 27, 2023

codecov bot commented Nov 27, 2023 •

edited

Loading

TedBrookings commented Nov 27, 2023

TedBrookings commented Nov 28, 2023

clintval left a comment •

edited

Loading

clintval Dec 26, 2023

TedBrookings Mar 4, 2024

clintval Feb 29, 2024

TedBrookings Mar 4, 2024

clintval Feb 29, 2024

TedBrookings Mar 4, 2024

clintval left a comment

clintval Mar 4, 2024

clintval Mar 4, 2024

clintval Mar 4, 2024

TedBrookings commented Mar 18, 2024

nh13 left a comment

Feature/metric with dataclass or attr #80

Feature/metric with dataclass or attr #80

Conversation

TedBrookings commented Nov 27, 2023

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

TedBrookings commented Nov 27, 2023

TedBrookings commented Nov 28, 2023

clintval left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TedBrookings commented Mar 18, 2024

nh13 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 27, 2023 •

edited

Loading

clintval left a comment •

edited

Loading