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

Warn user when importing submodule without the submodules dependencies #7369

Merged
merged 9 commits into from Mar 21, 2024

Conversation

alasdairwilson
Copy link
Member

Adds a warning when user imports a submodule without having all the deps defined in that submodules extras.

Closes #7333

In [1]: from sunpy.map import Map
H:\sm2\sunpy\sunpy\util\sysinfo.py:111: ImportWarning: Importing sunpy.map without its extra dependencies may result in errors.
The following packages are not installed:
['matplotlib>=3.5.0']
To install sunpy with these dependencies use `install sunpy[map]`or `install sunpy[all]` for all extras.

@alasdairwilson alasdairwilson requested review from a team as code owners January 6, 2024 23:44
@nabobalis
Copy link
Contributor

How do we feel about at least a backport to 5.1?

@nabobalis nabobalis added the util Issues relating to sunpy.util label Jan 10, 2024
sunpy/image/__init__.py Outdated Show resolved Hide resolved
sunpy/map/__init__.py Outdated Show resolved Hide resolved
sunpy/net/__init__.py Outdated Show resolved Hide resolved
sunpy/net/__init__.py Outdated Show resolved Hide resolved
sunpy/net/__init__.py Show resolved Hide resolved
sunpy/timeseries/__init__.py Outdated Show resolved Hide resolved
sunpy/util/sysinfo.py Outdated Show resolved Hide resolved
sunpy/visualization/__init__.py Outdated Show resolved Hide resolved
sunpy/image/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Doc build is failing. At a guess it might be because one of the sub-modules you've imported warn_missing_deps into is missing an __all__, so is trying to document warn_missing_deps?

alasdairwilson and others added 4 commits January 11, 2024 09:42
tweak warning msg

prevent warning on no errors
swapped to warn user

capitilsation of comments, block warn function from export
@alasdairwilson
Copy link
Member Author

I made the _warn function private which solves import issues, and should have been done in the first place regardless.

@alasdairwilson
Copy link
Member Author

How do we feel about at least a backport to 5.1?

I don't think it is particularly bugfixxy but I don't really care if you want to backport it.

@dstansby dstansby dismissed their stale review January 11, 2024 11:03

Tests fixed

@nabobalis nabobalis added the backport 5.1 on-merge: backport to 5.1 label Jan 11, 2024
@nabobalis
Copy link
Contributor

How do we feel about at least a backport to 5.1?

I don't think it is particularly bugfixxy but I don't really care if you want to backport it.

While its not a bugfix, I feel like its a useful fix that helps anyone who installs sunpy now.

@nabobalis nabobalis added the backport 5.0 on-merge: backport to 5.0 label Jan 11, 2024
sunpy/net/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
if missing_deps:
warn_user(f"Importing sunpy.{extras} without its extra dependencies may result in errors.\n"
f"The following packages are not installed:\n{missing_deps}\n"
f"To install sunpy with these dependencies use `install sunpy[{extras}]`"
Copy link
Member

@dstansby dstansby Jan 22, 2024

Choose a reason for hiding this comment

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

Suggested change
f"To install sunpy with these dependencies use `install sunpy[{extras}]`"
f"To install sunpy with these dependencies use `pip install sunpy[{extras}]` "

Copy link
Member

Choose a reason for hiding this comment

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

I think having just install sunpy[blah] in the warning is confusing - if something is within backticks I would expect it to be a complete command I can copy/paste. So I think this should be pip install sunpy[...], and there should also be a note that if installed from conda this is a bug that should be reported (because conda install sunpy should install all the extras)

Copy link
Member

Choose a reason for hiding this comment

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

if installed from conda this is a bug that should be reported

It is potentially a bug; a user could have broken their conda environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a line below this, see if either of you like it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, hadn't gotten round to making changes, I had originally written it this way to avoid the conda/pip issue. But I am happy if it has the extra context about conda.

@nabobalis nabobalis removed request for a team January 25, 2024 16:29
@wtbarnes wtbarnes requested a review from ayshih March 14, 2024 14:39
@wtbarnes wtbarnes added the Needs Review Needs reviews before merge label Mar 14, 2024
@nabobalis nabobalis dismissed dstansby’s stale review March 20, 2024 21:52

Should be fixed now.

@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Mar 20, 2024
@nabobalis nabobalis added the Merge When CI Passes Hit that merge button when it's all green! label Mar 20, 2024
@alasdairwilson alasdairwilson merged commit d94d45b into sunpy:main Mar 21, 2024
26 of 28 checks passed
@alasdairwilson alasdairwilson deleted the check-installed-extras branch March 21, 2024 00:50
Copy link

lumberbot-app bot commented Mar 21, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 5.0
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 d94d45b08b06b26c732fa5f761251f376e9b940a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #7369: Warn user when importing submodule without the submodules dependencies'
  1. Push to a named branch:
git push YOURFORK 5.0:auto-backport-of-pr-7369-on-5.0
  1. Create a PR against branch 5.0, I would have named this PR:

"Backport PR #7369 on branch 5.0 (Warn user when importing submodule without the submodules dependencies)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app lumberbot-app bot added the Still Needs Manual Backport This PR needs manually backporting label Mar 21, 2024
Copy link

lumberbot-app bot commented Mar 21, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 5.1
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 d94d45b08b06b26c732fa5f761251f376e9b940a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #7369: Warn user when importing submodule without the submodules dependencies'
  1. Push to a named branch:
git push YOURFORK 5.1:auto-backport-of-pr-7369-on-5.1
  1. Create a PR against branch 5.1, I would have named this PR:

"Backport PR #7369 on branch 5.1 (Warn user when importing submodule without the submodules dependencies)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

alasdairwilson added a commit to alasdairwilson/sunpy that referenced this pull request Mar 21, 2024
alasdairwilson added a commit to alasdairwilson/sunpy that referenced this pull request Mar 21, 2024
alasdairwilson added a commit that referenced this pull request Mar 21, 2024
…-on-5.0

Backport PR #7369 on branch 5.0 (Warn user when importing submodule without the submodules dependencies)
alasdairwilson added a commit that referenced this pull request Mar 21, 2024
…-on-5.1

Backport PR #7369 on branch 5.1 (Warn user when importing submodule without the submodules dependencies)
@alasdairwilson alasdairwilson removed the Still Needs Manual Backport This PR needs manually backporting label Mar 21, 2024
@Cadair
Copy link
Member

Cadair commented Mar 26, 2024

For what it's worth, I don't think this PR should have been backported. It broke the DKIST CI for a start.

Cadair added a commit to Cadair/sunpy that referenced this pull request Mar 26, 2024
@Cadair Cadair mentioned this pull request Mar 26, 2024
Cadair added a commit that referenced this pull request Mar 26, 2024
…module without the submodules dependencies)"
Cadair added a commit that referenced this pull request Mar 26, 2024
…module without the submodules dependencies)"
Cadair added a commit that referenced this pull request Mar 26, 2024
nabobalis added a commit that referenced this pull request Mar 26, 2024
…369-on-5.1

Revert "Backport PR #7369 on branch 5.1 (Warn user when importing submodule without the submodules dependencies)"
nabobalis added a commit that referenced this pull request Mar 26, 2024
…369-on-5.0

Revert "Backport PR #7369 on branch 5.0 (Warn user when importing submodule without the submodules dependencies)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 Merge When CI Passes Hit that merge button when it's all green! util Issues relating to sunpy.util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning when importing map (or other subpackages) if mpl (or others?) are not installed,
6 participants