Repository: omni-us/jsonargparse
Pull request #643 (2024-12-12 16:03:29)

Nested `attrs.define` dataclasses not serialized correctly when a subfield is also an `attrs.define` dataclass marked with `attrs.field`
## What does this PR do?

I am trying to see if I can convert a bunch of internal `dataclasses.dataclass` config objects to be defined with `attrs.define` instead for field validation and conversion.

I've noticed that `jsonargparse` handles simple cases with `attrs`-style dataclasses, but it breaks down with nested `attrs`-style dataclasses ONLY when the subfield is marked with an `attrs.field` descriptor, such as when using a default factory.

Here is a simple example:

```python
from attrs import define, field
from jsonargparse import ArgumentParser

@define
class SubField:
    x: int = 0
    y: int = 1

@define
class Args:
    a: int
    subfield: SubField = field(factory=SubField)

parser = ArgumentParser()
parser.add_argument("--args", type=Args)
```

which leads to this call stack:

```txt
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/site-packages/jsonargparse/_core.py", line 127, in add_argument
    self.add_dataclass_arguments(kwargs.pop("type"), nested_key, **kwargs)
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/site-packages/jsonargparse/_signatures.py", line 476, in add_dataclass_arguments
    self._add_signature_parameter(
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/site-packages/jsonargparse/_signatures.py", line 416, in _add_signature_parameter
    action = container.add_argument(*args, **kwargs)
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/site-packages/jsonargparse/_core.py", line 127, in add_argument
    self.add_dataclass_arguments(kwargs.pop("type"), nested_key, **kwargs)
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/site-packages/jsonargparse/_signatures.py", line 471, in add_dataclass_arguments
    defaults = dataclass_to_dict(default)
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/site-packages/jsonargparse/_signatures.py", line 623, in dataclass_to_dict
    return dataclasses.asdict(value)
  File "/scratch/ccmartin6/miniconda3/envs/pst/lib/python3.10/dataclasses.py", line 1237, in asdict
    raise TypeError("asdict() should be called on dataclass instances")
TypeError: asdict() should be called on dataclass instances
```

Thus, it would appear that the only change is needed in `jsonargparse._signatures.dataclass_to_dict`. **More specifically, this pull request does the following in terms of code changes**:
1. After checking if a dataclass-like object is a `pydantic` dataclass, it will check if the `attrs` library is available using the flag already computed in `jsonargparse._optional`
2. If `attrs` is available, then it will check if the input dataclass-like object is an `attrs.define`-style dataclass.
3. If yes, then use `attrs.asdict` to serialize the model instead of `dataclasses.asdict`

### Alternatives
There are several alternatives to implementing this pull request that all fall short in some way.

#### 1. `jsonargparse.lazy_instance`
This makes the `attrs.define`-style dataclass work with `jsonargparse` and is possibly the simplest alternative, but it does NOT allow creating an instance with no init args:

```python
from attrs import define
from jsonargparse import ArgumentParser, lazy_instance

@define
class SubField:
    x: int = 0
    y: int = 1

@define
class Args:
    a: int
    subfield: SubField = lazy_instance(SubField)

>>> parser = ArgumentParser()
>>> parser.add_argument("--args", type=Args)
>>> parser.parse_args(["--args.a", "2"])
Namespace(args=Namespace(a=2, subfield=Namespace(x=0, y=1)))

>>> Args(a=2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<attrs generated repr __main__.Args>", line 13, in __repr__
  File "<attrs generated repr __main__.SubField>", line 13, in __repr__
AttributeError: 'LazyInstance_SubField' object has no attribute 'x'
```

which is not desired when these dataclasses are not just intermediates between command-line inputs and the rest of the code base.

#### 2. Don't make the subfields have default values
This is similar to the above in that `jsonargparse` can handle this case and will correctly see that `SubField` has default values. However, this has the same problem that you cannot create a default `Args` object without init values. There is a further complication with field order when introducing required values.

```python
from attrs import define
from jsonargparse import ArgumentParser, lazy_instance

@define
class SubField:
    x: int = 0
    y: int = 1

@define
class Args:
    a: int
    subfield: SubField 

>>> parser = ArgumentParser()
>>> parser.add_argument("--args", type=Args)
>>> parser.parse_args(["-h"])
usage: [-h] [--args CONFIG] --args.a A [--args.subfield CONFIG] [--args.subfield.x X] [--args.subfield.y Y]

options:
  -h, --help            Show this help message and exit.

Method generated by attrs for class Args:
  --args CONFIG         Path to a configuration file.
  --args.a A            (required, type: int)

Method generated by attrs for class SubField:
  --args.subfield CONFIG
                        Path to a configuration file.
  --args.subfield.x X   (type: int, default: 0)
  --args.subfield.y Y   (type: int, default: 1)

>>> Args(a=2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Args.__init__() missing 1 required positional argument: 'subfield'
```

#### 3. Make use of `attrs.field` converters
This technically works on the code end for constructing default values and when using `jsonargparse.CLI`, but the fields on the `SubField` are not shown in the CLI help page, which is not ideal. 

```python
from typing import Optional, TypeVar
from functools import partial
from jsonargparse import ArgumentParser, CLI
from attrs import define, field

T = TypeVar("T")

@define
class SubField:
    a: int = 0
    b: str = "1"

def convert_default_sub_dataclasses(value: Optional[T], cls: type[T]) -> T:
    if value is None:
        return cls()
    return value

@define
class Args:
    c: float
    d: Optional[SubField] = field(default=None, converter=partial(convert_default_sub_dataclasses, cls=SubField))

def main(args: Args):
    print(args)
    return args

>>> code_default = Args(c=1.2)
>>> code_default
Args(c=1.2, d=SubField(a=0, b='1'))

>>> cli_default = CLI(main, args=["--args.c=1.2"])
Args(c=1.2, d=SubField(a=0, b='1'))

>>> code_default == cli_default
True

>>> parser = ArgumentParser()
>>> parser.add_argument("--args", type=Args)
>>> parser.parse_args(["-h"])
usage: [-h] [--args CONFIG] --args.c C [--args.d D]

options:
  -h, --help     Show this help message and exit.

Method generated by attrs for class Args:
  --args CONFIG  Path to a configuration file.
  --args.c C     (required, type: float)
  --args.d D     (type: Optional[SubField], default: null)
```

#### 4. Don't nest `attrs.define`-style dataclasses
This would obviously solve the problems without code changes, but considering that nested `dataclasses.dataclass` objects are handled AND this lib should not impose that kind of design decision on users, this is not really that great of a choice. Further, the required changes are extremely minimal. 

## Before submitting

<!--
Use the following list as tasks to be completed before marking a pull request as
"Ready for review". Fill with an "x" all tasks done. Use "n/a" if you think a
task is not relevant or leave empty when in doubt.
-->

- [x] Did you read the [contributing guideline](https://github.com/omni-us/jsonargparse/blob/main/CONTRIBUTING.rst)?
- [n/a] Did you update **the documentation**? (readme and public docstrings)
- [x] Did you write **unit tests** such that there is 100% coverage on related code? (required for bug fixes and new features)
- [x] Did you verify that new and existing **tests pass locally**?
- [x] Did you make sure that all changes preserve **backward compatibility**?
- [x] Did you update **the CHANGELOG**? (not for typos, docs, test updates, or minor internal changes/refactors)
  - I added a section, but I was unsure how this might fit into the indicated changes for `v4.35.0` since I only tested with the latest available release `v4.34.1`


mauvilsa: Figured out why tests were not being run. Github has a new "merge experience" which I had active. Apparently that doesn't have button to approve runs for new contributors.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/omni-us/jsonargparse/pull/643?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=omni-us) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 100.00%. Comparing base [(`8f22f69`)](https://app.codecov.io/gh/omni-us/jsonargparse/commit/8f22f69ad3d4052de1fe67caa856d5c1f133ea45?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=omni-us) to head [(`9bd7fba`)](https://app.codecov.io/gh/omni-us/jsonargparse/commit/9bd7fba80995fa8173c5ae2e9624c5d2cec5e3f0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=omni-us).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff            @@
##              main      #643   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         6499      6504    +5     
=========================================
+ Hits          6499      6504    +5     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/omni-us/jsonargparse/pull/643?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=omni-us).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=omni-us).

cody-mar10: Hi, thanks for responding and taking a look at this. I believe that I have made the changes you requested 
cody-mar10: Also, I am not sure why 2 of the tests are failing, since the error messages aren't that helpful

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index ff1ac63..9a58560 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -19,6 +19,9 @@ Fixed
 ^^^^^
 - Help for ``Protocol`` types not working correctly (`#645
   <https://github.com/omni-us/jsonargparse/pull/645>`__).
+- Adding ``attrs.define`` dataclasses with nested dataclasses that are marked with
+  ``attrs.field`` (such as for a default factory) are not parsed correctly (`#643
+  <https://github.com/omni-us/jsonargparse/pull/643>`__)
 
 
 v4.35.0 (2024-12-16)
@@ -52,7 +55,6 @@ Deprecated
   config argument as ``--print_%s`` instead of being always ``--print_config``
   (`#630 <https://github.com/omni-us/jsonargparse/pull/630>`__).
 
-
 v4.34.1 (2024-12-02)
 --------------------
 
diff --git a/jsonargparse/_signatures.py b/jsonargparse/_signatures.py
index ee27720..8b8afe2 100644
--- a/jsonargparse/_signatures.py
+++ b/jsonargparse/_signatures.py
@@ -17,12 +17,8 @@ from ._common import (
     is_subclass,
 )
 from ._namespace import Namespace
-from ._optionals import get_doc_short_description, is_pydantic_model, pydantic_support
-from ._parameter_resolvers import (
-    ParamData,
-    get_parameter_origins,
-    get_signature_parameters,
-)
+from ._optionals import attrs_support, get_doc_short_description, is_pydantic_model, pydantic_support
+from ._parameter_resolvers import ParamData, get_parameter_origins, get_signature_parameters
 from ._typehints import (
     ActionTypeHint,
     LazyInitBaseClass,
@@ -575,6 +571,13 @@ def dataclass_to_dict(value) -> dict:
         pydantic_model = is_pydantic_model(type(value))
         if pydantic_model:
             return value.dict() if pydantic_model == 1 else value.model_dump()
+
+    if attrs_support:
+        import attrs
+
+        is_attrs_dataclass = attrs.has(type(value))
+        if is_attrs_dataclass:
+            return attrs.asdict(value)
     return dataclasses.asdict(value)
 
 


Test patch:
diff --git a/jsonargparse_tests/test_dataclass_like.py b/jsonargparse_tests/test_dataclass_like.py
index bce6865..4a75345 100644
--- a/jsonargparse_tests/test_dataclass_like.py
+++ b/jsonargparse_tests/test_dataclass_like.py
@@ -8,13 +8,7 @@ from unittest.mock import patch
 import pytest
 import yaml
 
-from jsonargparse import (
-    ArgumentError,
-    ArgumentParser,
-    Namespace,
-    compose_dataclasses,
-    lazy_instance,
-)
+from jsonargparse import ArgumentError, ArgumentParser, Namespace, compose_dataclasses, lazy_instance
 from jsonargparse._namespace import NSKeyError
 from jsonargparse._optionals import (
     attrs_support,
@@ -25,10 +19,7 @@ from jsonargparse._optionals import (
     typing_extensions_import,
 )
 from jsonargparse.typing import PositiveFloat, PositiveInt, final
-from jsonargparse_tests.conftest import (
-    get_parser_help,
-    skip_if_docstring_parser_unavailable,
-)
+from jsonargparse_tests.conftest import get_parser_help, skip_if_docstring_parser_unavailable
 
 annotated = typing_extensions_import("Annotated")
 type_alias_type = typing_extensions_import("TypeAliasType")
@@ -942,6 +933,21 @@ if attrs_support:
         def __attrs_post_init__(self):
             self.p1 = {}
 
+    @attrs.define
+    class AttrsSubField:
+        p1: str = "-"
+        p2: int = 0
+
+    @attrs.define
+    class AttrsWithNestedDefaultDataclass:
+        p1: float
+        subfield: AttrsSubField = attrs.field(factory=AttrsSubField)
+
+    @attrs.define
+    class AttrsWithNestedDataclassNoDefault:
+        p1: float
+        subfield: AttrsSubField
+
 
 @pytest.mark.skipif(not attrs_support, reason="attrs package is required")
 class TestAttrs:
@@ -978,3 +984,13 @@ class TestAttrs:
         assert cfg == Namespace()
         init = parser.instantiate_classes(cfg)
         assert init.data.p1 == {}
+
+    def test_nested_with_default(self, parser):
+        parser.add_argument("--data", type=AttrsWithNestedDefaultDataclass)
+        cfg = parser.parse_args(["--data.p1=1.23"])
+        assert cfg.data == Namespace(p1=1.23, subfield=Namespace(p1="-", p2=0))
+
+    def test_nested_without_default(self, parser):
+        parser.add_argument("--data", type=AttrsWithNestedDataclassNoDefault)
+        cfg = parser.parse_args(["--data.p1=1.23"])
+        assert cfg.data == Namespace(p1=1.23, subfield=Namespace(p1="-", p2=0))