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

fix: Add numpy as a required dependency for py client #6308

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

jmao-denver
Copy link
Contributor

and clean up setup.py a bit

Fixes #6304

@jmao-denver
Copy link
Contributor Author

@devinrsmith wondered if we should make numpy a soft (optional) dependency. I guess we could use a pattern like the following but the body of unique also references specific numpy types, which requires more code branches and I don't think it is worth the effort.

if np is None:
    Sentinel_Type = Union[str, bool]
else:
    Sentinel_Type = Union[np.number, str, bool]


def unique(cols: Union[str, List[str]] = None, include_nulls: bool = False,
           non_unique_sentinel: Sentinel_Type = None) -> Aggregation:

@rcaudy
Copy link
Member

rcaudy commented Oct 29, 2024

@devinrsmith wondered if we should make numpy a soft (optional) dependency. I guess we could use a pattern like the following but the body of unique also references specific numpy types, which requires more code branches and I don't think it is worth the effort.

if np is None:
    Sentinel_Type = Union[str, bool]
else:
    Sentinel_Type = Union[np.number, str, bool]


def unique(cols: Union[str, List[str]] = None, include_nulls: bool = False,
           non_unique_sentinel: Sentinel_Type = None) -> Aggregation:

Is this the only feature we require numpy for? If so, I'm OK with the optional route.

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I like the optional approach better, I usually prefer to limit dependencies when possible.

Also, would be good to use TypeAlias for >= 3.10 (like we do with TableDefinitionLike in table.py).

@jmao-denver
Copy link
Contributor Author

  1. I see the reason for making numpy optional, however the ROI isn't high considering the changes we need to make to the code and the doc for such a common lib
  2. Yes, TypeAlias or just type (3.12+) is the way to go for readability.

@jmao-denver jmao-denver merged commit 640ab8f into deephaven:main Oct 29, 2024
24 checks passed
@jmao-denver jmao-denver deleted the 6304-missing-numpy-dep branch October 29, 2024 18:32
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

numpy is missing from the required dependencies of pydeephaven
4 participants