Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: start typing Content classes #1657

Closed
wants to merge 14 commits into from

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Sep 1, 2022

This PR is the first step in a series of PRs to add type information to Awkward.

I'm not yet clear on the best strategy for this, but here are some initial thoughts:

  • Deferred annotations mean we can have minimal import-time overheads
    • No need to define typing_extensions as package dependency (not sure about how this works with distributing though)
    • Can specify mypy Python version to use latest typing syntax (not sure about ergonomics of this)
  • Python 3.6 doesn't support deferred annotations (from future import __annotations__)
    • Wait until we drop 3.6
    • Use type stubs
    • Use eager annotations

@codecov
Copy link

codecov bot commented Sep 4, 2022

Codecov Report

Merging #1657 (b77062f) into main (e692946) will increase coverage by 0.00%.
The diff coverage is 78.63%.

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

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_v2/_connect/jax/__init__.py 90.47% <ø> (+1.58%) ⬆️
src/awkward/_v2/highlevel.py 71.68% <33.33%> (-0.16%) ⬇️
src/awkward/_v2/behaviors/categorical.py 73.13% <60.00%> (-8.94%) ⬇️
src/awkward/_v2/contents/content.py 75.34% <62.63%> (-0.96%) ⬇️
src/awkward/_v2/typing.py 80.00% <80.00%> (ø)
src/awkward/_v2/operations/ak_categories.py 88.88% <88.88%> (ø)
src/awkward/_v2/operations/ak_to_categorical.py 90.90% <90.90%> (ø)
src/awkward/_v2/operations/ak_from_categorical.py 93.33% <93.33%> (ø)
src/awkward/_v2/operations/__init__.py 100.00% <100.00%> (ø)
src/awkward/_v2/operations/ak_from_json.py 91.77% <100.00%> (+0.03%) ⬆️
... and 5 more

mypy.ini Outdated
@@ -0,0 +1,2 @@
[mypy]
python_version = 3.11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd set this to 3.7, if you only do one, the lowest is best, usually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still figuring out how best to do this. I want, at least initially, to avoid too many crutches to support typing. I can't find any good documentation as to how this affects mypy's interpretation of type annotations. I am hoping to use the latest union notation x | y and built-in type subscription dict[str, int] to keep annotations readable and concise, and it was my read that this specifies the version of Python that mypy will treat as the running Python when interpreting annotations. Is there a better way to do this?

Copy link
Member

@henryiii henryiii Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use those things regardless of the target version, at least in annotations. You might not be able to use them within type aliases - but it's much more "normal" to give up on modern syntax for type aliases and/or use TypeAlias as below.

IMO, there's no harm at all adding typing_extensions to the requirements - any requirement that is only required on older Pythons is safe to add, as you aren't adding a requirement for latest and future Python users. So don't try to work around that, IMO. Also, almost everything requires typing_extensions - the usual problem is if you limit it too much, not if you include it.

I recommend adding awkward.typing, and doing all conditional imports for typing/typing_extensions there.

BehaviorType: TypeAlias = "dict[str, ak._v2.highlevel.Array | ak._v2.record.Record]"

Is the "correct" way to do "nice" type aliases, I think.

Copy link
Collaborator Author

@agoose77 agoose77 Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's really me trying to avoid having lots of

`if sys.version(....):
    import ...
else:
    import ...

I think I'll just bite the bullet and add a typing submodule ;)

You can use those things regardless of the target version, at least in annotations.

Huh, so does mypy support future typing PEPs in older versions without setting python_version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In annotations, yes. It might complain if they are used outside of future annotations. But in annotations they are perfectly valid - that's kind of the point of the future import. :)

Copy link
Collaborator Author

@agoose77 agoose77 Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that PEP563 just tackled deferred evaluation. Does mypy also use this as a cue to support the latest annotations? That would be useful!

@agoose77
Copy link
Collaborator Author

This is too hard to rebase, so I'll close it in favour of a new PR.

@agoose77 agoose77 closed this Sep 26, 2022
@agoose77 agoose77 deleted the agoose77/refactor-typing-contents branch September 26, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants