Skip to content

Enable Mypy lint #593

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 18 commits into from
Closed

Enable Mypy lint #593

wants to merge 18 commits into from

Conversation

imjuanleonard
Copy link
Contributor

@imjuanleonard imjuanleonard commented Mar 31, 2020 •
edited
Loading

What this PR does / why we need it:
Will help to know the type of functions explicitly

Which issue(s) this PR fixes:
Fixes #594

Does this PR introduce a user-facing change?:

None

@feast-ci-bot feast-ci-bot added size/L and removed size/M labels Apr 1, 2020
@imjuanleonard
Copy link
Contributor Author

/assign @woop

@imjuanleonard imjuanleonard changed the title WIP: Mypy lint Enable Mypy lint Apr 1, 2020
@imjuanleonard
Copy link
Contributor Author

/assign @khorshuheng

@imjuanleonard
Copy link
Contributor Author

/assign @zhilingc

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: imjuanleonard
To complete the pull request process, please assign woop
You can assign the PR to them by writing /assign @woop in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

woop
woop reviewed Apr 1, 2020
View reviewed changes
woop
woop reviewed Apr 1, 2020
View reviewed changes
woop
woop reviewed Apr 1, 2020
View reviewed changes
woop
woop reviewed Apr 1, 2020
View reviewed changes
sdk/python/feast/job.py Outdated
@@ -172,8 +173,11 @@ def to_chunked_dataframe(
pd.DataFrame:
Pandas DataFrame of the feature values.
"""

# Object is Avro row type object, refer to self.result function for this type
records: List[object] = []
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using object instead of the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was:

  1. The Avro Row will be populated to object thus overriding the type
  2. The records don't call any method, it is just being passed around, so it is safe
  3. After checking all the tests pass (need to tests for the e2e as well thought)
  4. I cannot find what is AvroRow filetype, do you know?
    I have tried to dig deeper into the class until I found it was reading only from args and kwargs without return type, is it byte?

Copy link
Member

Choose a reason for hiding this comment

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

Types are there to add safety. It seems like you are looking for a shortcut here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let me try to simulate this function call to Avro row, and print the type, let me see what is the type.

Although I am certain object should be sufficient, we can see the type to make sure

Copy link
Member

@woop woop Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Although I am certain object should be sufficient

Sufficient to do what?

Copy link
Contributor Author

@imjuanleonard imjuanleonard Apr 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sufficient to do what?

O yeah, the object is not sufficient, I thought it will inherit everything, but it doesn't allow you to inherit the function as well until typecasted.

I just checked it out, the fastreader.reader will return List of dict hence, have rebased the commit.

Thanks

woop
woop reviewed Apr 1, 2020
View reviewed changes
@imjuanleonard imjuanleonard force-pushed the mypy-lint branch 2 times, most recently from 6a543de to f3263ef Compare April 1, 2020 16:45
woop
woop reviewed Apr 2, 2020
View reviewed changes
woop
woop reviewed Apr 2, 2020
View reviewed changes
@imjuanleonard imjuanleonard mentioned this pull request Apr 3, 2020
Closed
@imjuanleonard imjuanleonard force-pushed the mypy-lint branch 2 times, most recently from 436cb0a to 39179d6 Compare April 6, 2020 08:17
@SwampertX SwampertX mentioned this pull request May 28, 2020
Merged
@woop
Copy link
Member

woop commented Jun 3, 2020

Closing in favor of #749

@woop woop closed this Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woop woop woop left review comments

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@pradithya pradithya Awaiting requested review from pradithya

@zhilingc zhilingc Awaiting requested review from zhilingc

Assignees

@woop woop

@zhilingc zhilingc

@khorshuheng khorshuheng

Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable Mypy linter on the lint-python side
5 participants
@imjuanleonard @feast-ci-bot @woop @zhilingc @khorshuheng