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.

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

Add static typing to labeler models #672

Merged
merged 17 commits into from
Oct 6, 2022
Merged

Add static typing to labeler models #672

merged 17 commits into from
Oct 6, 2022

Conversation

tonywu315
Copy link
Contributor

@taylorfturner taylorfturner added the static_typing mypy static typing issues label Sep 30, 2022
taylorfturner
taylorfturner reviewed Sep 30, 2022
View reviewed changes
Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

generally good after inital read... just some comments

dataprofiler/labelers/base_model.py Outdated Show resolved Hide resolved
dataprofiler/labelers/base_model.py Outdated Show resolved Hide resolved
dataprofiler/labelers/base_model.py Outdated Show resolved Hide resolved
dataprofiler/labelers/regex_model.py Outdated Show resolved Hide resolved
@taylorfturner taylorfturner enabled auto-merge (squash) September 30, 2022 21:16
auto-merge was automatically disabled October 3, 2022 13:44

Head branch was pushed to by a user without write access

JGSweets
JGSweets reviewed Oct 3, 2022
View reviewed changes
dataprofiler/labelers/character_level_cnn_model.py Outdated
reset_weights=False,
verbose=True,
):
train_data: Union[pd.DataFrame, pd.Series, np.ndarray],
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should consider making an alias for Union[pd.DataFrame, pd.Series, np.ndarray] and utilizing this everywhere instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah I like this idea! https://docs.python.org/3.9/library/typing.html#type-aliases

@taylorfturner taylorfturner enabled auto-merge (squash) October 3, 2022 14:39
taylorfturner
taylorfturner requested changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

comments (recommend aliasing where possible) and request around setting something to None by default without Optional[] typing

dataprofiler/labelers/char_load_tf_model.py Show resolved Hide resolved
dataprofiler/labelers/character_level_cnn_model.py Outdated
reset_weights=False,
verbose=True,
):
train_data: Union[pd.DataFrame, pd.Series, np.ndarray],
Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah I like this idea! https://docs.python.org/3.9/library/typing.html#type-aliases

dataprofiler/labelers/character_level_cnn_model.py Outdated Show resolved Hide resolved
auto-merge was automatically disabled October 4, 2022 18:11

Head branch was pushed to by a user without write access

taylorfturner
taylorfturner reviewed Oct 5, 2022
View reviewed changes
dataprofiler/labelers/char_load_tf_model.py Outdated
@@ -18,14 +22,18 @@
logger = dp_logging.get_child_logger(__name__)
labeler_utils.hide_tf_logger_warnings()

Data = Union[pd.DataFrame, pd.Series, np.ndarray]
Copy link
Contributor

Choose a reason for hiding this comment

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

@tonywu315 love it --can we make this used everywhere? I see other places in other files where Union[pd.DataFrame, pd.Series, np.ndarray] is used but not with an alias. I'd recommend doing this so we don't have to repeat in other files too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to utils.py

Copy link
Contributor

Choose a reason for hiding this comment

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

dope

@taylorfturner taylorfturner enabled auto-merge (squash) October 5, 2022 13:49
taylorfturner
taylorfturner previously approved these changes Oct 5, 2022
View reviewed changes
auto-merge was automatically disabled October 5, 2022 14:43

Head branch was pushed to by a user without write access

@taylorfturner taylorfturner enabled auto-merge (squash) October 5, 2022 14:49
taylorfturner
taylorfturner reviewed Oct 5, 2022
View reviewed changes
dataprofiler/labelers/character_level_cnn_model.py Outdated
@@ -653,7 +670,7 @@ def fit(
history[metric_label] = model_results[i]

if val_data:
f1, f1_report = self._validate_training(val_data)
f1, f1_report = self._validate_training(val_data) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

let's see how we might be able to remove the #type: ignore and maybe remove if val_data on line 672

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed redundant if val_data

auto-merge was automatically disabled October 5, 2022 15:16

Head branch was pushed to by a user without write access

JGSweets
JGSweets reviewed Oct 5, 2022
View reviewed changes
dataprofiler/labelers/utils.py Outdated
import numpy as np
import pandas as pd

DataArray = Union[pd.DataFrame, pd.Series, np.ndarray]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to create our own. I might suggest we start following a similar pardigm to numpy and utilize our own _typing folder or _typing.py file at the root for universal typing that can be applied throughout the repo.

https://github.com/numpy/numpy/tree/main/numpy/_typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added _typing.py

tonywu315 and others added 3 commits October 5, 2022 14:44
@tonywu315 @taylorfturner
Change f1_report default to None
467f1cf 
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
@tonywu315
Create _typing.py
b5f02f1
@tonywu315
Fix default value
b52dfa5
@taylorfturner taylorfturner enabled auto-merge (squash) October 6, 2022 14:31
@taylorfturner taylorfturner merged commit 356b0f9 into capitalone:main Oct 6, 2022
@tonywu315 tonywu315 deleted the static_typing/labelers/model branch October 6, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JGSweets JGSweets JGSweets left review comments

@taylorfturner taylorfturner taylorfturner approved these changes

@micdavis micdavis micdavis approved these changes

@ksneab7 ksneab7 Awaiting requested review from ksneab7

@tyfarnan tyfarnan Awaiting requested review from tyfarnan

Assignees

@tonywu315 tonywu315

Labels
static_typing mypy static typing issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tonywu315 @JGSweets @taylorfturner @micdavis