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

fix wrong exception in lazy import __getattr__ #15741

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix wrong exception in lazy import __getattr__ #15741

wants to merge 1 commit into from
+37 −14

Conversation

protagonyist
Copy link

@protagonyist protagonyist commented Oct 17, 2024 •
edited
Loading

raise AttributeError instead of ModuleNotFoundError in __getattr__ in prefect/__init__.py (issue #15739)

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

@protagonyist
Copy link
Author

closes #15739

@protagonyist protagonyist changed the title fix wrong exception in lazy import __getattr__ (#15739) fix wrong exception in lazy import __getattr__ Oct 17, 2024
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Oct 17, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #15741 will not alter performance

Comparing protagonyist:fix-getattr-lazy-import (b6b6d03) with main (cd4994c)

Summary

✅ 3 untouched benchmarks

@desertaxle desertaxle linked an issue Oct 17, 2024 that may be closed by this pull request
ModuleNotFoundError when importing skops after import prefect (prefect 3) #15739
Open
@desertaxle
Copy link
Member

Thank you so much for the PR @protagonyist! Could you please add a test checking that incorrect imports from prefect raise an AttributeError to guard against future regression?

@protagonyist
Copy link
Author

@desertaxle added test case to tests/test_exceptions.py

zzstoatzz
zzstoatzz approved these changes Oct 18, 2024
View reviewed changes
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

lgtm! one small nit on naming

thank you @protagonyist

tests/test_exceptions.py Outdated Show resolved Hide resolved
zzstoatzz
zzstoatzz requested changes Oct 18, 2024
View reviewed changes
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

oops! @desertaxle made a good point, which is that we should also have a case testing

from prefect import foo

@protagonyist
Copy link
Author

Fixed AttributeError test case name, added tests for from prefect import foo and import prefect.foo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zzstoatzz zzstoatzz zzstoatzz requested changes

@aaazzam aaazzam Awaiting requested review from aaazzam aaazzam is a code owner

@chrisguidry chrisguidry Awaiting requested review from chrisguidry chrisguidry is a code owner

@cicdw cicdw Awaiting requested review from cicdw cicdw is a code owner

@desertaxle desertaxle Awaiting requested review from desertaxle desertaxle is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ModuleNotFoundError when importing skops after import prefect (prefect 3)
3 participants
@protagonyist @desertaxle @zzstoatzz