Fix: issue #1297: datasaver does not work with __future__.annotations

[kreczko]

Fixes issue #1297 by changing return_annotation retrieval function_modifiers.adapters.datasaver.validate.

Changes

• modified datasaver.validate to check against __future__.annotations compatible type hints

How I tested this

• added from __future__ import annotations to test_adapters
• ran pytest tests/function_modifiers → test failed
• made above changes
• ran pytest tests/function_modifiers → test succeeded

Notes

`correct_ds_function` in `test_adapters.py` uses an alias for `dict`: `dict_ = dict`. However, for the string comparison this is not resolved to `dict`; added `dict_` to the validation for now.

I do not understand why this is done in the tests and should, in theory, not impact "normal" use.
Feedback on this is welcome.

Checklist

• PR has an informative and human-readable title (this will be pulled into the release notes)
• Changes are limited to a single goal (no scope creep)
• Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future TODOs are captured in comments
• Project documentation has been updated if adding/changing functionality.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 422b9a7 in 2 minutes and 36 seconds

More details

• Looked at 23 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. hamilton/function_modifiers/adapters.py:863

• Draft comment:
  When checking the return annotation, consider using typing.get_type_hints to resolve forward references. Relying on string comparisons (e.g. "dict", "dict_") may not be robust if users define custom aliases or in more complex scenarios. If possible, resolve the type hints once to compare against the actual type objects.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment makes a valid technical point - string comparison of type annotations is generally less robust than using typing.get_type_hints(). However, looking at the full context:

1. The code already uses typing.get_type_hints() earlier to get the return annotation
2. This is a validation function specifically for checking dict return types
3. The string comparisons handle common cases like "dict" forward references
4. The suggested change would actually be more restrictive by removing support for string annotations
   The comment's suggestion could make the type checking more brittle by removing support for string-based forward references that the current code handles. The current approach, while not perfect, may be intentionally more permissive.
   While using typing.get_type_hints() is generally good practice, in this specific validation context the current string comparison approach provides useful flexibility for handling forward references without adding significant risk.
   The comment should be deleted as the current implementation is an intentional design choice that provides more flexibility for handling forward references, and changing it could break valid use cases.

2. tests/function_modifiers/test_adapters.py:1

• Draft comment:
  Adding 'from future import annotations' is appropriate to test forward references. Document this requirement in the Sphinx docs under 'docs/annotations.rst' (or similar) for contributors.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. hamilton/function_modifiers/adapters.py:863

• Draft comment:
  Consider using typing.get_type_hints(fn)['return'] to automatically resolve future.annotations instead of the manual string comparisons. If retaining the current approach, add a comment referencing issue #1297 explaining the inclusion of 'dict' and 'dict_'.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

4. hamilton/function_modifiers/adapters.py:343

• Draft comment:
  Typo detected: In the docstring of the load_from__meta__ class, 'we couold add attributes dynamically' should be 'we could add attributes dynamically'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. tests/function_modifiers/test_adapters.py:195

• Draft comment:
  In the comment on line 195, consider replacing 'cause' with 'because' to improve clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. tests/function_modifiers/test_adapters.py:760

• Draft comment:
  On line 760, correct the spelling of 'guarenteed' to 'guaranteed' in the reason message for pytest.skipif.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_oFF0ooRiGtGIF5P3

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[kreczko]

The bot makes a good suggestion: instead of my change, it is better to change
# return_annotation = inspect.signature(fn).return_annotation to return_annotation = typing.get_type_hints(fn)["return"]
which does resolve "dict_" and "dict" to dict.
As a result, the block if return_annotation is inspect.Signature.empty can be replaced with if return_annotation is None.
I assume that's preferable and make the change

[elijahbenizzy]

Thank you! This is great -- I've been meaning to make this change to everywhere in the codebase and new spots keep popping up in which it's needed.

Just checking -- does the change to test_adapters mean that this won't work if we don't use from __future__ import annotations?

[kreczko]

Just checking -- does the change to test_adapters mean that this won't work if we don't use from __future__ import annotations?

Good point, did not test this.

--- a/tests/function_modifiers/test_adapters.py
+++ b/tests/function_modifiers/test_adapters.py
@@ -1,5 +1,3 @@
-from __future__ import annotations
-

pytest -s tests/function_modifiers/
======================================= 383 passed, 13 warnings in 3.21s =======================================

luckily same result.

Out of curiosity, I tried this example:

class B(dict):

    pass

def func() -> B:
    return B()

def func2() -> dict:
    return dict()

import inspect
import typing


print(inspect.signature(func).return_annotation)
print(inspect.signature(func2).return_annotation)

print(typing.get_type_hints(func)["return"])
print(typing.get_type_hints(func2)["return"])

without annotations they are identical

inspect: <class '__main__.B'>
inspect: <class 'dict'>
typing: <class '__main__.B'>
typing: <class 'dict'>

with from __future__ import annotations the inspect ones turn to string:

inspect: B
inspect: dict
typing: <class '__main__.B'>
typing: <class 'dict'>

Hope this helps

[skrawcz]

@elijahbenizzy bump

[elijahbenizzy]

Thanks!