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

Explicitly import distributions from torch #3333

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Explicitly import distributions from torch #3333

merged 3 commits into from
Feb 27, 2024

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Feb 26, 2024

Addresses #3332 #2550

This attempts to resolve mypy attribute undefined errors for dist.Normal and others. After these fixes, I am able to use VS Code to (1) hover over dist.Normal to read docs, and (2) detect type errors in e.g. dist.Abnormal.

  1. Add --config-settings editable_mode=strict to make install which helps mypy
  2. Explicitly add py.typed to package_data for setuptools<69
  3. Explicitly add distributions to __all__ in pyro.distributions.torch
  4. Explicitly add distributions to __all__ in pyro.distributions
  5. Runs make format with the latest black
  • Question: will this break anyone's installs? E.g. if torch renames a distribution, we might get an import error in pyro.distributions.
    Answer: No, since we're using both import * and try...except around explicit imports.

ordabayevy
ordabayevy previously approved these changes Feb 27, 2024
Copy link
Member

@ordabayevy ordabayevy 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 having explicit imports. If a distribution gets renamed or a new distribution gets added maybe we can try to mitigate it by putting try/except guards to work around import errors?

@fritzo
Copy link
Member Author

fritzo commented Feb 27, 2024

@ordabayevy Great, I've added try..except around the explicit imports, and also used the same implicit+explicit pattern in pyro.distributions.transforms and pyro.distributions.constraints.

Copy link
Member

@ordabayevy ordabayevy left a comment

Choose a reason for hiding this comment

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

Looks great!

@fritzo fritzo merged commit e8af7cd into dev Feb 27, 2024
9 checks passed
@ordabayevy ordabayevy deleted the dist-explicit-import branch February 27, 2024 21:53
@akotlar
Copy link

akotlar commented May 24, 2024

We just ran into this; gentle suggestion to cut a patch release with this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants