-
-
Notifications
You must be signed in to change notification settings - Fork 2k
tensorflow feature columns #10052
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
tensorflow feature columns #10052
Conversation
| already_sorted: bool = False | ||
|
|
||
| class RaggedFeature(NamedTuple): | ||
| class RowSplits(NamedTuple): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mypy dislikes and I'm unsure how to type this. The real code uses collections.namedtuple and does have nested namedtuples. But mypy gives an error of,
Invalid statement in NamedTuple definition; expected "field_name: field_type [= default]"
because of them. pyright is happy with them. I could move the nested namedtuples out and having them marked private like, _RowSplits(NamedTuple). That does disagree with runtime as RaggedFeature.RowSplits is the real way to access them, but unsure how to deal with it. Or maybe just type: ignore[misc] for them?
This comment has been minimized.
This comment has been minimized.
| # typing.NamedTuple because they use multiple inheritance with other non namedtuple classes. | ||
| # _cls instead of cls is because collections.namedtuple uses _cls for __new__. | ||
| class NumericColumn(DenseColumn): | ||
| key: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As these are attributes from collections.namedtuple subclassing they are read only and could maybe be marked property? Sorta neutral to being more precise here unsure namedtuple fields are properties either. Especially as right now it's little more lenient and all these classes aren't documented so being stricter seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| class FixedLenFeature(NamedTuple): | ||
| shape: _ShapeLike | ||
| dtype: _DTypeLike | ||
| default_value: _TensorCompatible | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File "/Users/pa-loaner/Snapchat/Dev/.venvs/typeshed/lib/python3.9/site-packages/pytype/ast/visitor.py", line 55, in _call_visitor
return visitor(node)
File "/Users/pa-loaner/Snapchat/Dev/.venvs/typeshed/lib/python3.9/site-packages/pytype/pyi/parser.py", line 477, in visit_AnnAssign
raise ParseError(msg)
pytype.pyi.types.ParseError: File: "stubs/tensorflow/tensorflow/io/__init__.pyi", line 56
default_value: _TensorCompatible | None = None
^
ParseError: Default value for default_value: typing.Union can only be '...', got None
I think this error happens with any default value in NamedTuple in a stub file. A more minimal example that also triggers an error like this is,
class Foo(NamedTuple):
x: bool = FalseI'll file a bug report on pytype as well. This only happens with .pyi files and namedtuple defaults work fine for .py files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also isn't an error that would be good to pytype_exclude_list as right now it causes a parse error so any file that imports this one also fails transitively and I think many tensorflow files would wind up needing to be excluded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My workaround is just drop defaults for now and use ... and I'll add them back later after pytype allows namedtuple default values.
|
I think this is first typeshed pr I've had that found bug report for all 3 type checkers used in CI. A different bug for each one although all related sort of to NamedTuples. |
This comment has been minimized.
This comment has been minimized.
Wow, you really hit the jackpot on this one! |
| from _typeshed import Incomplete | ||
| from collections.abc import Iterable | ||
|
|
||
| def rmtree(path: str) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double-checking, should these use _typeshed.StrOrPath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to be yes. Just tested and listdir works with Path[str]. Will test each manually and update them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of them support Path except for glob. Also discovered they all support both str/bytes paths.
|
Also looks like a bunch of the |
pyrightconfig.json
Outdated
| "stubs/**/@tests/test_cases", | ||
| // Bug in pyright that can't be ignored as it passes this one, | ||
| // but fails pyrightconfig.stricter.json. Can be removed after | ||
| // upgrading to 1.1.304 and removing ignores. | ||
| "stubs/tensorflow/tensorflow/io/__init__.pyi", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed now, as per #10052 (comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it can be removed. I'll cleanup pyright ignores/this later today.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Summary
Add stubs for feature_column api. They fully cover documented api.
Most of functions return undocumented FeatureColumn classes. I've included stubs for FeatureColumn classes with some common easier methods, but those are incomplete and as they're all undocumented I did not put fallback getattr for each one.