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

Change function signature #6459

Merged
merged 2 commits into from
Mar 27, 2019
Merged

Change function signature #6459

merged 2 commits into from
Mar 27, 2019

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Mar 27, 2019

The type NCFDataset is used in the type declaration on line 81 but it is never imported.

flake8 testing of https://github.com/tensorflow/models on Python 3.7.1

$ flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics

./official/recommendation/data_preprocessing.py:180:3: F821 undefined name 'NCFDataset'
  # type: (str, str, dict, typing.Optional[str], bool, typing.Optional[str]) -> (NCFDataset, typing.Callable)
  ^
1    F821 undefined name 'NCFDataset'
1

E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. These 5 are different from most other flake8 issues which are merely "style violations" -- useful for readability but they do not effect runtime safety.

  • F821: undefined name name
  • F822: undefined name name in __all__
  • F823: local variable name referenced before assignment
  • E901: SyntaxError or IndentationError
  • E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

The type __NCFDataset__ is used in the type declaration on line 81 but it is never imported.

[flake8](http://flake8.pycqa.org) testing of https://github.com/tensorflow/models on Python 3.7.1

$ __flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics__
```
./official/recommendation/data_preprocessing.py:180:3: F821 undefined name 'NCFDataset'
  # type: (str, str, dict, typing.Optional[str], bool, typing.Optional[str]) -> (NCFDataset, typing.Callable)
  ^
1    F821 undefined name 'NCFDataset'
1
```
__E901,E999,F821,F822,F823__ are the "_showstopper_" [flake8](http://flake8.pycqa.org) issues that can halt the runtime with a SyntaxError, NameError, etc. These 5 are different from most other flake8 issues which are merely "style violations" -- useful for readability but they do not effect runtime safety.
* F821: undefined name `name`
* F822: undefined name `name` in `__all__`
* F823: local variable name referenced before assignment
* E901: SyntaxError or IndentationError
* E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree
@cclauss cclauss requested review from karmel and a team as code owners March 27, 2019 18:34
@robieta robieta requested review from robieta and removed request for karmel March 27, 2019 19:48
@robieta
Copy link
Contributor

robieta commented Mar 27, 2019

I think the type annotation is just incorrect. I believe it should be:
(int, int, data_pipeline.BaseDataConstructor)

@cclauss cclauss changed the title from NCF_input import NCFDataset for line 181 Change function signature Mar 27, 2019
@cclauss
Copy link
Contributor Author

cclauss commented Mar 27, 2019

What typechecker does Kokoro run? Mypy, ptype, pyright?

@robieta
Copy link
Contributor

robieta commented Mar 27, 2019

Alas, none of the above. If you'd like, feel free to open a PR updating the
linting portion of the presubmit script with your recommendation. Just keep in mind that:

  1. It will run under Python 2.7 (I know, I know. Doomsday clock and all.)
  2. Most of the code isn't type annotated.

Copy link
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@robieta robieta merged commit 0b2b899 into tensorflow:master Mar 27, 2019
@cclauss cclauss deleted the patch-4 branch March 27, 2019 20:37
wlongxiang pushed a commit to wlongxiang/models that referenced this pull request Apr 24, 2019
* from NCF_input import NCFDataset for line 181

The type __NCFDataset__ is used in the type declaration on line 81 but it is never imported.

[flake8](http://flake8.pycqa.org) testing of https://github.com/tensorflow/models on Python 3.7.1

$ __flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics__
```
./official/recommendation/data_preprocessing.py:180:3: F821 undefined name 'NCFDataset'
  # type: (str, str, dict, typing.Optional[str], bool, typing.Optional[str]) -> (NCFDataset, typing.Callable)
  ^
1    F821 undefined name 'NCFDataset'
1
```
__E901,E999,F821,F822,F823__ are the "_showstopper_" [flake8](http://flake8.pycqa.org) issues that can halt the runtime with a SyntaxError, NameError, etc. These 5 are different from most other flake8 issues which are merely "style violations" -- useful for readability but they do not effect runtime safety.
* F821: undefined name `name`
* F822: undefined name `name` in `__all__`
* F823: local variable name referenced before assignment
* E901: SyntaxError or IndentationError
* E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

* int, int, data_pipeline.BaseDataConstructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants